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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D115661#3192983 <https://reviews.llvm.org/D115661#3192983>, @estewart08 wrote:

> In D115661#3190477 <https://reviews.llvm.org/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.



================
Comment at: clang/test/CodeGenCXX/cxx11-extern-constexpr.cpp:10
 // 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
----------------
estewart08 wrote:
> yaxunl wrote:
> > Do you know why this is not treated as constant initialization?
> > 
> 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;
> ```
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.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115661/new/

https://reviews.llvm.org/D115661