[Mesa-dev] [PATCH 1/2] nir/dead_cf: Don't crash on unreachable after-loop blocks

[Connor Abbott]

On Thu, Sep 1, 2016 at 6:19 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
> On Thu, Sep 1, 2016 at 12:41 PM, Connor Abbott <cwabbott0 at gmail.com> wrote:
>>
>> As-is, this change will make us delete trivially infinite loops (i.e.
>> loops with no break statement). The most likely scenario where we get
>> one of those (besides for silly Piglit tests) is a developer with an
>> accidental bug in their shader. In that case, it seems kinda mean to
>> then delete the entire loop and pretend like it doesn't exist. Maybe
>> add something like
>>
>> if (!after->imm_dom)
>>    return false;
>>
>> with a comment explaining what's going on? This would also help us
>> catch other places where we don't handle infinite loops correctly that
>> might've been hidden by this.
>
>
> I wrote that patch and kicked it off to Jenkins and it passed just fine.
> However, I don't know if I like it.  Matt and I were talking about this
> yesterday and his suggestion was to, if they have an infinite loop, delete
> the entire shader and draw pink.  That's more useful to developers than
> hanging their GPU.

Hmm... I guess if you did that it would be wise to also print a
message somewhere the developer could see, since "uhh now the entire
thing is pink" probably won't help clarify what the problem is, even
if it shows that there is in fact a problem :)

But I guess Matt is right that the danger is more theoretical than
practical. So go ahead and add my r-b to both patches with or without
the suggestion. Fake edges begone!

>
>> On Thu, Sep 1, 2016 at 3:11 PM, Jason Ekstrand <jason at jlekstrand.net>
>> wrote:
>> > Signed-off-by: Jason Ekstrand <jason at jlekstrand.net>
>> > ---
>> >  src/compiler/nir/nir_opt_dead_cf.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/compiler/nir/nir_opt_dead_cf.c
>> > b/src/compiler/nir/nir_opt_dead_cf.c
>> > index 3551124..1490e68 100644
>> > --- a/src/compiler/nir/nir_opt_dead_cf.c
>> > +++ b/src/compiler/nir/nir_opt_dead_cf.c
>> > @@ -205,7 +205,8 @@ loop_is_dead(nir_loop *loop)
>> >     nir_metadata_require(impl, nir_metadata_live_ssa_defs |
>> >                                nir_metadata_dominance);
>> >
>> > -   for (nir_block *cur = after->imm_dom; cur != before; cur =
>> > cur->imm_dom) {
>> > +   for (nir_block *cur = after->imm_dom; cur && cur != before;
>> > +        cur = cur->imm_dom) {
>> >        nir_foreach_instr(instr, cur) {
>> >           if (!nir_foreach_ssa_def(instr, def_not_live_out, after))
>> >              return false;
>> > --
>> > 2.5.0.400.gff86faf
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev at lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>