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

[Jason Ekstrand]

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...

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-stable/attachments/20181002/54c56ea9/attachment.html>