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

[Kenneth Graunke]

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...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-stable/attachments/20151120/746e451f/attachment.sig>