Revert "[AMDGPU][LTO] Assume closed world after linking (#105845)"

[shiltian]

We can't assume closed world even in full LTO post-link stage. It is only true
if we are building a "GPU executable". However, AMDGPU does support "dyamic
library". I'm not aware of any approach to tell if it is relocatable link when
we create the pass. For now let's revert the patch as it is currently breaking things.
We can re-enable it once we can handle it correctly.

[shiltian]

• Revert "[AMDGPU][LTO] Assume closed world after linking (#105845)" #106889 👈
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @shiltian and the rest of your teammates on Graphite

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-lto

Author: Shilei Tian (shiltian)

Changes

We can't assume closed world even in full LTO post-link stage. It is only true
if we are building a "GPU executable". However, AMDGPU does support "dyamic
library". I'm not aware of any approach to tell if it is relocatable link when
we create the pass. For now let's revert the patch. We can re-enable it once we
can handle it correctly.

---

Full diff: https://github.com/llvm/llvm-project/pull/106889.diff

3 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp (-4)
• (modified) llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp (+2-6)
• (removed) llvm/test/LTO/AMDGPU/gpu-rdc-amdgpu-attrs.ll (-13)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp b/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
index 0d7b4caa8156ed..72049f0aa6b86e 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp
@@ -1065,10 +1065,6 @@ static bool runImpl(Module &M, AnalysisGetter &AG, TargetMachine &TM,
 
   Attributor A(Functions, InfoCache, AC);
 
-  LLVM_DEBUG(dbgs() << "Module " << M.getName() << " is "
-                    << (AC.IsClosedWorldModule ? "" : "not ")
-                    << "assumed to be a closed world.\n");
-
   for (Function &F : M) {
     if (F.isIntrinsic())
       continue;
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
index a769bc9e486573..7df39f35478077 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
@@ -771,12 +771,8 @@ void AMDGPUTargetMachine::registerPassBuilderCallbacks(PassBuilder &PB) {
           PM.addPass(AMDGPUSwLowerLDSPass(*this));
         if (EnableLowerModuleLDS)
           PM.addPass(AMDGPULowerModuleLDSPass(*this));
-
-        if (EnableAMDGPUAttributor && Level != OptimizationLevel::O0) {
-          AMDGPUAttributorOptions Opts;
-          Opts.IsClosedWorld = true;
-          PM.addPass(AMDGPUAttributorPass(*this, Opts));
-        }
+        if (EnableAMDGPUAttributor && Level != OptimizationLevel::O0)
+          PM.addPass(AMDGPUAttributorPass(*this));
       });
 
   PB.registerRegClassFilterParsingCallback(
diff --git a/llvm/test/LTO/AMDGPU/gpu-rdc-amdgpu-attrs.ll b/llvm/test/LTO/AMDGPU/gpu-rdc-amdgpu-attrs.ll
deleted file mode 100644
index 8228f21b56b132..00000000000000
--- a/llvm/test/LTO/AMDGPU/gpu-rdc-amdgpu-attrs.ll
+++ /dev/null
@@ -1,13 +0,0 @@
-; RUN: opt -O3 -debug-only=amdgpu-attributor -S -o - %s 2>&1 | FileCheck %s --check-prefix=PRE-LINK
-; RUN: opt -passes="lto<O3>" -debug-only=amdgpu-attributor -S -o - %s 2>&1 | FileCheck %s --check-prefix=POST-LINK
-
-; REQUIRES: amdgpu-registered-target
-; REQUIRES: asserts
-
-target triple = "amdgcn-amd-amdhsa"
-
-; PRE-LINK: Module {{.*}} is not assumed to be a closed world.
-; POST-LINK: Module {{.*}} is assumed to be a closed world.
-define hidden noundef i32 @_Z3foov() {
-  ret i32 1
-}

[jhuber6]

Maybe we could modify the AMDGPUAttributor pass to do this by default if there's no external symbols?

[shiltian]

Well, the assumption doesn't hold even w/ externally visible symbols in relocatable link. For example, if we read the callee from an externally visible global variable, that variable could be modified by other "dynamic library" to a function that is not part of this module.

This needs either to be properly handled in AAIndirectCallInfo, or only to set the closed world bit when it is really a closed world.