Call CreateTempAllocaWithoutCast for ActiveFlag

[yaxunl]

Summary

Introduced CreateMemTempWithoutCast and CreateTemporaryAllocaWithoutCast to emit alloca
without casting to default addr space.

ActiveFlag is a temporary variable emitted for clean up. It is defined as AllocaInst* type and there is
a cast to AlllocaInst in SetActiveFlag. An alloca casted to generic pointer causes assertion in
SetActiveFlag.

Since there is only load/store of ActiveFlag, it is safe to use the original alloca, therefore use
CreateMemTempWithoutCast is called.

[rjmccall]

Maybe there should just be a method that makes a primitive alloca without the casting, and then you can call that in CreateTempAlloca.

[yaxunl]

In D47099#1105493, @rjmccall wrote:

Maybe there should just be a method that makes a primitive alloca without the casting, and then you can call that in CreateTempAlloca.

In many cases we still need to call CreateTempAlloca with cast enabled, since we are not certain there is only load from it and store to it. Any time it is stored to another memory location or passed to another function (e.g. constructor/destructor), it needs to be a pointer to the language's default address space since the language sees it that way.

An alternative fix would be just let ActiveFlag to be llvm::Value instead of llvm::AllocaInst, since it does not really need to be an AllocaInst.

[rjmccall]

In D47099#1105574, @yaxunl wrote:

In D47099#1105493, @rjmccall wrote:

Maybe there should just be a method that makes a primitive alloca without the casting, and then you can call that in CreateTempAlloca.

In many cases we still need to call CreateTempAlloca with cast enabled, since we are not certain there is only load from it and store to it. Any time it is stored to another memory location or passed to another function (e.g. constructor/destructor), it needs to be a pointer to the language's default address space since the language sees it that way.

Yes, I understand that. But there are some cases, like this, that do not need to produce something in LangAS::Default, and extracting out a method that just does an alloca without the unnecessary cast seems sensible.

An alternative fix would be just let ActiveFlag to be llvm::Value instead of llvm::AllocaInst, since it does not really need to be an AllocaInst.

I don't care at all about the type of ActiveFlag, but if we can generate better code at -O0 by avoiding the casts in situations where we obviously don't need them, we should do it.

[yaxunl]

Add CreateMemTempWithoutCast and CreateTempAllocaWithoutCast by John's comments.

[rjmccall]

lib/CodeGen/CGExpr.cpp

78–79

Could you change this to call CreateTempAllocaWithoutCast?

[yaxunl]

Revised by John's comments.

[rjmccall]

Thanks, LGTM.

[yaxunl]

I revert it since it caused regression on arm and some other arch's.

Script:
--
/home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/stage1/bin/clang -cc1 -internal-isystem /home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/stage1/lib/clang/7.0.0/include -nostdsysteminc -emit-llvm /home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/tools/clang/test/CodeGenCXX/conditional-temporaries.cpp -o - -triple=x86_64-apple-darwin9 -O3 | /home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/stage1/bin/FileCheck /home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/tools/clang/test/CodeGenCXX/conditional-temporaries.cpp
/home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/stage1/bin/clang -cc1 -internal-isystem /home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/stage1/lib/clang/7.0.0/include -nostdsysteminc -emit-llvm /home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/tools/clang/test/CodeGenCXX/conditional-temporaries.cpp -o - -triple=amdgcn-amd-amdhsa -O3 | /home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/stage1/bin/FileCheck /home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/tools/clang/test/CodeGenCXX/conditional-temporaries.cpp
--
Exit Code: 1

Command Output (stderr):
--
/home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/tools/clang/test/CodeGenCXX/conditional-temporaries.cpp:42:12: error: expected string not found in input
 // CHECK: ret i32 5
           ^
<stdin>:11:33: note: scanning from here
define i32 @_Z12getCtorCallsv() local_unnamed_addr #0 {
                                ^
<stdin>:14:2: note: possible intended match here
 ret i32 %0
 ^
/home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/tools/clang/test/CodeGenCXX/conditional-temporaries.cpp:48:12: error: expected string not found in input
 // CHECK: ret i32 5
           ^
<stdin>:18:33: note: scanning from here
define i32 @_Z12getDtorCallsv() local_unnamed_addr #0 {
                                ^
<stdin>:21:2: note: possible intended match here
 ret i32 %0
 ^
