[PATCH] D19257: [SimplifyCFG] Extend TryToSimplifyUncondBranchFromEmptyBlock for empty block including lifetime intrinsics

Hans Wennborg via llvm-commits llvm-commits at lists.llvm.org
Fri Apr 22 09:44:34 PDT 2016


hans added a comment.

I've put some comments in the code, but my main concern is that this patch needs a lot of test cases.


================
Comment at: include/llvm/IR/BasicBlock.h:159
@@ +158,3 @@
+  /// instruction
+  /// coupled with the following lifetime intrinsic.
+  Instruction *getFirstNonPHIOrDbgOrLifetimeOrBitCast();
----------------
nit: The formatting of this comment doesn't look right.

================
Comment at: lib/IR/BasicBlock.cpp:216
@@ +215,3 @@
+    if (auto *II = dyn_cast<IntrinsicInst>(&I))
+      if (II->getIntrinsicID() == Intrinsic::lifetime_end)
+        continue;
----------------
The getFirstNonPHIOrDbgOrLifetime() function also considers Intrinsic::lifetime_start. Should this function do that too?

================
Comment at: lib/Transforms/Utils/Local.cpp:799
@@ -798,1 +798,3 @@
 
+/// hasLifetimeEnd - Return true if BB has lifetime.end intrinsic.
+///
----------------
No need to repeat the name of the function in the comment. This applies to the other functions as well.

================
Comment at: lib/Transforms/Utils/Local.cpp:803
@@ +802,3 @@
+  BasicBlock::iterator I = BB->begin();
+  while (!I->isTerminator()) {
+    if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(I))
----------------
How about writing this with a range-based for-loop instead?

================
Comment at: lib/Transforms/Utils/Local.cpp:816
@@ +815,3 @@
+  BasicBlock::iterator I = BB->begin();
+  return isa<PHINode>(I) ? true : false;
+}
----------------
No need for the the ?: operator here.
In fact, how about just:

return isa<PHINode>(BB->begin());

And that means maybe we don't need a separate function for this? For example, on line 977 we already do the same kind of check with "if (isa<PHINode>(Succ->begin())) {"

================
Comment at: lib/Transforms/Utils/Local.cpp:825
@@ +824,3 @@
+  BasicBlock::iterator I = BB->begin();
+  BasicBlock::iterator NI = BB->begin();
+
----------------
No need to declare these before they're used.

================
Comment at: lib/Transforms/Utils/Local.cpp:830
@@ +829,3 @@
+  // we cannot hoist lifetime.end intrinsics because it would change semantics.
+  for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI) {
+    BasicBlock *Pred = *PI;
----------------
You could use a range-based for-loop over predecessors(BB) instead

================
Comment at: lib/Transforms/Utils/Local.cpp:834
@@ +833,3 @@
+    if (auto *BI = dyn_cast<BranchInst>(PTI)) {
+      if (BI->isUnconditional())
+        continue;
----------------
Would it be sufficient to just check "BI->getSingleSuccessor()"?

================
Comment at: lib/Transforms/Utils/Local.cpp:839
@@ +838,3 @@
+    } else
+      return false;
+  }
----------------
Some of these "else" and "return false" lines seem unnecessary.. how about something like:

  for (each predecessor P) {
    if (not P has single successor)
      return false;
  }

================
Comment at: lib/Transforms/Utils/Local.cpp:846
@@ +845,3 @@
+    if (isa<BitCastInst>(I)) {
+      for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI) {
+        BasicBlock *Pred = *PI;
----------------
Again, range-based for-loop would be nice.

================
Comment at: lib/Transforms/Utils/Local.cpp:849
@@ +848,3 @@
+        pred_iterator NPI = PI;
+        if (++NPI == E) {
+          Pred->getInstList().splice(Pred->getTerminator()->getIterator(),
----------------
I'm not sure this extra logic to splice instead of clone the instruction into the predecessor is worth it.

================
Comment at: lib/Transforms/Utils/Local.cpp:855
@@ +854,3 @@
+      }
+      I = BB->begin();
+      NI = BB->begin();
----------------
I suppose this is because we might have moved I into a predecessor? This makes the loop hard to follow. If you used cloned the instruction into the predecessor, there would be no need for this, right?

================
Comment at: lib/Transforms/Utils/Local.cpp:873
@@ +872,3 @@
+            if ((PEI != Pred->begin()) && isa<BitCastInst>(--PEI))
+              NPEI->setOperand(1, &*(PEI));
+          }
----------------
This all looks very complicated.

================
Comment at: lib/Transforms/Utils/Local.cpp:881
@@ +880,3 @@
+    } else
+      return false;
+  }
----------------
Didn't we check earlier the contents of the BB so we know it can be hoisted? In that case there shouldn't be a need for all this checking and "return false" statements. Can you just add asserts that the instructions are what we expect, and then clone them to the predecessors?

================
Comment at: lib/Transforms/Utils/Local.cpp:905
@@ +904,3 @@
+
+    for (pred_iterator PI = pred_begin(BB), E = pred_end(BB); PI != E; ++PI)
+      if (hasPHI(*PI))
----------------
You could use a range-based for-loop over predecessors(BB) instead, and successors(BB) in the loop below.

================
Comment at: lib/Transforms/Utils/Local.cpp:917
@@ +916,3 @@
+
+      // Copy over any phi, debug or lifetime instruction.
+      BB->getTerminator()->eraseFromParent();
----------------
Didn't we check above that BB has no phi instructions?


http://reviews.llvm.org/D19257





More information about the llvm-commits mailing list