[PATCH] D18762: Add Aggressive Control Dead Code Elimination

[David Majnemer via llvm-commits]

majnemer added a subscriber: majnemer.

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:321-322
@@ +320,4 @@
+    MPM.add(createAggressiveControlDCEPass());  // Delete dead instructions
+  }
+  else {
+    MPM.add(createAggressiveDCEPass());         // Delete dead instructions
----------------
Please format this appropriately.

================
Comment at: lib/Transforms/Scalar/ACDCE.cpp:508-513
@@ +507,8 @@
+bool AggressiveControlDCE::alwaysLive(Instruction &I) const {
+  if (I.isEHPad() || I.mayHaveSideEffects()) {
+    if (isInstrumentsConstant(I)) {
+      return false;
+    }
+    return true;
+  }
+  if (!isa<TerminatorInst>(I)) {
----------------
Why are EHPads always considered live?

================
Comment at: lib/Transforms/Scalar/ACDCE.cpp:609
@@ +608,3 @@
+
+  /* dead terminators which control live blocks become live */
+  for (auto BB : IDFBlocks) {
----------------
Please use // comments.

================
Comment at: lib/Transforms/Scalar/ACDCE.cpp:617
@@ +616,3 @@
+
+// there are scenarios in which a phi node is really short-hand
+// for a copy operations on the edge and the semantics of the program
----------------
Please start sentences with a capital letter.

================
Comment at: lib/Transforms/Scalar/ACDCE.cpp:617-623
@@ +616,9 @@
+
+// there are scenarios in which a phi node is really short-hand
+// for a copy operations on the edge and the semantics of the program
+// rely on this. We look for situations where there are distinct values from
+// blocks with dead terminators (which we assume
+// are all replaced with
+// a single edge) and if we find this case we mark some of those terminators
+// as live.
+void AggressiveControlDCE::checkPhiNodes() {
----------------
majnemer wrote:
> Please start sentences with a capital letter.
It would be helpful if you gave a short example of this in IR (as a comment).

================
Comment at: lib/Transforms/Scalar/ACDCE.cpp:620-621
@@ +619,4 @@
+// rely on this. We look for situations where there are distinct values from
+// blocks with dead terminators (which we assume
+// are all replaced with
+// a single edge) and if we find this case we mark some of those terminators
----------------
The formatting got weird here.

================
Comment at: lib/Transforms/Scalar/ACDCE.cpp:645-647
@@ +644,5 @@
+
+// Traverse the connected component associated with BBInfo where
+//    nodes whose RegionVisited field is > FirstVisit are considered 'visited'
+//    and we stop at tops of live blocks.
+void AggressiveControlDCE::visitRegion(BlockInfoType *BBInfo,
----------------
Some strange indentation made its way here.

================
Comment at: lib/Transforms/Scalar/ACDCE.cpp:731
@@ +730,3 @@
+  // Set LivePostDom fields for blocks which dead terminators
+  SmallVector<DomTreeNodeBase<BasicBlock> *, 16> deadPdoms;
+  for (auto &Info : BlockInfoVec) {
----------------
Please obey the naming convention.

================
Comment at: lib/Transforms/Scalar/ACDCE.cpp:818
@@ +817,3 @@
+
+  // update each phi based on the new incoming edges
+  for (auto it = BB->begin(); PHINode *PN = dyn_cast<PHINode>(it); ++it) {
----------------
Please start each sentence with a capital letter and end it with a period.

================
Comment at: lib/Transforms/Scalar/ACDCE.cpp:849
@@ +848,3 @@
+
+  // update edges
+  for (auto PredInfo : Info->LivePreds) {
----------------
Ditto.

================
Comment at: lib/Transforms/Scalar/ACDCE.cpp:877-878
@@ +876,4 @@
+  DEBUG_MSG("making unconditional " << BB->getName());
+  IRBuilder<> Builder(PredTerm);
+  auto NewTerm = Builder.CreateBr(Target);
+  // don't update InstInfo to protect pointers into it...
----------------
I wouldn't bother with an IRBuilder for a single instruction, `BranchInst::Create(Target, PredTerm)` would do.


http://reviews.llvm.org/D18762