[clang][amdgpu] - Choose when to promote VarDecl to address space 4.

[estewart08]

Summary

There are instances where clang codegen creates stores to
address space 4 in ctors, which causes a crash in llc.
This store was being optimized out at opt levels > 0.

For example:

pragma omp declare target
static const double log_smallx = log2(smallx);
pragma omp end declare target

This patch ensures that any global const that does not
have constant initialization stays in address space 1.

Note - a second patch is in the works where all global
constants are placed in address space 1 during
codegen and then the opt pass InferAdressSpaces
will promote to address space 4 where necessary.

[estewart08]

Resubmit patch with lint.

[arsenm]

There's no real advantage to using addrspace(4) at all

[arsenm]

In D115661#3190041, @arsenm wrote:

There's no real advantage to using addrspace(4) at all

There are a few places where it's used as an optimization hint / as a crutch where we don't have a proper analysis. Fundamentally I would like to eliminate addrspace(4) eventually

[JonChesterfield]

Patch looks ok to me. This will fix the miscompile (we end up with a store to addrspace(4) at present) without upsetting whatever hacks rely on addrspace(4). @arsenm reasonable as a point fix?

[arsenm]

In D115661#3190324, @JonChesterfield wrote:

Patch looks ok to me. This will fix the miscompile (we end up with a store to addrspace(4) at present) without upsetting whatever hacks rely on addrspace(4). @arsenm reasonable as a point fix?

Yes, I'm mostly saying to not waste your time trying to infer addrspace(4) in InferAddressSpaces or anything like that

[JonChesterfield]

That sounds good here. Infer addrspaces is pretty complicated, given marginal benefit for (4) this patch seems to hit the mark.

[yaxunl]

This may cause perf regressions for HIP.

clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp

10

Do you know why this is not treated as constant initialization?

[estewart08]

In D115661#3190477, @yaxunl wrote:

This may cause perf regressions for HIP.

Do you have a test that would show such a regression? Emitting a store to address space (4) in a constructor seems the wrong thing to do.

clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp

10

There is no initialization here:

const int A::Foo;

It seems the compiler ignores the 123 in the struct.

I see address space (4) when the test is written like this:

struct A {
  static const int Foo;
};
const int A::Foo = 123;

[yaxunl]

In D115661#3192983, @estewart08 wrote:

In D115661#3190477, @yaxunl wrote:

This may cause perf regressions for HIP.

Do you have a test that would show such a regression? Emitting a store to address space (4) in a constructor seems the wrong thing to do.

The two lit tests which changed from addr space 4 to 1 demonstrated that. In alias analysis, if a variable is in addr space 4, the backend knows that it is constant and can do optimizations on it. After changing to addr space 1, those optimizations are gone.

I am not objecting to putting variables like const float x=log2(y) to addr space 1. However, the lit tests indicate the condition check is too restrictive.

clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp

10

In this case, there are two Decl's for A::Foo and the initializer of A::Foo is on the first initializer whereas hasConstantInitialization only checks the final Decl, which does not have an initializer. Therefore hasConstantInitialization may conservatively return false for a variable with a constant initializer in a previous decl.

clang codegen calls VarDecl::getAnyInitializer to checks all Decls of a variable when emitting its initializer in LLVM IR

https://github.com/llvm/llvm-project/blob/5336befe8c3cde08cec020583700b4d2ba25ac16/clang/lib/AST/Decl.cpp#L2277

I would suggest calling getAnyInitializer to get the initializer, then check whether it is constant expr to determine whether the variable will have a constant initializer in IR. I think hasConstantInitialization does not do that because it has multiple usages, not just for clang codegen, therefore it does not check all decls.

[arsenm]

In D115661#3193152, @yaxunl wrote:

In D115661#3192983, @estewart08 wrote:

In D115661#3190477, @yaxunl wrote:

This may cause perf regressions for HIP.

Do you have a test that would show such a regression? Emitting a store to address space (4) in a constructor seems the wrong thing to do.

The two lit tests which changed from addr space 4 to 1 demonstrated that. In alias analysis, if a variable is in addr space 4, the backend knows that it is constant and can do optimizations on it. After changing to addr space 1, those optimizations are gone.

