CodeGen: Fix invalid bitcast for lifetime.start/end

[yaxunl]

Summary

lifetime.start/end expects pointer argument in alloca address space.
However in C++ a temporary variable is in default address space.

This patch changes API CreateMemTemp and CreateTempAlloca to
get the original alloca instruction and pass it lifetime.start/end.

It only affects targets with non-zero alloca address space.

[rjmccall]

These functions must predate the addition of CreateLifetimeStart and CreateLifetimeEnd methods on IRBuilder; we should just be calling those.

[yaxunl]

Revised by John's comments.

[yaxunl]

In D45900#1074400, @rjmccall wrote:

These functions must predate the addition of CreateLifetimeStart and CreateLifetimeEnd methods on IRBuilder; we should just be calling those.

Sorry. I think I misunderstood your comments. I will revise again.

[yaxunl]

Call Builder.CreateLifetimeStart/End.

I cannot replace CodeGenFunction::EmitLifetimeStart with Builder.CreateLifetimeStart since we still need to check ShouldEmitLifetimeMarkers and do the address space cast.

[rjmccall]

Oh, I see, it's not that the lifetime intrinsics don't handle pointers in the alloca address space, it's that we might have already promoted them into DefaultAS.

Do the LLVM uses of lifetime intrinsics actually look through these address space casts? I'm wondering if we might need to change how we emit the intrinsics so that they're emitted directly on (bitcasts of) the underlying allocas.

[yaxunl]

In D45900#1083377, @rjmccall wrote:

Oh, I see, it's not that the lifetime intrinsics don't handle pointers in the alloca address space, it's that we might have already promoted them into DefaultAS.

Do the LLVM uses of lifetime intrinsics actually look through these address space casts? I'm wondering if we might need to change how we emit the intrinsics so that they're emitted directly on (bitcasts of) the underlying allocas.

Some passes do not look through address space casts. Although there is InferAddressSpace pass which can eliminate the redundant address space casts, still it is desirable not to emit redundant address space in Clang.

To avoid increasing complexity of alloca emitting API, I think we need a way to track the original alloca and the alloca casted to default address space. I can think of two ways:

1. add OriginalPointer member to Address, which is the originally emitted LLVM value for the variable. Whenever we pass the address of a variable we also pass the original LLVM value.

2. add a map to CodeGenFunction to map the casted alloca to the real alloca.

Any suggestion? Thanks.

[rjmccall]

In D45900#1093154, @yaxunl wrote:

In D45900#1083377, @rjmccall wrote:

Oh, I see, it's not that the lifetime intrinsics don't handle pointers in the alloca address space, it's that we might have already promoted them into DefaultAS.

Do the LLVM uses of lifetime intrinsics actually look through these address space casts? I'm wondering if we might need to change how we emit the intrinsics so that they're emitted directly on (bitcasts of) the underlying allocas.

Some passes do not look through address space casts. Although there is InferAddressSpace pass which can eliminate the redundant address space casts, still it is desirable not to emit redundant address space in Clang.

To avoid increasing complexity of alloca emitting API, I think we need a way to track the original alloca and the alloca casted to default address space. I can think of two ways:

1. add OriginalPointer member to Address, which is the originally emitted LLVM value for the variable. Whenever we pass the address of a variable we also pass the original LLVM value.

2. add a map to CodeGenFunction to map the casted alloca to the real alloca.

Any suggestion? Thanks.

Can we just call CreateLifetimeStart (and push the cleanup to call CreateLifetimeEnd) immediately after creating the alloca instead of waiting until later like we do now?

Modifying Address is not appropriate, and adding a map to CGF would be big waste.

[yaxunl]

In D45900#1093160, @rjmccall wrote:

In D45900#1093154, @yaxunl wrote:

In D45900#1083377, @rjmccall wrote:

Oh, I see, it's not that the lifetime intrinsics don't handle pointers in the alloca address space, it's that we might have already promoted them into DefaultAS.

Do the LLVM uses of lifetime intrinsics actually look through these address space casts? I'm wondering if we might need to change how we emit the intrinsics so that they're emitted directly on (bitcasts of) the underlying allocas.

Some passes do not look through address space casts. Although there is InferAddressSpace pass which can eliminate the redundant address space casts, still it is desirable not to emit redundant address space in Clang.

