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

80 ↗

(On Diff #147860)

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/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");
+                                    "cleanup.cond", /*ArraySize=*/nullptr,
+                                    /*Alloca=*/nullptr, /*Cast=*/false);
 
   // 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);
 

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++; }