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

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:116
@@ +115,3 @@
+  /// A PHI node records values associated with different paths in the
+  /// control flow graph as if a logical copy poeration was performed from
+  /// an input value to the named value of the PHI node. Here we identify
----------------
s/poeration/operation

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:121
@@ +120,3 @@
+  /// effectively live so that hte control which determines the final values
+  /// is preserved.
+  void markLivePhiNodeInputs();
----------------
s/hte/the


================
Comment at: lib/Transforms/Scalar/ADCE.cpp:474
@@ +473,3 @@
+    // We detected the multiple distinct values from dead predecessors and
+    // force some predecessors live (such as "br label %cond.false" here)
+    // to correctly model the control dependence.
----------------
There is no `br label %cond.false` here.

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

================
Comment at: lib/Transforms/Scalar/ADCE.cpp:477
@@ +476,3 @@
+    
+    Value *CommonReachngDefintion = nullptr;
+    auto NumIdx = PN->getNumIncomingValues();
----------------
s/CommonReachngDefintion/CommonReachingDefinition/


https://reviews.llvm.org/D23824