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

[Daniel Berlin via llvm-commits]

dberlin 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:
> 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").



https://reviews.llvm.org/D23824