/home/ssglocal/clang-cmake-x86_64-avx2-linux/clang-cmake-x86_64-avx2-linux/llvm/tools/clang/test/CodeGenCXX/conditional-temporaries.cpp:54:12: error: expected string not found in input
 // CHECK: ret i1 true
           ^
<stdin>:25:34: note: scanning from here
define zeroext i1 @_Z7successv() local_unnamed_addr #0 {
                                 ^
<stdin>:30:2: note: possible intended match here
 ret i1 %cmp
 ^

--

Strange thing is that this only happens on some arch's. It passes on my x86_64/ubuntu built with clang.

lib/CodeGen/CGCall.cpp

case ABIArgInfo::InAlloca: {
       }
       break;
     }
 
     case ABIArgInfo::Indirect: {
       assert(NumIRArgs == 1);
       if (!I->isAggregate()) {
         // Make a temporary alloca to pass the argument.
-        Address Addr = CreateMemTemp(I->Ty, ArgInfo.getIndirectAlign(),
-                                     "indirect-arg-temp", /*Alloca=*/nullptr,
-                                     /*Cast=*/false);
+        Address Addr = CreateMemTempWithoutCast(
+            I->Ty, ArgInfo.getIndirectAlign(), "indirect-arg-temp");
         IRCallArgs[FirstIRArg] = Addr.getPointer();
 
         I->copyInto(*this, Addr);
       } else {
         // We want to avoid creating an unnecessary temporary+copy here;
         // however, we need one in three cases:
         // 1. If the argument is not byval, and we are required to copy the
         //    source.  (This case doesn't occur on any common architecture.)
case ABIArgInfo::Indirect: {
               (ArgInfo.getIndirectByVal() &&
                ((AS != LangAS::Default && AS != LangAS::opencl_private &&
                  AS != CGM.getASTAllocaAddressSpace())))) {
             NeedCopy = true;
           }
         }
         if (NeedCopy) {
           // Create an aligned temporary, and copy to it.
-          Address AI = CreateMemTemp(I->Ty, ArgInfo.getIndirectAlign(),
-                                     "byval-temp", /*Alloca=*/nullptr,
-                                     /*Cast=*/false);
+          Address AI = CreateMemTempWithoutCast(
+              I->Ty, ArgInfo.getIndirectAlign(), "byval-temp");
           IRCallArgs[FirstIRArg] = AI.getPointer();
           I->copyInto(*this, AI);
         } else {
           // Skip the extra memcpy call.
           auto *T = V->getType()->getPointerElementType()->getPointerTo(
               CGM.getDataLayout().getAllocaAddrSpace());
           IRCallArgs[FirstIRArg] = getTargetHooks().performAddrSpaceCast(
               *this, V, LangAS::Default, CGM.getASTAllocaAddressSpace(), T,

lib/CodeGen/CGCleanup.cpp

void EHScopeStack::popNullFixups() {
 
   while (BranchFixups.size() > MinSize &&
          BranchFixups.back().Destination == nullptr)
     BranchFixups.pop_back();
 }
 
 void CodeGenFunction::initFullExprCleanup() {
   // Create a variable to decide whether the cleanup needs to be run.
-  Address active = CreateTempAlloca(Builder.getInt1Ty(), CharUnits::One(),
-                                    "cleanup.cond");
+  Address active = CreateTempAllocaWithoutCast(
+      Builder.getInt1Ty(), CharUnits::One(), "cleanup.cond");
 
   // Initialize it to false at a site that's guaranteed to be run
   // before each evaluation.
   setBeforeOutermostConditional(Builder.getFalse(), active);
 
   // Initialize it to true at the current location.
   Builder.CreateStore(Builder.getTrue(), active);
 

lib/CodeGen/CGExpr.cpp

if (addressSpace)
     destType = llvm::Type::getInt8PtrTy(getLLVMContext(), addressSpace);
 
   if (value->getType() == destType) return value;
   return Builder.CreateBitCast(value, destType);
 }
 
 /// CreateTempAlloca - This creates a alloca and inserts it into the entry
 /// block.
-Address CodeGenFunction::CreateTempAlloca(llvm::Type *Ty, CharUnits Align,
+Address CodeGenFunction::CreateTempAllocaWithoutCast(llvm::Type *Ty,
+                                                     CharUnits Align,
                                                      const Twine &Name,
-                                          llvm::Value *ArraySize,
-                                          Address *AllocaAddr,
-                                          bool CastToDefaultAddrSpace) {
+                                                     llvm::Value *ArraySize) {
   auto Alloca = CreateTempAlloca(Ty, Name, ArraySize);
   Alloca->setAlignment(Align.getQuantity());
+  return Address(Alloca, Align);
+}
+
+/// CreateTempAlloca - This creates a alloca and inserts it into the entry
+/// block. The alloca is casted to default address space if necessary.
+Address CodeGenFunction::CreateTempAlloca(llvm::Type *Ty, CharUnits Align,
+                                          const Twine &Name,
+                                          llvm::Value *ArraySize,
+                                          Address *AllocaAddr) {
+  auto Alloca = CreateTempAllocaWithoutCast(Ty, Align, Name, ArraySize);
   if (AllocaAddr)
-    *AllocaAddr = Address(Alloca, Align);
-  llvm::Value *V = Alloca;
+    *AllocaAddr = Alloca;
+  llvm::Value *V = Alloca.getPointer();
   // Alloca always returns a pointer in alloca address space, which may
   // be different from the type defined by the language. For example,
   // in C++ the auto variables are in the default address space. Therefore
   // cast alloca to the default address space when necessary.
-  if (CastToDefaultAddrSpace && getASTAllocaAddressSpace() != LangAS::Default) {
+  if (getASTAllocaAddressSpace() != LangAS::Default) {
     auto DestAddrSpace = getContext().getTargetAddressSpace(LangAS::Default);
     llvm::IRBuilderBase::InsertPointGuard IPG(Builder);
     // When ArraySize is nullptr, alloca is inserted at AllocaInsertPt,
     // otherwise alloca is inserted at the current insertion point of the
     // builder.
     if (!ArraySize)
       Builder.SetInsertPoint(AllocaInsertPt);
     V = getTargetHooks().performAddrSpaceCast(

 }
 
 Address CodeGenFunction::CreateIRTemp(QualType Ty, const Twine &Name) {
   CharUnits Align = getContext().getTypeAlignInChars(Ty);
   return CreateTempAlloca(ConvertType(Ty), Align, Name);
 }
 
 Address CodeGenFunction::CreateMemTemp(QualType Ty, const Twine &Name,
-                                       Address *Alloca,
-                                       bool CastToDefaultAddrSpace) {
+                                       Address *Alloca) {
   // FIXME: Should we prefer the preferred type alignment here?
-  return CreateMemTemp(Ty, getContext().getTypeAlignInChars(Ty), Name, Alloca,
-                       CastToDefaultAddrSpace);
+  return CreateMemTemp(Ty, getContext().getTypeAlignInChars(Ty), Name, Alloca);
 }
 
 Address CodeGenFunction::CreateMemTemp(QualType Ty, CharUnits Align,
-                                       const Twine &Name, Address *Alloca,
-                                       bool CastToDefaultAddrSpace) {
+                                       const Twine &Name, Address *Alloca) {
   return CreateTempAlloca(ConvertTypeForMem(Ty), Align, Name,
-                          /*ArraySize=*/nullptr, Alloca,
-                          CastToDefaultAddrSpace);
+                          /*ArraySize=*/nullptr, Alloca);
+}
+
+Address CodeGenFunction::CreateMemTempWithoutCast(QualType Ty, CharUnits Align,
+                                                  const Twine &Name) {
+  return CreateTempAllocaWithoutCast(ConvertTypeForMem(Ty), Align, Name);
+}
+
+Address CodeGenFunction::CreateMemTempWithoutCast(QualType Ty,
+                                                  const Twine &Name) {
+  return CreateMemTempWithoutCast(Ty, getContext().getTypeAlignInChars(Ty),
+                                  Name);
 }
 
 /// EvaluateExprAsBool - Perform the usual unary conversions on the specified
 /// expression and compare the result against zero, returning an Int1Ty value.
 llvm::Value *CodeGenFunction::EvaluateExprAsBool(const Expr *E) {
   PGO.setCurrentStmt(E);
   if (const MemberPointerType *MPT = E->getType()->getAs<MemberPointerType>()) {
     llvm::Value *MemPtr = EmitScalarExpr(E);

lib/CodeGen/CodeGenFunction.h

public:
   /// LangAS::Default. This can happen when the target uses a restricted
   /// address space for the stack but the source language requires
   /// LangAS::Default to be a generic address space. The latter condition is
   /// common for most programming languages; OpenCL is an exception in that
   /// LangAS::Default is the private address space, which naturally maps
   /// to the stack.
   ///
   /// Because the address of a temporary is often exposed to the program in
-  /// various ways, this function will perform the cast by default. The cast
-  /// may be avoided by passing false as \p CastToDefaultAddrSpace; this is
+  /// various ways, this function will perform the cast. The original alloca
+  /// instruction is returned through \p Alloca if it is not nullptr.
+  ///
+  /// The cast is not performaed in CreateTempAllocaWithoutCast. This is
   /// more efficient if the caller knows that the address will not be exposed.
-  /// The original alloca instruction is returned through \p Alloca if it is
-  /// not nullptr.
   llvm::AllocaInst *CreateTempAlloca(llvm::Type *Ty, const Twine &Name = "tmp",
                                      llvm::Value *ArraySize = nullptr);
   Address CreateTempAlloca(llvm::Type *Ty, CharUnits align,
                            const Twine &Name = "tmp",
                            llvm::Value *ArraySize = nullptr,
-                           Address *Alloca = nullptr,
-                           bool CastToDefaultAddrSpace = true);
+                           Address *Alloca = nullptr);
+  Address CreateTempAllocaWithoutCast(llvm::Type *Ty, CharUnits align,
+                                      const Twine &Name = "tmp",
+                                      llvm::Value *ArraySize = nullptr);
 
   /// CreateDefaultAlignedTempAlloca - This creates an alloca with the
   /// default ABI alignment of the given LLVM type.
   ///
   /// IMPORTANT NOTE: This is *not* generally the right alignment for
   /// any given AST type that happens to have been lowered to the
   /// given IR type.  This should only ever be used for function-local,
   /// IR-driven manipulations like saving and restoring a value.  Do
public:
   /// PHI construction), but the type is the IR type, not the type appropriate
   /// for storing in memory.
   ///
   /// That is, this is exactly equivalent to CreateMemTemp, but calling
   /// ConvertType instead of ConvertTypeForMem.
   Address CreateIRTemp(QualType T, const Twine &Name = "tmp");
 
   /// CreateMemTemp - Create a temporary memory object of the given type, with
-  /// appropriate alignment. Cast it to the default address space if
-  /// \p CastToDefaultAddrSpace is true. Returns the original alloca
-  /// instruction by \p Alloca if it is not nullptr.
+  /// appropriate alignmen and cast it to the default address space. Returns
+  /// the original alloca instruction by \p Alloca if it is not nullptr.
   Address CreateMemTemp(QualType T, const Twine &Name = "tmp",
-                        Address *Alloca = nullptr,
-                        bool CastToDefaultAddrSpace = true);
+                        Address *Alloca = nullptr);
   Address CreateMemTemp(QualType T, CharUnits Align, const Twine &Name = "tmp",
-                        Address *Alloca = nullptr,
-                        bool CastToDefaultAddrSpace = true);
+                        Address *Alloca = nullptr);
+
+  /// CreateMemTemp - Create a temporary memory object of the given type, with
+  /// appropriate alignmen without casting it to the default address space.
+  Address CreateMemTempWithoutCast(QualType T, const Twine &Name = "tmp");
+  Address CreateMemTempWithoutCast(QualType T, CharUnits Align,
+                                   const Twine &Name = "tmp");
 
   /// CreateAggTemp - Create a temporary memory object for the given
   /// aggregate type.
   AggValueSlot CreateAggTemp(QualType T, const Twine &Name = "tmp") {
     return AggValueSlot::forAddr(CreateMemTemp(T, Name),
                                  T.getQualifiers(),
                                  AggValueSlot::IsNotDestructed,
                                  AggValueSlot::DoesNotNeedGCBarriers,

test/CodeGenCXX/conditional-temporaries.cpp

 // RUN: %clang_cc1 -emit-llvm %s -o - -triple=x86_64-apple-darwin9 -O3 | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm %s -o - -triple=amdgcn-amd-amdhsa -O3 | FileCheck %s
 
 namespace {
 
 static int ctorcalls;
 static int dtorcalls;
   
 struct A {
   A() : i(0) { ctorcalls++; }