To avoid increasing complexity of alloca emitting API, I think we need a way to track the original alloca and the alloca casted to default address space. I can think of two ways:

1. add OriginalPointer member to Address, which is the originally emitted LLVM value for the variable. Whenever we pass the address of a variable we also pass the original LLVM value.

2. add a map to CodeGenFunction to map the casted alloca to the real alloca.

Any suggestion? Thanks.

Can we just call CreateLifetimeStart (and push the cleanup to call CreateLifetimeEnd) immediately after creating the alloca instead of waiting until later like we do now?

Modifying Address is not appropriate, and adding a map to CGF would be big waste.

Since CreateTempAlloca returns the casted alloca by default, whereas CreateLifetimeStart expects the original alloca. If we want to call CreateLifetimeStart with original alloca, we have to do it in CreateTempAlloca.

This incurs two issues:

1. we need an enum parameter to control how lifetime.start/end is emitted. There are cases that lifetime.start is not emitted, and different ways to push cleanups for lifetime.end, e.g. in CodeGenFunction::

EmitMaterializeTemporaryExpr

switch (M->getStorageDuration()) {
case SD_Automatic:
case SD_FullExpression:
  if (auto *Size = EmitLifetimeStart(
          CGM.getDataLayout().getTypeAllocSize(Object.getElementType()),
          Object.getPointer())) {
    if (M->getStorageDuration() == SD_Automatic)
      pushCleanupAfterFullExpr<CallLifetimeEnd>(NormalEHLifetimeMarker,
                                                Object, Size);
    else
      pushFullExprCleanup<CallLifetimeEnd>(NormalEHLifetimeMarker, Object,
                                           Size);
  }
  break;

2. There are situations that the pushed cleanup for lifetime.end is deactivated and emitted early, e.g. in AggExprEmitter::withReturnValueSlot

// If there's no dtor to run, the copy was the last use of our temporary.
// Since we're not guaranteed to be in an ExprWithCleanups, clean up
// eagerly.
CGF.DeactivateCleanupBlock(LifetimeEndBlock, LifetimeStartInst);
CGF.EmitLifetimeEnd(LifetimeSizePtr, RetAddr.getPointer());

In this case there is no good way to get the LifetimeStartInst and the original alloca inst.

Basically the emitting of lifetime.start/end is not uniform enough to be incorporated as part of CreateTempAlloca.

How about letting CreateTempAlloca have an optional pointer argument for returning the original alloca?

[rjmccall]

That would work. I think it would be reasonable for AutoVarEmission to store that pointer if it makes things easier.

[yaxunl]

Add optional argument to CreateMemTemp and CreateTempAlloca to get the original alloca and use it for lifetime intrinsic.

[rjmccall]

LGTM.

lib/CodeGen/CGDecl.cpp

 
 /// Emit a lifetime.begin marker if some criteria are satisfied.
 /// \return a pointer to the temporary size Value if a marker was emitted, null
 /// otherwise
 llvm::Value *CodeGenFunction::EmitLifetimeStart(uint64_t Size,
                                                 llvm::Value *Addr) {
   if (!ShouldEmitLifetimeMarkers)
     return nullptr;
-
-  llvm::Value *SizeV = llvm::ConstantInt::get(Int64Ty, Size);
-  Addr = Builder.CreateBitCast(Addr, AllocaInt8PtrTy);
-  llvm::CallInst *C =
-      Builder.CreateCall(CGM.getLLVMLifetimeStartFn(), {SizeV, Addr});
-  C->setDoesNotThrow();
+  Addr = EmitCastToVoidPtrInAllocaAddrSpace(Addr);
+  auto *SizeV = llvm::ConstantInt::get(Int64Ty, Size);
+  Builder.CreateLifetimeStart(Addr, SizeV);
   return SizeV;
 }
 
 void CodeGenFunction::EmitLifetimeEnd(llvm::Value *Size, llvm::Value *Addr) {
-  Addr = Builder.CreateBitCast(Addr, AllocaInt8PtrTy);
-  llvm::CallInst *C =
-      Builder.CreateCall(CGM.getLLVMLifetimeEndFn(), {Size, Addr});
-  C->setDoesNotThrow();
+  Addr = EmitCastToVoidPtrInAllocaAddrSpace(Addr);
+  Builder.CreateLifetimeEnd(Addr, llvm::cast<llvm::ConstantInt>(Size));
 }
 
 void CodeGenFunction::EmitAndRegisterVariableArrayDimensions(
     CGDebugInfo *DI, const VarDecl &D, bool EmitDebugInfo) {
   // For each dimension stores its QualType and corresponding
   // size-expression Value.
   SmallVector<CodeGenFunction::VlaSizePair, 4> Dimensions;
 

lib/CodeGen/CGExpr.cpp

llvm::Value *CodeGenFunction::EmitCastToVoidPtr(llvm::Value *value) {
   llvm::PointerType *destType = Int8PtrTy;
   if (addressSpace)
     destType = llvm::Type::getInt8PtrTy(getLLVMContext(), addressSpace);
 
   if (value->getType() == destType) return value;
   return Builder.CreateBitCast(value, destType);
 }
 
+llvm::Value *
+CodeGenFunction::EmitCastToVoidPtrInAllocaAddrSpace(llvm::Value *V) {
+  if (V->getType()->getPointerAddressSpace() !=
+      CGM.getDataLayout().getAllocaAddrSpace()) {
+    return getTargetHooks().performAddrSpaceCast(
+        *this, V, getASTAllocaAddressSpace(), LangAS::Default, AllocaInt8PtrTy,
+        /*non-null*/ true);
+  } else {
+    return Builder.CreateBitCast(V, AllocaInt8PtrTy);
+  }
+}
+
 /// CreateTempAlloca - This creates a alloca and inserts it into the entry
 /// block.
 Address CodeGenFunction::CreateTempAlloca(llvm::Type *Ty, CharUnits Align,
                                           const Twine &Name,
                                           llvm::Value *ArraySize,
                                           bool CastToDefaultAddrSpace) {
   auto Alloca = CreateTempAlloca(Ty, Name, ArraySize);
   Alloca->setAlignment(Align.getQuantity());

lib/CodeGen/CodeGenFunction.h

                                  AggValueSlot::DoesNotNeedGCBarriers,
                                  AggValueSlot::IsNotAliased,
                                  AggValueSlot::DoesNotOverlap);
   }
 
   /// Emit a cast to void* in the appropriate address space.
   llvm::Value *EmitCastToVoidPtr(llvm::Value *value);
 
+  /// Emit a cast to void* in alloca address space.
+  llvm::Value *EmitCastToVoidPtrInAllocaAddrSpace(llvm::Value *V);
+
   /// EvaluateExprAsBool - Perform the usual unary conversions on the specified
   /// expression and compare the result against zero, returning an Int1Ty value.
   llvm::Value *EvaluateExprAsBool(const Expr *E);
 
   /// EmitIgnoredExpr - Emit an expression in a context which ignores the result.
   void EmitIgnoredExpr(const Expr *E);
 
   /// EmitAnyExpr - Emit code to compute the specified expression which can have

test/CodeGenCXX/amdgcn_declspec_get.cpp

+// RUN: %clang_cc1 -triple amdgcn-amd-amdhsa -emit-llvm -O3 -fdeclspec \
+// RUN:     -disable-llvm-passes -o - %s | FileCheck %s
+
+int get_x();
+
+struct A {
+   __declspec(property(get = _get_x)) int x;
+   static int _get_x(void) {
+     return get_x();
+   };
+};
+
+extern const A a;
+
+// CHECK-LABEL: define void @_Z4testv()
+// CHECK:  %i = alloca i32, align 4, addrspace(5)
+// CHECK:  %[[ii:.*]] = addrspacecast i32 addrspace(5)* %i to i32*
+// CHECK:  %[[cast1:.*]] = addrspacecast i32* %[[ii]] to i8 addrspace(5)*
+// CHECK:  call void @llvm.lifetime.start.p5i8(i64 4, i8 addrspace(5)* %[[cast1]])
+// CHECK:  %call = call i32 @_ZN1A6_get_xEv()
+// CHECK:  store i32 %call, i32* %[[ii]]
+// CHECK:  %[[cast2:.*]] = addrspacecast i32* %[[ii]] to i8 addrspace(5)*
+// CHECK:  call void @llvm.lifetime.end.p5i8(i64 4, i8 addrspace(5)* %[[cast2]])
+void test()
+{
+  int i = a.x;
+}