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.
> 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.
> Either way, I think this change is obviously correct. This patch is
>
> Reviewed-by: Ian Romanick <ian.d.roman...@intel.com>
>
Thanks!
> > J.A.
> >
> >
> >
> > > > CC: Ian Romanick <ian.d.roman...@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);
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
