[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:475
@@ +474,3 @@
+    // force some predecessors live (such as "br label %cond.false" here)
+    // to correctly model the control dependence.
+    
----------------
mehdi_amini wrote:
> Thanks for the extensive comment and the example, unfortunately this is not really the information I was looking for.
> Phi nodes are dependent on the control flow, and we need to mark predecessors live in general, I got that part, but what isn't just clear to me is *why* only *some* blocks are marked live.
> 
> For instance:
> 
> ```
> define i8 @foo(i1 %cond1, i1 %cond2, i8 %a, i8 %b, i8 %c, i8 %d) nounwind {
> entry:
>   br i1 %cond1, label %first_true, label %first_false
> first_true:
>   br i1 %cond2, label %second_true, label %second_false
> first_false:
>   br label %end
> second_true:
>   br label %end
> second_false:
>   br label %end
> end: 
>   %result = phi i8 [ %a, %first_false ], [ %b, %second_true ], [ %c, %second_false ] 
>   ret i8 %result
> }
> ```
> 
> Assuming none of the `br label %end` is live, and considering the only phi, as written, all predecessors will be marked live.
> If I replace the phi with ` phi i8 [ %a, %first_false ], [ %a, %second_true ], [ %c, %second_false ] `, then only `%second_false` is marked live.
> 
> Now, what I find annoying is that with the same edges but just changing the order of the operands, you would get a different results. For instance: `phi i8  [ %c, %second_false ], [ %a, %first_false ], [ %a, %second_true ]` now `%second_false` is *not* marked live and instead `%first_false` and `%second_true` are.
You are correct the it is sensitive to the order. Marking one is sufficient for correctness. I have considered marking the lexically first one as live which would provide more stability but it is a rare occurrence so it is not clear if it is worth fixing. I don't have any collected data on this.


https://reviews.llvm.org/D23824