[AMDGPU][LowerModuleLDS] Refactor partially lowered module detection #85793
Conversation
Refactor the logic that checks if a module contains mixed absolute/non-lowered LDS GVs. The check now happens latter when the "worklists" are formed. This is because in some cases (OpenMP) we can have non-lowered GVs in a lowered module, and this is normal because those GVs are just unused and removed from the list at some point before the end of `getUsesOfLDSByFunction`. Doing the check later ensures that if a mixed module is spotted, then it's a _real_ mixed module that needs rejection, not a module containing an intentionally ignored GV.
Pierre-vh
force-pushed
the
fix-lowerlds-check
branch
from
de0e5cc
to
d578f75
Compare
|
@llvm/pr-subscribers-backend-amdgpu Author: Pierre van Houtryve (Pierre-vh) ChangesRefactor the logic that checks if a module contains mixed absolute/non-lowered LDS GVs. The check now happens latter when the "worklists" are formed. This is because in some cases (OpenMP) we can have non-lowered GVs in a lowered module, and this is normal because those GVs are just unused and removed from the list at some point before the end of Doing the check later ensures that if a mixed module is spotted, then it's a real mixed module that needs rejection, not a module containing an intentionally ignored GV. Full diff: https://github.com/llvm/llvm-project/pull/85793.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp b/llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp
index b85cb26fdc9565..656f10e44c3bbc 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPULowerModuleLDSPass.cpp
@@ -340,26 +340,11 @@ class AMDGPULowerModuleLDS {
// Get uses from the current function, excluding uses by called functions
// Two output variables to avoid walking the globals list twice
- std::optional<bool> HasAbsoluteGVs;
for (auto &GV : M.globals()) {
if (!AMDGPU::isLDSVariableToLower(GV)) {
continue;
}
- // Check if the module is consistent: either all GVs are absolute (happens
- // when we run the pass more than once), or none are.
- const bool IsAbsolute = GV.isAbsoluteSymbolRef();
- if (HasAbsoluteGVs.has_value()) {
- if (*HasAbsoluteGVs != IsAbsolute) {
- report_fatal_error(
- "Module cannot mix absolute and non-absolute LDS GVs");
- }
- } else
- HasAbsoluteGVs = IsAbsolute;
-
- if (IsAbsolute)
- continue;
-
for (User *V : GV.users()) {
if (auto *I = dyn_cast<Instruction>(V)) {
Function *F = I->getFunction();
@@ -469,6 +454,31 @@ class AMDGPULowerModuleLDS {
}
}
+ // Verify that we fall into one of 2 cases:
+ // - All variables are absolute: this is a re-run of the pass
+ // so we don't have anything to do.
+ // - No variables are absolute.
+ std::optional<bool> HasAbsoluteGVs;
+ for (auto Map : {direct_map_kernel, indirect_map_kernel}) {
+ for (auto [Fn, GVs] : Map) {
+ for (auto GV : GVs) {
+ bool IsAbsolute = GV->isAbsoluteSymbolRef();
+ if (HasAbsoluteGVs.has_value()) {
+ if (*HasAbsoluteGVs != IsAbsolute) {
+ report_fatal_error(
+ "Module cannot mix absolute and non-absolute LDS GVs");
+ }
+ } else
+ HasAbsoluteGVs = IsAbsolute;
+ }
+ }
+ }
+
+ // If we only had absolute GVs, we have nothing to do, return an empty
+ // result.
+ if (HasAbsoluteGVs && *HasAbsoluteGVs)
+ return {FunctionVariableMap(), FunctionVariableMap()};
+
return {std::move(direct_map_kernel), std::move(indirect_map_kernel)};
}
@@ -1143,6 +1153,7 @@ class AMDGPULowerModuleLDS {
}
bool runOnModule(Module &M) {
+ dbgs() << "LowerModuleLDS: " << M.getName() << " (" << (void *)&M << ")\n";
CallGraph CG = CallGraph(M);
bool Changed = superAlignLDSGlobals(M);
diff --git a/llvm/test/CodeGen/AMDGPU/lds-reject-mixed-absolute-addresses.ll b/llvm/test/CodeGen/AMDGPU/lds-reject-mixed-absolute-addresses.ll
index b512a43aa10222..b1f4f2ef1ef535 100644
--- a/llvm/test/CodeGen/AMDGPU/lds-reject-mixed-absolute-addresses.ll
+++ b/llvm/test/CodeGen/AMDGPU/lds-reject-mixed-absolute-addresses.ll
@@ -8,7 +8,7 @@
define amdgpu_kernel void @kern() {
%val0 = load i32, ptr addrspace(3) @var1
%val1 = add i32 %val0, 4
- store i32 %val1, ptr addrspace(3) @var1
+ store i32 %val1, ptr addrspace(3) @var2
ret void
}
|
|
I think the control flow around deleting dead variables is a little off. For example, the openmp cause here is a variable in llvm.compiler.used and otherwise dead - removing that despite being in compiler.user seems legitimate as the compiler does nothing with dead lds variables. An argument could be made for deleting all unused lds variables, ignoring llvm.used as well, since they're module-local and otherwise inaccessible, and I think that is partially reified in the pass. However in the new might-run-multiple-times world, it's not totally clear to me whether an unused variable with an absolute address assigned is dead or not. We're on slightly thin ice here. The pass was written assuming it was run once on the whole code object. We'd like it to run incrementally and/or somehow support separate linking. That's quite complicated and not an immediate req so we're trying to perturb it to work in the thinlto style case. I can see bugs coming in. @arsenm did we ever get a reasonable place in tree for runtime tests? As in a bunch of programs written in C++ (ideally) or HIP/opencl (if one must) for things like this? There's some tests in libc which do the right sort of thing but I'm not totally sure I can pitch backend specific execution tests as being a part of libc |
This is permanently stuck in "coming soon". There seems to be a fragment of HIP support in llvm-test-suite, but last I tried to use it I couldn't get it to build. It seemed to have odd expectations for how to find rocm |
…lvm#85793) Refactor the logic that checks if a module contains mixed absolute/non-lowered LDS GVs. The check now happens latter when the "worklists" are formed. This is because in some cases (OpenMP) we can have non-lowered GVs in a lowered module, and this is normal because those GVs are just unused and removed from the list at some point before the end of `getUsesOfLDSByFunction`. Doing the check later ensures that if a mixed module is spotted, then it's a _real_ mixed module that needs rejection, not a module containing an intentionally ignored GV.
Refactor the logic that checks if a module contains mixed absolute/non-lowered LDS GVs.
The check now happens latter when the "worklists" are formed. This is because in some cases (OpenMP) we can have non-lowered GVs in a lowered module, and this is normal because those GVs are just unused and removed from the list at some point before the end of
getUsesOfLDSByFunction.Doing the check later ensures that if a mixed module is spotted, then it's a real mixed module that needs rejection, not a module containing an intentionally ignored GV.