[Mesa-stable] [Mesa-dev] [PATCH 1/5] nir/cf: Remove phi sources if needed in nir_handle_add_jump

[Iago Toral]

On Tue, 2018-10-02 at 07:50 -0500, Jason Ekstrand wrote:
> On Tue, Oct 2, 2018 at 7:30 AM Jason Ekstrand <jason at jlekstrand.net>
> wrote:
> > On Tue, Oct 2, 2018 at 5:53 AM Iago Toral <itoral at igalia.com>
> > wrote:
> > > On Sat, 2018-09-22 at 16:39 -0500, Jason Ekstrand wrote:
> > > 
> > > > If the block in which the jump is inserted is the predecessor
> > > of a
> > > 
> > > > phi
> > > 
> > > > then we need to remove phi sources otherwise the phi may end up
> > > with
> > > 
> > > > things improperly connected.  Found by running the Vulkan CTS
> > > with
> > > 
> > > > SPIR-V optimizations enabled.
> > > 
> > > > 
> > > 
> > > > Cc: mesa-stable at lists.freedesktop.org
> > > 
> > > > ---
> > > 
> > > >  src/compiler/nir/nir_control_flow.c | 36 +++++++++++++++----
> > > ------
> > > 
> > > > ----
> > > 
> > > >  1 file changed, 19 insertions(+), 17 deletions(-)
> > > 
> > > > 
> > > 
> > > > diff --git a/src/compiler/nir/nir_control_flow.c
> > > 
> > > > b/src/compiler/nir/nir_control_flow.c
> > > 
> > > > index 3b0a0f1a5b0..a82f35550b8 100644
> > > 
> > > > --- a/src/compiler/nir/nir_control_flow.c
> > > 
> > > > +++ b/src/compiler/nir/nir_control_flow.c
> > > 
> > > > @@ -437,6 +437,23 @@ nearest_loop(nir_cf_node *node)
> > > 
> > > >     return nir_cf_node_as_loop(node);
> > > 
> > > >  }
> > > 
> > > >  
> > > 
> > > > +static void
> > > 
> > > > +remove_phi_src(nir_block *block, nir_block *pred)
> > > 
> > > > +{
> > > 
> > > > +   nir_foreach_instr(instr, block) {
> > > 
> > > > +      if (instr->type != nir_instr_type_phi)
> > > 
> > > > +         break;
> > > 
> > > > +
> > > 
> > > > +      nir_phi_instr *phi = nir_instr_as_phi(instr);
> > > 
> > > > +      nir_foreach_phi_src_safe(src, phi) {
> > > 
> > > > +         if (src->pred == pred) {
> > > 
> > > > +            list_del(&src->src.use_link);
> > > 
> > > > +            exec_node_remove(&src->node);
> > > 
> > > > +         }
> > > 
> > > > +      }
> > > 
> > > > +   }
> > > 
> > > > +}
> > > 
> > > > +
> > > 
> > > >  /*
> > > 
> > > >   * update the CFG after a jump instruction has been added to
> > > the end
> > > 
> > > > of a block
> > > 
> > > >   */
> > > 
> > > > @@ -447,6 +464,8 @@ nir_handle_add_jump(nir_block *block)
> > > 
> > > >     nir_instr *instr = nir_block_last_instr(block);
> > > 
> > > >     nir_jump_instr *jump_instr = nir_instr_as_jump(instr);
> > > 
> > > >  
> > > 
> > > > +   if (block->successors[0])
> > > 
> > > > +      remove_phi_src(block->successors[0], block);
> > > 
> > > 
> > > 
> > > Don't we need to do the same for block->successors[1]?
> > 
> > I was going to say no because his function handles *adding* a phi
> > and so the block should already only have one successor.  However,
> > I suppose you could add a phi right before an if.  I'll add the one
> > for block->successors[1] just to be safe.
> 
> On further thought, I don't think it's possible to end up with phi
> sources at block->successors[1].  The only type of block that can
> have multiple successors is one right before an if and both sides of
> the if have only one predecessor so they can't have phis.  Unless, of
> course, we add a bunch of no-op phis for some reason.  Eh, removing
> phis on block->successors[1] is harmless and probably more correct. 
> Still, it's a very weird case...

Yeah, that makes sense.
Iago
> --Jason
> 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20181003/24a87603/attachment.html>