[PATCH] D23824: [ADCE] Add handling of PHI nodes when removing control flow

[David Callahan via llvm-commits]

david2050 added inline comments.

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:463
@@ +462,3 @@
+      }
+      if (CommonReachngDefinition != Value) {
+        // When there are two definitions from "dead" predecessor paths we
----------------
dberlin wrote:
> dberlin wrote:
> > david2050 wrote:
> > > This step is necessary otherwise we delete a branch which determines a live value. It is an artifact of the semantics of PHI nodes. This is about preserving existing structure not translating to a new form. 
> > > 
> > > And Yes, outside of loops, the baseline code catches almost all cases. The primary motivation was dead, may-be-infinute loops.
> > I understand the semantics of phi nodes :)
> > 
> > I'm asking why you don't just mark them *all* live.
> > Traditionally, in DCE, what would happen is that if the phi is live, you mark all values that are incoming to the phi as live (and in a control dependence DCE, any control dependences of the necessary edges)
> > 
> > Here, it seems like you are trying to figure out if the phi is useless by seeing if all branches have the same incoming value, and if so, you are arbitrarily choosing one so you can delete at least one branch.
> > That seems .. pointless. 
> > 
> > It should only also matter in the case of either predecessors that only exist for control flow (IE empty basic blocks), or critical edges.
> > 
> > 
> > The only interesting loop case i'm aware of where this matters is:
> > if (b)
> > {
> > int i;
> > for (i = 0; i<1000; ++i);
> >  j = 0;
> > }
> > return j;
> > }
> > 
> > But your current code won't allow deleting the loop in this case, AFAICT.
> > (among other things, you would need to determine what the control dependence would look like if the critical edge was split without splitting it. This is what GCC does)
> >  
> > 
> > 
> > 
> > 
> (and to be super clear, when i say "I'm asking why you don't just mark them *all* live.", i mean "all the arguments and edges of a given phi node", not "all phis").
> 
It is not all branches, it is all branches from dead predecessors since in the case that we should remove all such predecessors to be replaced with a single edge, we verify that there is a unique value for that edge. If not we select one predecessor to keep live. Consider 
  
  a = 0
  if (x) { 
        if (y) a =1
        else  a = 2
   }
   else a  =3
   phi(...)
   
The phi node following this block has tree edges labeled with values 1,2,3 respectively.
The guarded basic blocks are empty because the assignments are folded into the phi node.
The branch decision at which tests 'y' is dead at this point because there are no live operations
in any block it controls.  
Here we determine that we need to keep at least one of the empty blocks around to distinguish between 1 and 2, which models the behavior if the assignments to 'a' were instead explicitly in the original blocks.

Given a more complex example we might have

  a = 0
  if (x) { 
        if (z)  
           a=1
        else { 
           if (y)  { }  
           else { } 
        }
   }
   else a  =3
   phi(...)

Here there are 4 predecessors to the join, three are 2 but only two distinct values and we only
need to have the branch on 'z' live. The branch on 'y' need not be. This is why the code does not mark 
all incoming edges live but does not do the extra work to try and pick and optimal choice (the path with a=1 in this case). The scenario is rare enough that it is not clear extra work is worth the effort.

In your example, I believe the current code will allow the remove the loop since the assignment to J is not control dependent on the loop but only on the branch testing 'b'. The subset of the CFG corresponding to the loop would be all marked dead and removed and the edge leaving the block holding the test on 'b' redirected to the block holding 'j=0'.  I don't understand your reference to splitting a critical edge.


https://reviews.llvm.org/D23824