[Mesa-dev] [PATCH] nir: move ALU instruction before the jump instruction

[Ian Romanick]

On 2/13/19 7:53 AM, Juan A. Suarez Romero wrote:
> On Tue, 2019-02-12 at 16:22 -0800, Ian Romanick wrote:
>> On 2/12/19 12:58 AM, Juan A. Suarez Romero wrote:
>>> opt_split_alu_of_phi moves ALU instruction to the end of continue block.
>>>
>>> But if the continue block ends with a jump instruction (an explicit
>>> "continue" instruction) then the ALU must be inserted before the jump,
>>> as it is illegal to add instructions after the jump.
>>
>> I'm assuming you found this by inspection?  Since this pass only
>> operates when the first block of the loop only has two predecessors (the
>> block before the loop and the implicit continue at the end of the loop),
>> this shouldn't be a a problem in practice... or were you able to trigger
>> it somehow?
> 
> Found when dealing with the SPIR-V code that I've sent in 
> https://lists.freedesktop.org/archives/mesa-dev/2019-February/214906.html
> 
> 
> The obtained NIR code has an explicit continue at the end of the loop (see 
> http://paste.debian.net/1067619/, in particular the loop with header block_2).

That loop is a mess.  Wow... I'm impressed. :) I see a continue inside
an else-clause at line 107 and a break at line 112.  I now understand
the fundamental problem.  Am I correct that this problem would also
occur before 8fb8ebfbb05?  And that's what "nir: allow stitching of
non-empty block" fixes?

Did nir_validate trip on this?  If not, it seems like it should...
though that might cause problems when nir_validate is run immediately
following translation into NIR.

It seems like we could craft a couple *simple* piglit tests that
exercise this.  I don't want to rely on a giant, ugly test case as our
only coverage for this issue.  I'm sure debugging this was not fun, and
I don't want someone to have to go through that again should a similar
issue creep back in.  I can take a stab at that if you don't already
have something ready.

Either way, I think this change is obviously correct.  This patch is

Reviewed-by: Ian Romanick <ian.d.romanick at intel.com>

> 	J.A.
> 
> 
> 
>>> CC: Ian Romanick <ian.d.romanick at intel.com>
>>> Fixes: 0881e90c099 ("nir: Split ALU instructions in loops that read phis")
>>> ---
>>>  src/compiler/nir/nir_opt_if.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/compiler/nir/nir_opt_if.c b/src/compiler/nir/nir_opt_if.c
>>> index 9afb901be14..932af9e37ab 100644
>>> --- a/src/compiler/nir/nir_opt_if.c
>>> +++ b/src/compiler/nir/nir_opt_if.c
>>> @@ -488,7 +488,7 @@ opt_split_alu_of_phi(nir_builder *b, nir_loop *loop)
>>>            *
>>>            * Insert the new instruction at the end of the continue block.
>>>            */
>>> -         b->cursor = nir_after_block(continue_block);
>>> +         b->cursor = nir_after_block_before_jump(continue_block);
>>>  
>>>           nir_ssa_def *const alu_copy =
>>>              clone_alu_and_replace_src_defs(b, alu, continue_srcs);