The backend also knows because the constant flag is set on the global variable. Addrspace(4) is a kludge which is largely redundant with other mechanisms for indicating constants

[JonChesterfield]

Changing the has-constant-initialiser check to a more precise/robust one sounds reasonable to me.

In D115661#3193157, @arsenm wrote:

The backend also knows because the constant flag is set on the global variable. Addrspace(4) is a kludge which is largely redundant with other mechanisms for indicating constants

There's an interesting edge case here - we might want variables that are assigned to from a global ctor and subsequently constant, in which case they are not 'constant' as far as IR is concerned, but they are amenable to scalar loads and will be invariant once loaded. Some tables of data that are computed once in a .ctor kernel launch and then read by other kernels. I'm not totally sure how to express that pattern in IR though.

[estewart08]

In D115661#3193157, @arsenm wrote:

In D115661#3193152, @yaxunl wrote:

In D115661#3192983, @estewart08 wrote:

In D115661#3190477, @yaxunl wrote:

This may cause perf regressions for HIP.

Do you have a test that would show such a regression? Emitting a store to address space (4) in a constructor seems the wrong thing to do.

The two lit tests which changed from addr space 4 to 1 demonstrated that. In alias analysis, if a variable is in addr space 4, the backend knows that it is constant and can do optimizations on it. After changing to addr space 1, those optimizations are gone.

The backend also knows because the constant flag is set on the global variable. Addrspace(4) is a kludge which is largely redundant with other mechanisms for indicating constants

If I am understanding you correctly, putting things in address space (4) has little to no performance benefit. @yaxunl seems to think otherwise. I agree that we can further constrain the address space (1) criteria, but I am getting conflicting viewpoints here on performance.

[yaxunl]

In D115661#3193157, @arsenm wrote:

In D115661#3193152, @yaxunl wrote:

In D115661#3192983, @estewart08 wrote:

In D115661#3190477, @yaxunl wrote:

This may cause perf regressions for HIP.

Do you have a test that would show such a regression? Emitting a store to address space (4) in a constructor seems the wrong thing to do.

The two lit tests which changed from addr space 4 to 1 demonstrated that. In alias analysis, if a variable is in addr space 4, the backend knows that it is constant and can do optimizations on it. After changing to addr space 1, those optimizations are gone.

The backend also knows because the constant flag is set on the global variable. Addrspace(4) is a kludge which is largely redundant with other mechanisms for indicating constants

If backend can only rely on constant flag then we do not need put global variables in constant addr space.

Let's leave this patch as it is now. And revisit it if there are any regressions found.

[yaxunl]

In D115661#3193413, @yaxunl wrote:

In D115661#3193157, @arsenm wrote:

In D115661#3193152, @yaxunl wrote:

In D115661#3192983, @estewart08 wrote:

In D115661#3190477, @yaxunl wrote:

This may cause perf regressions for HIP.

Do you have a test that would show such a regression? Emitting a store to address space (4) in a constructor seems the wrong thing to do.

The two lit tests which changed from addr space 4 to 1 demonstrated that. In alias analysis, if a variable is in addr space 4, the backend knows that it is constant and can do optimizations on it. After changing to addr space 1, those optimizations are gone.

The backend also knows because the constant flag is set on the global variable. Addrspace(4) is a kludge which is largely redundant with other mechanisms for indicating constants

If backend can only rely on constant flag then we do not need put global variables in constant addr space.

Let's leave this patch as it is now. And revisit it if there are any regressions found.

What about situations of a derived pointer to the global variable? For example

const int a[100] ;

foo(&a[50]);

If we put a in addr space 4, it is easy to deduce &a[50] is constant. If we put a in addr space 1, how does backend know &a[50] is constant?

[arsenm]

In D115661#3193431, @yaxunl wrote:

What about situations of a derived pointer to the global variable? For example

const int a[100] ;

foo(&a[50]);

If we put a in addr space 4, it is easy to deduce &a[50] is constant. If we put a in addr space 1, how does backend know &a[50] is constant?

