[Mesa-dev] [Mesa-stable] [PATCH] i965: Fix JIP to properly skip over unrelated control flow.

[Francisco Jerez]

Kenneth Graunke <kenneth at whitecape.org> writes:

> On Friday, November 20, 2015 05:36:35 PM Francisco Jerez wrote:
>> Kenneth Graunke <kenneth at whitecape.org> writes:
>> 
>> > On Friday, November 20, 2015 02:38:10 PM Francisco Jerez wrote:
>> >> Kenneth Graunke <kenneth at 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-stable at lists.freedesktop.org
>> >> >> Signed-off-by: Kenneth Graunke <kenneth at 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 <currojerez at riseup.net>

on this.

> _______________________________________________
> mesa-stable mailing list
> mesa-stable at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-stable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20151122/f9f44649/attachment.sig>