SimplifyCFG: Avoid miscompilations due to removed lifetime intrinsics

[dotdash]

Summary

For example in a loop, just removing a lifetime.end intrinsic can
interact badly with loop rotation, which might move the corresponding
lifetime.start intrinsic to the end of the loop.

So with the lifetime.end intrinsic removed you can end up with something
like:

block:
    store i8 %foo, i8* %bar
    call void @llvm.lifetime.start(i64 1, i8* %bar)

Without a corresponding lifetime.end, meaning that the store is invalid.
If the lifetime.end intrinsic is kept, we get this instead:

block:
    store i8 %foo, i8* %bar
    call void @llvm.lifetime.end(i64 1, i8* %bar)
    call void @llvm.lifetime.start(i64 1, i8* %bar)

Which is fine, since the store is within a valid lifetime region.

[rnk]

I guess this is OK for now. We really need to revisit the design of lifetime.end and start. We might want to invent something much more limited to describe the scope of stack objects, instead of having this general thing intended to describe heap allocations as well.

[• rafael]

This is fine by me.

I agree with Reid that we need to figure out what to do with the lifetime intrinsics, but lets not miscompile code while we do that.

[dotdash]

I should note that I do not have commit access and need somebody to commit this for me.

Thanks!

[• rafael]

214336

lib/Transforms/Utils/SimplifyCFG.cpp

 
 bool SimplifyCFGOpt::SimplifyUncondBranch(BranchInst *BI, IRBuilder<> &Builder){
   BasicBlock *BB = BI->getParent();
 
   if (SinkCommon && SinkThenElseCodeToEnd(BI))
     return true;
 
   // If the Terminator is the only non-phi instruction, simplify the block.
-  BasicBlock::iterator I = BB->getFirstNonPHIOrDbgOrLifetime();
+  BasicBlock::iterator I = BB->getFirstNonPHIOrDbg();
   if (I->isTerminator() && BB != &BB->getParent()->getEntryBlock() &&
       TryToSimplifyUncondBranchFromEmptyBlock(BB))
     return true;
 
   // If the only instruction in the block is a seteq/setne comparison
   // against a constant, try to simplify the block.
   if (ICmpInst *ICI = dyn_cast<ICmpInst>(I))
     if (ICI->isEquality() && isa<ConstantInt>(ICI->getOperand(1))) {

test/Transforms/SimplifyCFG/lifetime.ll

 ; RUN: opt < %s -simplifycfg -S | FileCheck %s
 
-; Test that a lifetime intrinsic doesn't prevent us from simplifying this.
+; Test that a lifetime intrinsic isn't removed because that would change semantics
 
 ; CHECK: foo
 ; CHECK: entry:
-; CHECK-NOT: bb0:
-; CHECK-NOT: bb1:
+; CHECK: bb0:
+; CHECK: bb1:
 ; CHECK: ret
 define void @foo(i1 %x) {
 entry:
   %a = alloca i8
   call void @llvm.lifetime.start(i64 -1, i8* %a) nounwind
   br i1 %x, label %bb0, label %bb1
 
 bb0: