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

[Hans Wennborg via llvm-commits]

hans added a comment.

Thanks! This is looking much better :-)

Some comments below.


================
Comment at: lib/IR/BasicBlock.cpp:211
@@ +210,3 @@
+  for (Instruction &I : *this) {
+    BasicBlock::iterator TI = I.getIterator();
+    if (isa<PHINode>(I) || isa<DbgInfoIntrinsic>(I))
----------------
No need to declare this until its use, further below.

But I also think maybe we can do without it; see below.

================
Comment at: lib/IR/BasicBlock.cpp:221
@@ +220,3 @@
+    if (auto *II = dyn_cast<BitCastInst>(&I))
+      if (auto *TII = dyn_cast<IntrinsicInst>(++TI))
+        if (TII->getIntrinsicID() == Intrinsic::lifetime_start ||
----------------
I think we can do away with the TI variable by just doing something like:

    if (auto *BCI = dyn_cast<BitCastInst>(&I)) {
      if (auto *II = dyn_cast<IntrinsicInst>(++I.getIterator())) {
        if ((II->getIntrinsicID() == Intrinsic::lifetime_start ||
             II->getIntrinsicID() == Intrinsic::lifetime_end) &&
            II->getOperand(1) == BCI) {
          continue;
        }
      }
    }


================
Comment at: lib/Transforms/Utils/Local.cpp:799
@@ -798,1 +798,3 @@
 
+/// hasLifetimeEnd - Return true if BB has lifetime.end intrinsic.
+///
----------------
Still no need to repeat the name in the comment. Just write this as

  /// Return true if BB has lifetime.end intrinsic.

This applies to the functions you're declaring below as well.

I know older functions do repeat the name in their comment, but it's unnecessary and we should stop doing that.

================
Comment at: lib/Transforms/Utils/Local.cpp:802
@@ +801,3 @@
+static bool hasLifetime(BasicBlock *BB) {
+  for (auto &I : *BB)
+    if (const IntrinsicInst *II = dyn_cast<IntrinsicInst>(&I))
----------------
I would suggest putting braces around the for-loop body. Even though it's technically one statement, it's several lines long so braces will help make it easier to read.

================
Comment at: lib/Transforms/Utils/Local.cpp:826
@@ +825,3 @@
+      for (auto Pred : predecessors(BB)) {
+        I.getIterator()->clone()->insertBefore(Pred->getTerminator());
+      }
----------------
clone() is a member of Instruction, so there's no need to go via the iterator here. Just

  I.clone()->insertBefore(Pred->getTerminator());

should do it.

================
Comment at: lib/Transforms/Utils/Local.cpp:835
@@ +834,3 @@
+          --PI;
+          --PI;
+          BasicBlock::iterator NPI = PI;
----------------
I'm not super happy with this code..

First, there's no need to iterate backwards to look for the IntrinsicInst we just inserted. We could do something like:

  Instruction *NewII = I->clone();
  NewII->insertBefore(pred->getTerminator());

and then use NewII below.

Also, even if the instruction before NewII is a BitCast, can we be sure it's *the right* BitCast, i.e. the one we cloned earlier? I wish we could do this in a more principled way..

I think it would be better if we could clone the bitcast and intrinsic together. How about we ignore the "if (isa<BitCastInst>)" case above, and then do something like this here:

  for (auto Pred : predecessors(BB)) {
    Instruction *NewII = I->clone();
    NewII->insertBefore(Pred->getTerminator());

    if (I != BB->begin()) {
      if (auto BC = dyn_cast<BitCastInst>(--I->getIterator())) {
        assert(BC == I->getOperand(1));
        auto NewBC = BC->clone();
        NewBC->insertBefore(NewII);
        NewII->setOperand(1, NewBC);
      }
    }
  }

================
Comment at: test/Transforms/SimplifyCFG/lifetime.ll:31
@@ +30,3 @@
+
+; Test that empty block including lifetime intrinsic can be removed.
+; Lifetime.end intrinsic is moved to predecessors because successor has
----------------
This is great, thanks!

It would be good to have a few tests that cover the situations when the block *cannot* be removed as well.


http://reviews.llvm.org/D19257