[GlobalOpt] Do not shrink global to bool for an unfavorable AS

[cdevadas]

Summary

Do not call TryToShrinkGlobalToBoolean for address spaces
that don't allow initializers. It inserts an initializer value
while shrinking to bool. Used the target hook introduced with
D109337 to skip this call for the restricted address spaces.

[arsenm]

llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll

2–3

Needs REQUIRES: amdgpu-registered-target, or to move to an AMDGPU test directory

[cdevadas]

Added REQUIRES field in the lit test.

[jdoerfert]

llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll

15

Why is the check and amdgcn line the same, that I don't understand. Should it not trigger for "check"?

[arsenm]

llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll

15

The host line won't fail the address space check, and it serves as a sanity check that the transform would be performed otherwise

[arsenm]

llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll

2

Probably should use an explicit target for the reference line

[cdevadas]

llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll

2

I intended to capture the default behavior along with the output for AMDGPU.
The default implementation of canHaveNonUndefGlobalInitializerInAddressSpace allows only AS(0) to have an initializer.
There is no target that currently allows initializer for AS(3) and any arbitrary target used here will transform the code similar to AMDGPU.
I will use x86. The test in the future shouldn't break without one.

[tra]

The patch looks reasonable, but I have a question about the test.

llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll

34–42

I'm confused. I see no differences between AMDGCN and CHECK anywhere in the test file. AFAICT, the test would work if I were to use CHECK in the test with --mtriple=amdgcn-amd-amdhsa. If both CHECK and AMDGCN are identical, why do we need two different check patterns?

Did I miss something?

[jdoerfert]

llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll

34–42

You did not: https://reviews.llvm.org/D109823#inline-1046556

[cdevadas]

Fixed the lit test to have only one check pattern.

[arsenm]

llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll

3

If they're going to have the same output, there's no point in the second run line. The cpu run line was only useful as a reference to make sure the transform happened otherwise

5

Would also need x86 check

[cdevadas]

Retained just one RUN line.

llvm/lib/Transforms/IPO/GlobalOpt.cpp

processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
   if (GS.StoredType == GlobalStatus::StoredOnce && StoredOnceValue) {
     // Avoid speculating constant expressions that might trap (div/rem).
     auto *SOVConstant = dyn_cast<Constant>(StoredOnceValue);
     if (SOVConstant && SOVConstant->canTrap())
       return Changed;
 
     Function &StoreFn =
         const_cast<Function &>(*GS.StoredOnceStore->getFunction());
+    bool CanHaveNonUndefGlobalInitializer =
+        GetTTI(StoreFn).canHaveNonUndefGlobalInitializerInAddressSpace(
+            GV->getType()->getAddressSpace());
     // If the initial value for the global was an undef value, and if only
     // one other value was stored into it, we can just change the
     // initializer to be the stored value, then delete all stores to the
     // global.  This allows us to mark it constant.
     // This is restricted to address spaces that allow globals to have
     // initializers. NVPTX, for example, does not support initializers for
     // shared memory (AS 3).
     if (SOVConstant && SOVConstant->getType() == GV->getValueType() &&
         isa<UndefValue>(GV->getInitializer()) &&
-        GetTTI(StoreFn).canHaveNonUndefGlobalInitializerInAddressSpace(
-            GV->getType()->getAddressSpace())) {
+        CanHaveNonUndefGlobalInitializer) {
       // Change the initial value here.
       GV->setInitializer(SOVConstant);
 
       // Clean up any obviously simplifiable users now.
       CleanupConstantGlobalUsers(GV, GV->getInitializer(), DL, GetTLI);
 
       if (GV->use_empty()) {
         LLVM_DEBUG(dbgs() << "   *** Substituting initializer allowed us to "
                           << "simplify all users and delete global!\n");
         GV->eraseFromParent();
         ++NumDeleted;
       }
       ++NumSubstitute;
       return true;
     }
 
     // Try to optimize globals based on the knowledge that only one value
     // (besides its initializer) is ever stored to the global.
     if (optimizeOnceStoredGlobal(GV, StoredOnceValue, GS.Ordering, DL, GetTLI))
       return true;
 
     // Otherwise, if the global was not a boolean, we can shrink it to be a
-    // boolean.
-    if (SOVConstant && GS.Ordering == AtomicOrdering::NotAtomic) {
+    // boolean. Skip this optimization for AS that doesn't allow an initializer.
+    if (SOVConstant && GS.Ordering == AtomicOrdering::NotAtomic &&
+        (!isa<UndefValue>(GV->getInitializer()) ||
+         CanHaveNonUndefGlobalInitializer)) {
       if (TryToShrinkGlobalToBoolean(GV, SOVConstant)) {
         ++NumShrunkToBool;
         return true;
       }
     }
   }
 
   return Changed;

llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll

+; RUN: opt -passes=globalopt --mtriple=amdgcn-amd-amdhsa < %s -S | FileCheck %s
+; REQUIRES: amdgpu-registered-target
+
+@gvar = internal unnamed_addr global i32 undef
+@lvar = internal unnamed_addr addrspace(3) global i32 undef
+
+; Should optimize @gvar.
+; CHECK-NOT: @gvar
+
+; Negative test for AS(3). Skip shrink global to bool optimization.
+; CHECK: @lvar = internal unnamed_addr addrspace(3) global i32 undef
+
+define void @test_global_var() {
+; CHECK-LABEL: @test_global_var(
+; CHECK:    store volatile i32 10, i32* undef, align 4
+;
+entry:
+  store i32 10, i32* @gvar
+  br label %exit
+exit:
+  %ld = load i32, i32* @gvar
+  store volatile i32 %ld, i32* undef
+  ret void
+}
+
+define void @test_lds_var() {
+; CHECK-LABEL: @test_lds_var(
+; CHECK:    store i32 10, i32 addrspace(3)* @lvar, align 4
+; CHECK:    [[LD:%.*]] = load i32, i32 addrspace(3)* @lvar, align 4
+; CHECK:    store volatile i32 [[LD]], i32* undef, align 4
+;
+entry:
+  store i32 10, i32 addrspace(3)* @lvar
+  br label %exit
+exit:
+  %ld = load i32, i32 addrspace(3)* @lvar
+  store volatile i32 %ld, i32* undef
+  ret void
+}