Fix lifetime call in landingpad blocking Simplifycfg pass

[zequanwu]

Summary

Fix lifetime call in landingpad blocks simplifycfg from removing the landingpad.

[rnk]

Thanks for the patch!

In the future, please upload with full context (-U99999) as described here:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

3975

Can this logic be shared with removeEmptyCleanup? It is the analogous transform for WinEH. That way, if we later add new no-op intrinsics to skip (llvm.assume), both EH styles benefit.

[zequanwu]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

3975

removeEmptyCleanup has similar logic. Should I adopt the logic from removeEmptyCleanup?

[zequanwu]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

3975

I put the shared logic into the function isIntrinsics to check if it is one of the specified intrinsics.

[rnk]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

3966

Let's use a more descriptive name. A reader could interpret this to mean, "is this any intrinsic instruction?"

I would suggest isCleanupBlockEmpty, and see below for ideas on factoring out the while loop.

3992–3993

I think you can factor out this while loop as well, so that the function takes the iterators as a parameter, and then returns a boolean. The caller could look like:

if (!isCleanupBlockEmpty(LPInst, RI))
  return false;

[zequanwu]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

3992–3993

The example code you give takes two parameters with instruction type. So, I make the function to take two instructions instead of iterators.

[rnk]

lgtm! Thanks.

I will commit this and push it for you shortly.

[jdoerfert]

By @lebedev.ri via email:
This likely should ideally also look past new attribute bundles/ephemeral values?

I agree. We need to unify these things instead of matching them explicitly, asking for droppable or ephemeral values. The interface is probably needs to provide some choices, e.g., for debug information, but I think by default treating all intrinsics we can safely remove as droppable should get as almost there.

[rnk]

In D77188#1973342, @jdoerfert wrote:

By @lebedev.ri via email:
This likely should ideally also look past new attribute bundles/ephemeral values?

I agree. We need to unify these things instead of matching them explicitly, asking for droppable or ephemeral values. The interface is probably needs to provide some choices, e.g., for debug information, but I think by default treating all intrinsics we can safely remove as droppable should get as almost there.

Sounds good, but such a classification tool does not exist, and this was Zequan's first patch, so I didn't ask him to create this abstraction.

Building this concept (meta-instructions from codegen seem similar) isn't easy. It's not legal to move code past a lifetime intrinsic, for example. So it's hard to nail down the exact semantics of what we're looking for here.

[efriedma]

Lifetime intrinsics specifically pose their own problem, I think; you can't really just individually drop one. Although, the semantics are sort of fuzzy.

Maybe it would be worth amending LangRef to explicitly allow dropping lifetime.end in general. Not sure what that text would look like. (You wouldn't want to, usually, since it would likely block coloring, but here the payoff might be big enough.)

I have a rough idea for a proposal bouncing around in my head to try to replace lifetime intrinsics with something more powerful/well-defined for local variables, but probably I don't have time to actually implement it anytime soon. Basically, instead of the current lifetime.start/end, we would have intrinsics that look basically like malloc/free, except that the memory allocation is backed by an alloca. In that world, it would be a lot easier to determine what transforms are actually legal/reasonable. If anyone is interested, I could try to write something up more formally.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

bool SimplifyCFGOpt::simplifyCommonResume(ResumeInst *RI) {
 
   // Delete the resume block if all its predecessors have been removed.
   if (pred_empty(BB))
     BB->eraseFromParent();
 
   return !TrivialUnwindBlocks.empty();
 }
 
+// Check if it is one of the specified instrinsics
+static bool isInstrinsics(BasicBlock::iterator I) {
+  auto *II = dyn_cast<IntrinsicInst>(I);
+  if (!II)
+    return false;
+
+  Intrinsic::ID IntrinsicID = II->getIntrinsicID();
+  switch (IntrinsicID) {
+    case Intrinsic::dbg_declare:
+    case Intrinsic::dbg_value:
+    case Intrinsic::dbg_label:
+    case Intrinsic::lifetime_end:
+      break;
+    default:
+      return false;
+  }
+  return true;
+}
+
 // Simplify resume that is only used by a single (non-phi) landing pad.
 bool SimplifyCFGOpt::simplifySingleResume(ResumeInst *RI) {
   BasicBlock *BB = RI->getParent();
   auto *LPInst = cast<LandingPadInst>(BB->getFirstNonPHI());
   assert(RI->getValue() == LPInst &&
          "Resume must unwind the exception that caused control to here");
 
   // Check that there are no other instructions except for debug intrinsics.
   BasicBlock::iterator I = LPInst->getIterator(), E = RI->getIterator();
-  while (++I != E)
-    if (!isa<DbgInfoIntrinsic>(I))
+  while (++I != E) {
+    if (!isInstrinsics(I))
       return false;
+  }
 
   // Turn all invokes that unwind here into calls and delete the basic block.
   for (pred_iterator PI = pred_begin(BB), PE = pred_end(BB); PI != PE;) {
     BasicBlock *Pred = *PI++;
     removeUnwindEdge(Pred);
   }
 
   // The landingpad is now unreachable.  Zap it.
static bool removeEmptyCleanup(CleanupReturnInst *RI) {
   // We cannot kill the pad if it has multiple uses.  This typically arises
   // from unreachable basic blocks.
   if (!CPInst->hasOneUse())
     return false;
 
   // Check that there are no other instructions except for benign intrinsics.
   BasicBlock::iterator I = CPInst->getIterator(), E = RI->getIterator();
   while (++I != E) {
-    auto *II = dyn_cast<IntrinsicInst>(I);
-    if (!II)
+    if (!isInstrinsics(I))
       return false;
-
-    Intrinsic::ID IntrinsicID = II->getIntrinsicID();
-    switch (IntrinsicID) {
-    case Intrinsic::dbg_declare:
-    case Intrinsic::dbg_value:
-    case Intrinsic::dbg_label:
-    case Intrinsic::lifetime_end:
-      break;
-    default:
-      return false;
-    }
   }
 
   // If the cleanup return we are simplifying unwinds to the caller, this will
   // set UnwindDest to nullptr.
   BasicBlock *UnwindDest = RI->getUnwindDest();
   Instruction *DestEHPad = UnwindDest ? UnwindDest->getFirstNonPHI() : nullptr;
 
   // We're about to remove BB from the control flow.  Before we do, sink any

llvm/test/Transforms/SimplifyCFG/lifetime-landingpad.ll

+; RUN: opt < %s -simplifycfg -S | FileCheck %s
+
+; CHECK-LABEL: define void @foo
+define void @foo() personality i32 (...)* @__gxx_personality_v0 {
+entry:
+; CHECK: alloca i8
+; CHECK: call void @llvm.lifetime.start.p0i8
+; CHECK: call void @bar()
+; CHECK: call void @llvm.lifetime.end.p0i8
+; CHECK: ret void
+  %a = alloca i8
+  call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull %a) nounwind
+  invoke void @bar() to label %invoke.cont unwind label %lpad
+
+invoke.cont:
+  call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull %a) nounwind
+  ret void
+
+lpad:
+; CHECK-NOT: landingpad
+  %b = landingpad { i8*, i32 }
+          cleanup
+  call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull %a) nounwind
+  resume { i8*, i32 } %b
+}
+
+declare void @bar()
+
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture) nounwind
+
+declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture) nounwind
+
+declare i32 @__gxx_personality_v0(...)