[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 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.

>
> _______________________________________________
> 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/20151120/e2be6686/attachment.sig>