Everything using alias analysis looks back for the definition of the original object

clang/lib/CodeGen/TargetInfo.cpp

AMDGPUTargetCodeGenInfo::getGlobalVarAddressSpace(CodeGenModule &CGM,
   if (!D)
     return DefaultGlobalAS;
 
   LangAS AddrSpace = D->getType().getAddressSpace();
   assert(AddrSpace == LangAS::Default || isTargetAddressSpace(AddrSpace));
   if (AddrSpace != LangAS::Default)
     return AddrSpace;
 
-  if (CGM.isTypeConstant(D->getType(), false)) {
+  // Only promote to address space 4 if VarDecl has constant initialization.
+  if (CGM.isTypeConstant(D->getType(), false) &&
+      D->hasConstantInitialization()) {
     if (auto ConstAS = CGM.getTarget().getConstantAddressSpace())
       return ConstAS.getValue();
   }
   return DefaultGlobalAS;
 }
 
 llvm::SyncScope::ID
 AMDGPUTargetCodeGenInfo::getLLVMSyncScopeID(const LangOptions &LangOpts,

clang/test/CodeGenCXX/cxx0x-initializer-stdinitializerlist.cpp

 // X86: @_ZN15partly_constant1kE ={{.*}} global i32 0, align 4
 // X86: @_ZN15partly_constant2ilE ={{.*}} global {{.*}} null, align 8
 // X86: @[[PARTLY_CONSTANT_OUTER:_ZGRN15partly_constant2ilE_]] = internal global {{.*}} zeroinitializer, align 8
 // X86: @[[PARTLY_CONSTANT_INNER:_ZGRN15partly_constant2ilE0_]] = internal global [3 x {{.*}}] zeroinitializer, align 8
 // X86: @[[PARTLY_CONSTANT_FIRST:_ZGRN15partly_constant2ilE1_]] = internal constant [3 x i32] [i32 1, i32 2, i32 3], align 4
 // X86: @[[PARTLY_CONSTANT_SECOND:_ZGRN15partly_constant2ilE2_]] = internal global [2 x i32] zeroinitializer, align 4
 // X86: @[[PARTLY_CONSTANT_THIRD:_ZGRN15partly_constant2ilE3_]] = internal constant [4 x i32] [i32 5, i32 6, i32 7, i32 8], align 4
 // AMDGCN: @_ZN15partly_constant1kE ={{.*}} addrspace(1) global i32 0, align 4
-// AMDGCN: @_ZN15partly_constant2ilE ={{.*}} addrspace(4) global {{.*}} null, align 8
-// AMDGCN: @[[PARTLY_CONSTANT_OUTER:_ZGRN15partly_constant2ilE_]] = internal addrspace(4) global {{.*}} zeroinitializer, align 8
-// AMDGCN: @[[PARTLY_CONSTANT_INNER:_ZGRN15partly_constant2ilE0_]] = internal addrspace(4) global [3 x {{.*}}] zeroinitializer, align 8
-// AMDGCN: @[[PARTLY_CONSTANT_FIRST:_ZGRN15partly_constant2ilE1_]] = internal addrspace(4) constant [3 x i32] [i32 1, i32 2, i32 3], align 4
-// AMDGCN: @[[PARTLY_CONSTANT_SECOND:_ZGRN15partly_constant2ilE2_]] = internal addrspace(4) global [2 x i32] zeroinitializer, align 4
-// AMDGCN: @[[PARTLY_CONSTANT_THIRD:_ZGRN15partly_constant2ilE3_]] = internal addrspace(4) constant [4 x i32] [i32 5, i32 6, i32 7, i32 8], align 4
+// AMDGCN: @_ZN15partly_constant2ilE ={{.*}} addrspace(1) global {{.*}} null, align 8
+// AMDGCN: @[[PARTLY_CONSTANT_OUTER:_ZGRN15partly_constant2ilE_]] = internal addrspace(1) global {{.*}} zeroinitializer, align 8
+// AMDGCN: @[[PARTLY_CONSTANT_INNER:_ZGRN15partly_constant2ilE0_]] = internal addrspace(1) global [3 x {{.*}}] zeroinitializer, align 8
+// AMDGCN: @[[PARTLY_CONSTANT_FIRST:_ZGRN15partly_constant2ilE1_]] = internal addrspace(1) constant [3 x i32] [i32 1, i32 2, i32 3], align 4
+// AMDGCN: @[[PARTLY_CONSTANT_SECOND:_ZGRN15partly_constant2ilE2_]] = internal addrspace(1) global [2 x i32] zeroinitializer, align 4
+// AMDGCN: @[[PARTLY_CONSTANT_THIRD:_ZGRN15partly_constant2ilE3_]] = internal addrspace(1) constant [4 x i32] [i32 5, i32 6, i32 7, i32 8], align 4
 
 // X86: @[[REFTMP1:.*]] = private constant [2 x i32] [i32 42, i32 43], align 4
 // X86: @[[REFTMP2:.*]] = private constant [3 x %{{.*}}] [%{{.*}} { i32 1 }, %{{.*}} { i32 2 }, %{{.*}} { i32 3 }], align 4
 // AMDGCN: @[[REFTMP1:.*]] = private addrspace(4) constant [2 x i32] [i32 42, i32 43], align 4
 // AMDGCN: @[[REFTMP2:.*]] = private addrspace(4) constant [3 x %{{.*}}] [%{{.*}} { i32 1 }, %{{.*}} { i32 2 }, %{{.*}} { i32 3 }], align 4
 
 // CHECK: appending global
 

clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp

 // RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=X86,CXX11X86
 // RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple x86_64-linux-gnu | FileCheck %s --check-prefixes=X86,CXX17X86
 // RUN: %clang_cc1 -std=c++11 %s -emit-llvm -o - -triple amdgcn-amd-amdhsa | FileCheck %s --check-prefixes=AMD,CXX11AMD
 // RUN: %clang_cc1 -std=c++1z %s -emit-llvm -o - -triple amdgcn-amd-amdhsa | FileCheck %s --check-prefixes=AMD,CXX17AMD
 
 struct A {
   static const int Foo = 123;
 };
 // X86: @_ZN1A3FooE ={{.*}} constant i32 123, align 4
-// AMD: @_ZN1A3FooE ={{.*}} addrspace(4) constant i32 123, align 4
+// AMD: @_ZN1A3FooE ={{.*}} addrspace(1) constant i32 123, align 4
 const int *p = &A::Foo; // emit available_externally
 const int A::Foo;       // convert to full definition
 
 struct PODWithInit {
   int g = 42;
   char h = 43;
 };
 struct CreatePOD {

 struct MutableBar {
   mutable int b;
 };
 
 struct Foo {
   // CXX11X86: @_ZN3Foo21ConstexprStaticMemberE = available_externally constant i32 42,
   // CXX17X86: @_ZN3Foo21ConstexprStaticMemberE = linkonce_odr constant i32 42,
   // CXX11AMD: @_ZN3Foo21ConstexprStaticMemberE = available_externally addrspace(4) constant i32 42,
-  // CXX17AMD: @_ZN3Foo21ConstexprStaticMemberE = linkonce_odr addrspace(4) constant i32 42,
+  // CXX17AMD: @_ZN3Foo21ConstexprStaticMemberE = linkonce_odr addrspace(4) constant i32 42, comdat, align 4
   static constexpr int ConstexprStaticMember = 42;
   // X86: @_ZN3Foo17ConstStaticMemberE = available_externally constant i32 43,
   // AMD: @_ZN3Foo17ConstStaticMemberE = available_externally addrspace(4) constant i32 43,
   static const int ConstStaticMember = 43;
 
   // CXX11X86: @_ZN3Foo23ConstStaticStructMemberE = available_externally constant %struct.Bar { i32 44 },
   // CXX17X86: @_ZN3Foo23ConstStaticStructMemberE = linkonce_odr constant %struct.Bar { i32 44 },
   // CXX11AMD: @_ZN3Foo23ConstStaticStructMemberE = available_externally addrspace(1) constant %struct.Bar { i32 44 },