[llvm] [AMDGPU][SplitModule] Cleanup CallsExternal Handling (PR #106528)
Pierre van Houtryve via llvm-commits
llvm-commits at lists.llvm.org
Sun Oct 6 22:45:11 PDT 2024
https://github.com/Pierre-vh updated https://github.com/llvm/llvm-project/pull/106528
>From dc18a3bba3b510b90e705df88f429464080971df Mon Sep 17 00:00:00 2001
From: pvanhout <pierre.vanhoutryve at amd.com>
Date: Mon, 16 Sep 2024 09:41:49 +0200
Subject: [PATCH 1/3] [AMDGPU][SplitModule] Cleanup CallsExternal Handling
- Don't treat inline ASM as indirect calls
- Remove alias testing, which was broken (only working by pure luck right now) and isn't needed anyway. GlobalOpt should take care of them for us.
---
llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp | 91 ++++++++++++++-----
.../AMDGPU/indirect-call-inline-asm.ll | 41 +++++++++
.../AMDGPU/kernels-alias-dependencies.ll | 41 ---------
.../AMDGPU/kernels-dependency-indirect.ll | 12 ---
4 files changed, 110 insertions(+), 75 deletions(-)
create mode 100644 llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm.ll
delete mode 100644 llvm/test/tools/llvm-split/AMDGPU/kernels-alias-dependencies.ll
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
index e97a7f4e075f7f..e76fc937793669 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
@@ -43,6 +43,7 @@
#include "llvm/Analysis/CallGraph.h"
#include "llvm/Analysis/TargetTransformInfo.h"
#include "llvm/IR/Function.h"
+#include "llvm/IR/InstIterator.h"
#include "llvm/IR/Instruction.h"
#include "llvm/IR/Module.h"
#include "llvm/IR/User.h"
@@ -103,6 +104,15 @@ static cl::opt<bool> NoExternalizeGlobals(
cl::desc("disables externalization of global variable with local linkage; "
"may cause globals to be duplicated which increases binary size"));
+static cl::opt<bool> NoExternalizeOnAddrTaken(
+ "amdgpu-module-splitting-no-externalize-address-taken", cl::Hidden,
+ cl::desc(
+ "disables externalization of functions whose addresses are taken"));
+
+static cl::opt<bool> InlineAsmIsIndirectCall(
+ "amdgpu-module-splitting-inline-asm-is-indirect-call", cl::Hidden,
+ cl::desc("consider inline assembly as an indirect call"));
+
static cl::opt<std::string>
ModuleDotCfgOutput("amdgpu-module-splitting-print-module-dotcfg",
cl::Hidden,
@@ -482,6 +492,9 @@ void SplitGraph::buildGraph(CallGraph &CG) {
dbgs()
<< "[build graph] constructing graph representation of the input\n");
+ // FIXME(?): Is the callgraph really worth using if we have to iterate the
+ // function again whenever it fails to give us enough information?
+
// We build the graph by just iterating all functions in the module and
// working on their direct callees. At the end, all nodes should be linked
// together as expected.
@@ -492,29 +505,52 @@ void SplitGraph::buildGraph(CallGraph &CG) {
continue;
// Look at direct callees and create the necessary edges in the graph.
- bool HasIndirectCall = false;
- Node &N = getNode(Cache, Fn);
+ SetVector<const Function *> DirectCallees;
+ bool CallsExternal = false;
for (auto &CGEntry : *CG[&Fn]) {
auto *CGNode = CGEntry.second;
- auto *Callee = CGNode->getFunction();
- if (!Callee) {
- // TODO: Don't consider inline assembly as indirect calls.
- if (CGNode == CG.getCallsExternalNode())
- HasIndirectCall = true;
- continue;
- }
-
- if (!Callee->isDeclaration())
- createEdge(N, getNode(Cache, *Callee), EdgeKind::DirectCall);
+ if (auto *Callee = CGNode->getFunction()) {
+ if (!Callee->isDeclaration())
+ DirectCallees.insert(Callee);
+ } else if (CGNode == CG.getCallsExternalNode())
+ CallsExternal = true;
}
// Keep track of this function if it contains an indirect call and/or if it
// can be indirectly called.
- if (HasIndirectCall) {
- LLVM_DEBUG(dbgs() << "indirect call found in " << Fn.getName() << "\n");
- FnsWithIndirectCalls.push_back(&Fn);
+ if (CallsExternal) {
+ LLVM_DEBUG(dbgs() << " [!] callgraph is incomplete for " << Fn.getName()
+ << " - analyzing function\n");
+
+ bool HasIndirectCall = false;
+ for (const auto &Inst : instructions(Fn)) {
+ // look at all calls without a direct callee.
+ if (const auto *CB = dyn_cast<CallBase>(&Inst);
+ CB && !CB->getCalledFunction()) {
+ // inline assembly can be ignored, unless InlineAsmIsIndirectCall is
+ // true.
+ if (CB->isInlineAsm()) {
+ if (InlineAsmIsIndirectCall)
+ HasIndirectCall = true;
+ LLVM_DEBUG(dbgs() << " found inline assembly\n");
+ continue;
+ }
+
+ // everything else is handled conservatively.
+ HasIndirectCall = true;
+ }
+ }
+
+ if (HasIndirectCall) {
+ LLVM_DEBUG(dbgs() << " indirect call found\n");
+ FnsWithIndirectCalls.push_back(&Fn);
+ }
}
+ Node &N = getNode(Cache, Fn);
+ for (const auto *Callee : DirectCallees)
+ createEdge(N, getNode(Cache, *Callee), EdgeKind::DirectCall);
+
if (canBeIndirectlyCalled(Fn))
IndirectlyCallableFns.push_back(&Fn);
}
@@ -1326,13 +1362,23 @@ static void splitAMDGPUModule(
//
// Additionally, it guides partitioning to not duplicate this function if it's
// called directly at some point.
- for (auto &Fn : M) {
- if (Fn.hasAddressTaken()) {
- if (Fn.hasLocalLinkage()) {
- LLVM_DEBUG(dbgs() << "[externalize] " << Fn.getName()
- << " because its address is taken\n");
+ //
+ // TODO: Could we be smarter about this ? This makes all functions whose
+ // addresses are taken non-copyable. We should probably model this type of
+ // constraint in the graph and use it to guide splitting, instead of
+ // externalizing like this. Maybe non-copyable should really mean "keep one
+ // visible copy, then internalize all other copies" for some functions?
+ if (!NoExternalizeOnAddrTaken) {
+ for (auto &Fn : M) {
+ // TODO: Should aliases count? Probably not but they're so rare I'm not
+ // sure it's worth fixing.
+ if (Fn.hasAddressTaken()) {
+ if (Fn.hasLocalLinkage()) {
+ LLVM_DEBUG(dbgs() << "[externalize] " << Fn.getName()
+ << " because its address is taken\n");
+ }
+ externalize(Fn);
}
- externalize(Fn);
}
}
@@ -1368,7 +1414,8 @@ static void splitAMDGPUModule(
dbgs() << "[graph] nodes:\n";
for (const SplitGraph::Node *N : SG.nodes()) {
dbgs() << " - [" << N->getID() << "]: " << N->getName() << " "
- << (N->isGraphEntryPoint() ? "(entry)" : "") << "\n";
+ << (N->isGraphEntryPoint() ? "(entry)" : "") << " "
+ << (N->isNonCopyable() ? "(noncopyable)" : "") << "\n";
}
});
diff --git a/llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm.ll b/llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm.ll
new file mode 100644
index 00000000000000..5c7ddaeecd7044
--- /dev/null
+++ b/llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm.ll
@@ -0,0 +1,41 @@
+; RUN: llvm-split -o %t %s -j 2 -mtriple amdgcn-amd-amdhsa -amdgpu-module-splitting-no-externalize-address-taken
+; RUN: llvm-dis -o - %t0 | FileCheck --check-prefix=CHECK0 --implicit-check-not=define %s
+; RUN: llvm-dis -o - %t1 | FileCheck --check-prefix=CHECK1 --implicit-check-not=define %s
+
+; RUN: llvm-split -o %t_as_indirect %s -j 2 -mtriple amdgcn-amd-amdhsa -amdgpu-module-splitting-no-externalize-address-taken -amdgpu-module-splitting-inline-asm-is-indirect-call
+; RUN: llvm-dis -o - %t_as_indirect0 | FileCheck --check-prefix=CHECK-INDIRECT0 --implicit-check-not=define %s
+; RUN: llvm-dis -o - %t_as_indirect1 | FileCheck --check-prefix=CHECK-INDIRECT1 --implicit-check-not=define %s
+
+; CHECK0: define internal void @HelperB
+; CHECK0: define amdgpu_kernel void @B
+
+; CHECK1: define internal void @HelperA()
+; CHECK1: define amdgpu_kernel void @A()
+
+; CHECK-INDIRECT0: define internal void @HelperB
+; CHECK-INDIRECT0: define amdgpu_kernel void @B
+
+; CHECK-INDIRECT1: define internal void @HelperA()
+; CHECK-INDIRECT1: define internal void @HelperB()
+; CHECK-INDIRECT1: define amdgpu_kernel void @A()
+
+ at addrthief = global [2 x ptr] [ptr @HelperA, ptr @HelperB]
+
+define internal void @HelperA() {
+ ret void
+}
+
+define internal void @HelperB() {
+ ret void
+}
+
+define amdgpu_kernel void @A() {
+ call void asm sideeffect "v_mov_b32 v0, 7", "~{v0}"()
+ call void @HelperA()
+ ret void
+}
+
+define amdgpu_kernel void @B(ptr %out) {
+ call void @HelperB()
+ ret void
+}
diff --git a/llvm/test/tools/llvm-split/AMDGPU/kernels-alias-dependencies.ll b/llvm/test/tools/llvm-split/AMDGPU/kernels-alias-dependencies.ll
deleted file mode 100644
index d7e84abd5f968d..00000000000000
--- a/llvm/test/tools/llvm-split/AMDGPU/kernels-alias-dependencies.ll
+++ /dev/null
@@ -1,41 +0,0 @@
-; RUN: llvm-split -o %t %s -j 2 -mtriple amdgcn-amd-amdhsa
-; RUN: llvm-dis -o - %t0 | FileCheck --check-prefix=CHECK0 --implicit-check-not=define %s
-; RUN: llvm-dis -o - %t1 | FileCheck --check-prefix=CHECK1 --implicit-check-not=define %s
-
-; 3 kernels:
-; - A calls nothing
-; - B calls @PerryThePlatypus
-; - C calls @Perry, an alias of @PerryThePlatypus
-;
-; We should see through the alias and put B/C in the same
-; partition.
-;
-; Additionally, @PerryThePlatypus gets externalized as
-; the alias counts as taking its address.
-
-; CHECK0: define amdgpu_kernel void @A
-
-; CHECK1: @Perry = internal alias ptr (), ptr @PerryThePlatypus
-; CHECK1: define hidden void @PerryThePlatypus()
-; CHECK1: define amdgpu_kernel void @B
-; CHECK1: define amdgpu_kernel void @C
-
- at Perry = internal alias ptr(), ptr @PerryThePlatypus
-
-define internal void @PerryThePlatypus() {
- ret void
-}
-
-define amdgpu_kernel void @A() {
- ret void
-}
-
-define amdgpu_kernel void @B() {
- call void @PerryThePlatypus()
- ret void
-}
-
-define amdgpu_kernel void @C() {
- call void @Perry()
- ret void
-}
diff --git a/llvm/test/tools/llvm-split/AMDGPU/kernels-dependency-indirect.ll b/llvm/test/tools/llvm-split/AMDGPU/kernels-dependency-indirect.ll
index 5be945bda48bf4..c2acb06d3e72e5 100644
--- a/llvm/test/tools/llvm-split/AMDGPU/kernels-dependency-indirect.ll
+++ b/llvm/test/tools/llvm-split/AMDGPU/kernels-dependency-indirect.ll
@@ -3,18 +3,6 @@
; RUN: llvm-dis -o - %t1 | FileCheck --check-prefix=CHECK1 --implicit-check-not=define %s
; RUN: llvm-dis -o - %t2 | FileCheck --check-prefix=CHECK2 --implicit-check-not=define %s
-; We have 4 kernels:
-; - Each kernel has an internal helper
-; - @A and @B's helpers does an indirect call.
-;
-; We default to putting A/B in P0, alongside a copy
-; of all helpers who have their address taken.
-; The other kernels can still go into separate partitions.
-;
-; Note that dependency discovery shouldn't stop upon finding an
-; indirect call. HelperC/D should also end up in P0 as they
-; are dependencies of HelperB.
-
; CHECK0: define internal void @HelperD
; CHECK0: define amdgpu_kernel void @D
>From d5361ad4c4087241673758a96b1b99b0f886a8f0 Mon Sep 17 00:00:00 2001
From: pvanhout <pierre.vanhoutryve at amd.com>
Date: Wed, 25 Sep 2024 09:23:51 +0200
Subject: [PATCH 2/3] Comments + adapt inline asm test w/o CL opt
---
llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp | 19 ++++---------
.../AMDGPU/indirect-call-inline-asm-debug.ll | 28 +++++++++++++++++++
.../AMDGPU/indirect-call-inline-asm.ll | 11 --------
3 files changed, 34 insertions(+), 24 deletions(-)
create mode 100644 llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm-debug.ll
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
index e76fc937793669..78218167e50108 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
@@ -109,10 +109,6 @@ static cl::opt<bool> NoExternalizeOnAddrTaken(
cl::desc(
"disables externalization of functions whose addresses are taken"));
-static cl::opt<bool> InlineAsmIsIndirectCall(
- "amdgpu-module-splitting-inline-asm-is-indirect-call", cl::Hidden,
- cl::desc("consider inline assembly as an indirect call"));
-
static cl::opt<std::string>
ModuleDotCfgOutput("amdgpu-module-splitting-print-module-dotcfg",
cl::Hidden,
@@ -519,8 +515,9 @@ void SplitGraph::buildGraph(CallGraph &CG) {
// Keep track of this function if it contains an indirect call and/or if it
// can be indirectly called.
if (CallsExternal) {
- LLVM_DEBUG(dbgs() << " [!] callgraph is incomplete for " << Fn.getName()
- << " - analyzing function\n");
+ LLVM_DEBUG(dbgs() << " [!] callgraph is incomplete for ";
+ Fn.printAsOperand(dbgs());
+ dbgs() << " - analyzing function\n");
bool HasIndirectCall = false;
for (const auto &Inst : instructions(Fn)) {
@@ -530,8 +527,6 @@ void SplitGraph::buildGraph(CallGraph &CG) {
// inline assembly can be ignored, unless InlineAsmIsIndirectCall is
// true.
if (CB->isInlineAsm()) {
- if (InlineAsmIsIndirectCall)
- HasIndirectCall = true;
LLVM_DEBUG(dbgs() << " found inline assembly\n");
continue;
}
@@ -1372,11 +1367,9 @@ static void splitAMDGPUModule(
for (auto &Fn : M) {
// TODO: Should aliases count? Probably not but they're so rare I'm not
// sure it's worth fixing.
- if (Fn.hasAddressTaken()) {
- if (Fn.hasLocalLinkage()) {
- LLVM_DEBUG(dbgs() << "[externalize] " << Fn.getName()
- << " because its address is taken\n");
- }
+ if (Fn.hasLocalLinkage() && Fn.hasAddressTaken()) {
+ LLVM_DEBUG(dbgs() << "[externalize] "; Fn.printAsOperand(dbgs());
+ dbgs() << " because its address is taken\n");
externalize(Fn);
}
}
diff --git a/llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm-debug.ll b/llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm-debug.ll
new file mode 100644
index 00000000000000..5b15e740f76b96
--- /dev/null
+++ b/llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm-debug.ll
@@ -0,0 +1,28 @@
+; REQUIRES: asserts
+
+; RUN: llvm-split -o %t %s -j 2 -mtriple amdgcn-amd-amdhsa -amdgpu-module-splitting-no-externalize-address-taken -debug-only=amdgpu-split-module 2>&1 | FileCheck %s
+
+; CHECK: [!] callgraph is incomplete for ptr @A - analyzing function
+; CHECK-NEXT: found inline assembly
+; CHECK-NOT: indirect call found
+
+ at addrthief = global [2 x ptr] [ptr @HelperA, ptr @HelperB]
+
+define internal void @HelperA() {
+ ret void
+}
+
+define internal void @HelperB() {
+ ret void
+}
+
+define amdgpu_kernel void @A() {
+ call void asm sideeffect "v_mov_b32 v0, 7", "~{v0}"()
+ call void @HelperA()
+ ret void
+}
+
+define amdgpu_kernel void @B(ptr %out) {
+ call void @HelperB()
+ ret void
+}
diff --git a/llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm.ll b/llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm.ll
index 5c7ddaeecd7044..13c30c9e45e808 100644
--- a/llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm.ll
+++ b/llvm/test/tools/llvm-split/AMDGPU/indirect-call-inline-asm.ll
@@ -2,23 +2,12 @@
; RUN: llvm-dis -o - %t0 | FileCheck --check-prefix=CHECK0 --implicit-check-not=define %s
; RUN: llvm-dis -o - %t1 | FileCheck --check-prefix=CHECK1 --implicit-check-not=define %s
-; RUN: llvm-split -o %t_as_indirect %s -j 2 -mtriple amdgcn-amd-amdhsa -amdgpu-module-splitting-no-externalize-address-taken -amdgpu-module-splitting-inline-asm-is-indirect-call
-; RUN: llvm-dis -o - %t_as_indirect0 | FileCheck --check-prefix=CHECK-INDIRECT0 --implicit-check-not=define %s
-; RUN: llvm-dis -o - %t_as_indirect1 | FileCheck --check-prefix=CHECK-INDIRECT1 --implicit-check-not=define %s
-
; CHECK0: define internal void @HelperB
; CHECK0: define amdgpu_kernel void @B
; CHECK1: define internal void @HelperA()
; CHECK1: define amdgpu_kernel void @A()
-; CHECK-INDIRECT0: define internal void @HelperB
-; CHECK-INDIRECT0: define amdgpu_kernel void @B
-
-; CHECK-INDIRECT1: define internal void @HelperA()
-; CHECK-INDIRECT1: define internal void @HelperB()
-; CHECK-INDIRECT1: define amdgpu_kernel void @A()
-
@addrthief = global [2 x ptr] [ptr @HelperA, ptr @HelperB]
define internal void @HelperA() {
>From b5e322fed6fca2fb35eae63620f9fd4c60d8e369 Mon Sep 17 00:00:00 2001
From: pvanhout <pierre.vanhoutryve at amd.com>
Date: Mon, 7 Oct 2024 07:44:57 +0200
Subject: [PATCH 3/3] Add break
---
llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp | 1 +
1 file changed, 1 insertion(+)
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
index 78218167e50108..a62c72d124825e 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
@@ -533,6 +533,7 @@ void SplitGraph::buildGraph(CallGraph &CG) {
// everything else is handled conservatively.
HasIndirectCall = true;
+ break;
}
}
More information about the llvm-commits
mailing list