[InstCombine] Remove trivially empty lifetime start/end ranges.

[aadg]

Summary

Some passes may open up opportunities for optimizations, leaving empty
lifetime start/end ranges. For example, with the following code:

void foo(char *, char *);
void bar(int Size, bool flag) {
  for (int i = 0; i < Size; ++i) {
    char text[1];
    char buff[1];
    if (flag)
      foo(text, buff); // BBFoo
  }
}

the loop unswitch pass will create 2 versions of the loop, one with
flag==true, and the other one with flag==false, but always leaving
the BBFoo basic block, with lifetime ranges covering the scope of the for
loop. Simplify CFG will then remove BBFoo in the case where flag==false,
but will leave the lifetime markers.

This patch teaches InstCombine to remove trivially empty lifetime marker
ranges, that is ranges ending right after they were started (ignoring
debug info in the range).

This fixes PR24598: excessive compile time after r234581.

[manmanren]

LGTM otherwise

test/Transforms/InstCombine/lifetime.ll

2

It is probably better to just check the 6 occurrences with a single RUN command.

[chandlerc]

LGTM with the fixes suggested inline.

I wonder if we should do something more aggressive to kill pointless lifetime ranges though. This handles the trivial case but non-trivial cases might be very interesting.

For example, even non-strictly-nested ranges won't be caught by this.

lib/Transforms/InstCombine/InstCombineCalls.cpp

1398

These should be loop variables.

test/Transforms/InstCombine/lifetime.ll

2

This already is checked by the FileCheck line. I think this can just be deleted.

[aadg]

I intended to catch another trivially empty ranges in a follow-up patch: the non strictly nested ones you refer to. The goal was to quickly fix an issue (PR24598), then improve the lifetime markers cleanup.

I think we should teach this cleanup to address less trivial cases, especially the one where the start dominates the end (garanteed by design) and the end post-dominate the start and the alloca is not used in between. I think this would catch lots of cases. But that's for another patch.

And the loop unroller probably needs to learn how not to mess to badly with the lifetime markers :).

Thanks for the reviews !

I'll fix and push this patch.

lib/Transforms/InstCombine/InstCombineCalls.cpp

1398

I agree on the principle, but BI needs to be incremented before entering the loop.

test/Transforms/InstCombine/lifetime.ll

2

Those 2 RUN lines were a very clumsy way of making sure the "if" basic block has been emptied, and the "else" was not modified. One RUN line is enough indeed, and I will add 2 checks:

CHECK: if:
CHECK-NEXT: br label %fin

lib/Transforms/InstCombine/InstCombineCalls.cpp

case Intrinsic::stackrestore: {
 
     // If the stack restore is in a return, resume, or unwind block and if there
     // are no allocas or calls between the restore and the return, nuke the
     // restore.
     if (!CannotRemove && (isa<ReturnInst>(TI) || isa<ResumeInst>(TI)))
       return EraseInstFromFunction(CI);
     break;
   }
+  case Intrinsic::lifetime_start: {
+    // Remove trivially empty lifetime_start/end ranges, i.e. a start
+    // immediately followed by an end (ignoring debuginfo in between).
+    BasicBlock::iterator BI = II, BE = II->getParent()->end();
+    for (++BI; BI != BE; ++BI) {
+      if (isa<DbgInfoIntrinsic>(BI))
+        continue;
+      if (IntrinsicInst *LTE = dyn_cast<IntrinsicInst>(BI))
+        if (LTE->getIntrinsicID() == Intrinsic::lifetime_end &&
+            II->getOperand(0) == LTE->getOperand(0) &&
+            II->getOperand(1) == LTE->getOperand(1)) {
+          EraseInstFromFunction(*LTE);
+          return EraseInstFromFunction(*II);
+        }
+      break;
+    }
+    break;
+  }
   case Intrinsic::assume: {
     // Canonicalize assume(a && b) -> assume(a); assume(b);
     // Note: New assumption intrinsics created here are registered by
     // the InstCombineIRInserter object.
     Value *IIOperand = II->getArgOperand(0), *A, *B,
           *AssumeIntrinsic = II->getCalledValue();
     if (match(IIOperand, m_And(m_Value(A), m_Value(B)))) {
       Builder->CreateCall(AssumeIntrinsic, A, II->getName());

test/Transforms/InstCombine/lifetime.ll

+; RUN: opt < %s -instcombine -S | FileCheck %s
+; RUN: opt < %s -instcombine -S | grep lifetime | count 6
+
+declare void @llvm.lifetime.start(i64, i8* nocapture)
+declare void @llvm.lifetime.end(i64, i8* nocapture)
+declare void @bar(i8* nocapture, i8* nocapture)
+
+define void @foo(i1 %flag) {
+entry:
+; CHECK-LABEL: @foo(
+; CHECK: %[[T:[^ ]+]] = getelementptr inbounds [1 x i8], [1 x i8]* %t
+; CHECK: %[[B:[^ ]+]] = getelementptr inbounds [1 x i8], [1 x i8]* %b
+; CHECK: call void @llvm.lifetime.start(i64 1, i8* %[[T]])
+; CHECK-NEXT: call void @llvm.lifetime.start(i64 1, i8* %[[B]])
+; CHECK-NEXT: call void @bar(i8* %[[B]], i8* %[[T]])
+; CHECK-NEXT: call void @llvm.lifetime.end(i64 1, i8* %[[B]])
+; CHECK-NEXT: call void @llvm.lifetime.end(i64 1, i8* %[[T]])
+  %t = alloca [1 x i8], align 1
+  %b = alloca [1 x i8], align 1
+  %0 = getelementptr inbounds [1 x i8], [1 x i8]* %t, i64 0, i64 0
+  %1 = getelementptr inbounds [1 x i8], [1 x i8]* %b, i64 0, i64 0
+  br i1 %flag, label %if, label %else
+
+if:
+  call void @llvm.lifetime.start(i64 1, i8* %0)
+  call void @llvm.lifetime.start(i64 1, i8* %1)
+  call void @llvm.lifetime.end(i64 1, i8* %1)
+  call void @llvm.lifetime.end(i64 1, i8* %0)
+  br label %fin
+
+else:
+  call void @llvm.lifetime.start(i64 1, i8* %0)
+  call void @llvm.lifetime.start(i64 1, i8* %1)
+  call void @bar(i8* %1, i8* %0)
+  call void @llvm.lifetime.end(i64 1, i8* %1)
+  call void @llvm.lifetime.end(i64 1, i8* %0)
+  br  label %fin
+
+fin:
+  ret void
+}
+
+