[Mesa-dev] [PATCH] nir: move ALU instruction before the jump instruction
Juan A. Suarez Romero
jasuarez at igalia.com
Thu Feb 14 07:25:16 UTC 2019
On Wed, 2019-02-13 at 11:53 -0800, Ian Romanick wrote:
> 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.
>
Not sure if incorrect. In fact while working on this I thought if it would make
sense just to allow momentaneously to accept instructions after jumps, if that
simplifies the optimization algorithm, and run later a DCE pass, or similar, to
eliminate those instructions that are after the jump and that will not be never
reached and executed.
Example is for the stitch case later: we need to check for this situation and
remove the instruction ourselves before calling stitch to avoid inserting after
jump.
> > > 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.
>
More information about the mesa-dev
mailing list