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

[Ian Romanick]

On 2/13/19 9:59 AM, Juan A. Suarez Romero wrote:
> On Wed, 2019-02-13 at 09:16 -0800, Ian Romanick wrote:
>> 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?
> 
> Yeah :) It's a SPIR-V code generated with some fuzzy tool.
> 
> 
> And yes, even with this patch applied, it will break later when stitching the
> code. 
> 
> Fortunately, I got rid of the "nir: allow stitching of non-empty block" patch
> and sent instead another one that removes one of the jumps to avoid the issue.
> 
>> 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.
> 
> The NIR code seems valid. The error (assert) was caught by nir_instr_insert,
> when applying the optimization.

Having instructions in a block after an unconditional jump seems bogus.
 It's technically valid, but it can never be correct.  I'm pretty sure
we scrub that from the GLSL frontend, and I'm not sure it's legal in
SPIR-V.  I'll look into it more.

>> 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.
>>
> 
> I didn't write a piglit test for this, as this test will end up in the Vulkan
> CTS.

Right.  We've always had a preference for simpler tests that poke at one
specific thing.  I'll come up with a couple tests, and I'll CC you on them.