[NewGVN] Fix lifetime coercion

[ManuelJBrito]

Before commit 14dee0a, NewGVN would not miscompile the function foo, because isMustAlias would return false for non-pointers, particularly for lifetime.start. We need to check whether the loaded pointer is defined bylifetime.start in order to safely simplify the load to uninitialized memory.

For getInitialValueOfAllocation, the behavior depends on the allocation function. Therefore, we take a conservative approach: we check whether it's a pointer type. If it is, then—based on the earlier check—we know that the allocation function defines the loaded pointer.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: None (ManuelJBrito)

Changes

Before commit 14dee0a, NewGVN would not miscompile the function foo, because isMustAlias would return false for non-pointers, particularly for lifetime.start. We need to check whether the loaded pointer is defined bylifetime.start in order to safely simplify the load to uninitialized memory.

For getInitialValueOfAllocation, the behavior depends on the allocation function. Therefore, we take a conservative approach: we check whether it's a pointer type. If it is, then—based on the earlier check—we know that the allocation function defines the loaded pointer.

---

Full diff: https://github.com/llvm/llvm-project/pull/141477.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/NewGVN.cpp (+11-3)
• (modified) llvm/test/Transforms/NewGVN/coercion-different-ptr.ll (+2-1)

diff --git a/llvm/lib/Transforms/Scalar/NewGVN.cpp b/llvm/lib/Transforms/Scalar/NewGVN.cpp
index 3eb118908959f..8f53b511ba2d2 100644
--- a/llvm/lib/Transforms/Scalar/NewGVN.cpp
+++ b/llvm/lib/Transforms/Scalar/NewGVN.cpp
@@ -1531,6 +1531,8 @@ NewGVN::performSymbolicLoadCoercion(Type *LoadType, Value *LoadPtr,
 
   // All of the below are only true if the loaded pointer is produced
   // by the dependent instruction.
+  // The following only checks the cases where DepInst returns a pointer.
+  // For lifetime begin and similar instructions we need to check the operand.
   if (LoadPtr != lookupOperandLeader(DepInst) &&
       DepInst->getType()->isPointerTy() && !AA->isMustAlias(LoadPtr, DepInst))
     return nullptr;
@@ -1543,11 +1545,17 @@ NewGVN::performSymbolicLoadCoercion(Type *LoadType, Value *LoadPtr,
   }
   // If this load occurs either right after a lifetime begin,
   // then the loaded value is undefined.
+  // Make sure the loaded pointer is defined by the lifetime begin.
   else if (auto *II = dyn_cast<IntrinsicInst>(DepInst)) {
-    if (II->getIntrinsicID() == Intrinsic::lifetime_start)
+    auto *LifetimePtr = II->getOperand(1);
+    if (II->getIntrinsicID() == Intrinsic::lifetime_start &&
+        (LoadPtr == lookupOperandLeader(LifetimePtr) ||
+         AA->isMustAlias(LoadPtr, LifetimePtr)))
       return createConstantExpression(UndefValue::get(LoadType));
-  } else if (auto *InitVal =
-                 getInitialValueOfAllocation(DepInst, TLI, LoadType))
+  } else if (DepInst->getType()->isPointerTy())
+    // Only use the allocation value if we known for sure that the loaded ptr is
+    // defined by the allocation function.
+    if (auto *InitVal = getInitialValueOfAllocation(DepInst, TLI, LoadType))
       return createConstantExpression(InitVal);
 
   return nullptr;
diff --git a/llvm/test/Transforms/NewGVN/coercion-different-ptr.ll b/llvm/test/Transforms/NewGVN/coercion-different-ptr.ll
index 61a6a633788e1..847f0868cfa61 100644
--- a/llvm/test/Transforms/NewGVN/coercion-different-ptr.ll
+++ b/llvm/test/Transforms/NewGVN/coercion-different-ptr.ll
@@ -13,7 +13,8 @@ define void @foo(ptr %arg) {
 ; CHECK-NEXT:    [[ALLOCA:%.*]] = alloca i8, align 16
 ; CHECK-NEXT:    call void @llvm.lifetime.start.p0(i64 1, ptr [[ALLOCA]])
 ; CHECK-NEXT:    [[LOAD:%.*]] = load ptr, ptr [[ARG]], align 8
-; CHECK-NEXT:    [[CALL:%.*]] = call ptr undef(ptr [[ALLOCA]])
+; CHECK-NEXT:    [[LOAD1:%.*]] = load ptr, ptr [[LOAD]], align 8
+; CHECK-NEXT:    [[CALL:%.*]] = call ptr [[LOAD1]](ptr [[ALLOCA]])
 ; CHECK-NEXT:    ret void
 ;
 bb:

[nikic]

I think it would be a lot cleaner if we first handled the lifetime intrinsic case separately (because it does not return a pointer) and then the alloc/allocation cases (which do return a pointer). Mixing them makes it very non-obvious whether the interaction between all the conditions is correct.

[ManuelJBrito]

I think it would be a lot cleaner if we first handled the lifetime intrinsic case separately (because it does not return a pointer) and then the alloc/allocation cases (which do return a pointer). Mixing them makes it very non-obvious whether the interaction between all the conditions is correct.

Thanks, that makes sense. I have updated it.

[nikic]

LGTM