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. > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
