Kenneth Graunke <kenn...@whitecape.org> writes: > On Friday, November 20, 2015 05:36:35 PM Francisco Jerez wrote: >> Kenneth Graunke <kenn...@whitecape.org> writes: >> >> > 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... >> >> Hmm, does it? AFAICT brw_find_next_block_end() will just give you the >> first WHILE instruction it finds even if the corresponding loop starts >> after the given offset. Not that it matters much anyway. > > Ahh. I thought you meant the code for patching WHILE's JIP was > similarly broken. But you're right, I think as is, > > > DO > CMP.f0.0 ... > (+f0.0) BREAK > > ... > > DO > ... > WHILE > > ... > WHILE > > will make the BREAK point to the first WHILE instead of the second. > The obvious fix would be to handle DO just like I handle IF. > Unfortunately, there is no DO instruction...and at this point, the > loop stack is gone. > > I suppose one fix would be to make brw_find_next_block_end() only > accept a WHILE as the end of the block if the WHILE's jump target > is before our starting offset. In other words, if the WHILE's > top-of-the-loop is after our BREAK/CONTINUE, it is obviously the > wrong loop. (This works because WHILE's jumps are set as soon as > we emit the WHILE, so they're already in place by brw_set_uip_jip.) > > I think that would work, but it would at least need a comment.
Yeah, that should work. Anyway if you don't feel like making the change as part of the same patch feel free to put my Reviewed-by: Francisco Jerez <curroje...@riseup.net> on this. > _______________________________________________ > mesa-stable mailing list > mesa-sta...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-stable
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
