On Friday, November 20, 2015 02:38:10 PM Francisco Jerez wrote: > Kenneth Graunke <kenn...@whitecape.org> writes: > > > On Thursday, November 19, 2015 02:05:44 PM Kenneth Graunke wrote: > >> We've apparently always been botching JIP for sequences such as: > >> > >> do > >> cmp.f0.0 ... > >> (+f0.0) break > >> ... > >> if > >> ... > >> else > >> ... > >> endif > >> ... > >> while > >> > >> Normally, UIP is supposed to point to the final destination of the jump, > >> while in nested control flow, JIP is supposed to point to the end of the > >> current nesting level. It essentially bounces out of the current nested > >> control flow, to an instruction that has a JIP which bounces out another > >> level, and so on. > >> > >> In the above example, when setting JIP for the BREAK, we call > >> brw_find_next_block_end(), which begins a search after the BREAK for the > >> next ENDIF, ELSE, WHILE, or HALT. It ignores the IF and finds the ELSE, > >> setting JIP there. > >> > >> This makes no sense at all. The break is supposed to skip over the > >> whole if/else/endif block entirely. They have a sibling relationship, > >> not a nesting relationship. > >> > >> This patch fixes brw_find_next_block_end() to track depth as it does > >> its search, and ignore anything not at depth 0. So when it sees the > >> IF, it ignores everything until after the ENDIF. That way, it finds > >> the end of the right block. > >> > >> Caught while debugging a tessellation shader - no apparent effect on > >> Piglit. I did look for actual applications that were affected, and > >> found that GLBenchmark Manhattan had a BREAK with a bogus JIP. > >> > >> Cc: mesa-sta...@lists.freedesktop.org > >> Signed-off-by: Kenneth Graunke <kenn...@whitecape.org> > > > > I tried pretty hard to produce a Piglit test that showed an actual > > problem from doing this wrong - and I wasn't able to. > > > > It seems it just steps through some extra instructions which do > > nothing, and is pretty harmless. > > > From my understanding of how control flow is implemented, jumping to the > ENDIF instruction of an inactive IF-ENDIF construct (or similarly to the > WHILE instruction of an inactive loop) is fully equivalent to jumping to > the same point of an active (i.e. properly nested) but non-diverging > IF-ENDIF construct, and will behave the same: It will have no effect on > the current per-channel enables (because the IP of the ENDIF instruction > won't match the UIP value present at the top of the stack), and for that > reason will go on and jump to the instruction pointed to by the JIP > value of the ENDIF, which will be another ENDIF/WHILE/HALT instruction > closer to the right ENDIF/WHILE instruction that closes the current > block. > > > So I don't think this should actually go to stable after all. > > > Yeah, seems pretty harmless -- If it weren't harmeless you'd also need > to apply a similar fix to WHILE loops, but I don't think you do.
Thanks, Curro! I appreciate you confirming the theory :) I haven't pushed the patch yet, so would you like to add a Reviewed-by? What WHILE fix are you thinking of? We may as well get it right, even if it is harmless. At a cursory glance, I didn't see anything wrong, as it uses the loop stack. But I might've missed something...
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
