[AMDGPU][SplitModule] Handle !callees metadata #108802
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-backend-amdgpu Author: Pierre van Houtryve (Pierre-vh) ChangesSee #106528 to review the first commit. Handle the Full diff: https://github.com/llvm/llvm-project/pull/108802.diff 5 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp b/llvm/lib/Target/AMDGPU/AMDGPUSplitModule.cpp
index e97a7f4e075f7f..17bbce8c1c98de 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,
@@ -476,12 +486,36 @@ void SplitGraph::Node::visitAllDependencies(
}
}
+/// Checks if \p I has MD_callees and if it does, parse it and put the function
+/// in \p Callees.
+///
+/// \returns true if there was metadata and it was parsed correctly. false if
+/// there was no MD or if it contained unknown entries.
+static bool handleCalleesMD(const Instruction &I,
+ SmallVector<Function *> &Callees) {
+ auto *MD = I.getMetadata(LLVMContext::MD_callees);
+ if (!MD)
+ return false;
+
+ for (const auto &Op : MD->operands()) {
+ Function *Callee = mdconst::extract_or_null<Function>(Op);
+ if (!Callee)
+ return false;
+ Callees.push_back(Callee);
+ }
+
+ return true;
+}
+
void SplitGraph::buildGraph(CallGraph &CG) {
SplitModuleTimer SMT("buildGraph", "graph construction");
LLVM_DEBUG(
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 +526,58 @@ 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");
+
+ SmallVector<Function *> KnownCallees;
+
+ 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;
+ }
+
+ if (handleCalleesMD(Inst, KnownCallees))
+ continue;
+
+ // everything else is handled conservatively.
+ HasIndirectCall = true;
+ }
+ }
+
+ if (HasIndirectCall) {
+ LLVM_DEBUG(dbgs() << " indirect call found\n");
+ FnsWithIndirectCalls.push_back(&Fn);
+ } else if (!KnownCallees.empty())
+ DirectCallees.insert(KnownCallees.begin(), KnownCallees.end());
}
+ 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 +1389,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 +1441,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()
+
+@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
-
-@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-callee-md.ll b/llvm/test/tools/llvm-split/AMDGPU/kernels-dependency-indirect-callee-md.ll
new file mode 100644
index 00000000000000..f1ed02b2502a06
--- /dev/null
+++ b/llvm/test/tools/llvm-split/AMDGPU/kernels-dependency-indirect-callee-md.ll
@@ -0,0 +1,69 @@
+; RUN: sed -s 's/_MD_/, !callees !{ptr @CallCandidate0}/' %s | llvm-split -o %t -j 3 -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
+; RUN: llvm-dis -o - %t2 | FileCheck --check-prefix=CHECK2 --implicit-check-not=define %s
+
+; RUN: sed -s 's/_MD_//g' %s | llvm-split -o %t-nomd -j 3 -mtriple amdgcn-amd-amdhsa
+; RUN: llvm-dis -o - %t-nomd0 | FileCheck --check-prefix=CHECK-NOMD0 --implicit-check-not=define %s
+; RUN: llvm-dis -o - %t-nomd1 | FileCheck --check-prefix=CHECK-NOMD1 --implicit-check-not=define %s
+; RUN: llvm-dis -o - %t-nomd2 | FileCheck --check-prefix=CHECK-NOMD2 --implicit-check-not=define %s
+
+; CHECK0: define internal void @HelperC
+; CHECK0: define amdgpu_kernel void @C
+
+; CHECK1: define hidden void @CallCandidate1
+; CHECK1: define internal void @HelperB
+; CHECK1: define amdgpu_kernel void @B
+
+; CHECK2: define internal void @HelperA
+; CHECK2: define hidden void @CallCandidate0
+; CHECK2: define amdgpu_kernel void @A
+
+; CHECK-NOMD0: define internal void @HelperC
+; CHECK-NOMD0: define amdgpu_kernel void @C
+
+; CHECK-NOMD1: define internal void @HelperB
+; CHECK-NOMD1: define amdgpu_kernel void @B
+
+; CHECK-NOMD2: define internal void @HelperA
+; CHECK-NOMD2: define hidden void @CallCandidate0
+; CHECK-NOMD2: define hidden void @CallCandidate1
+; CHECK-NOMD2: define amdgpu_kernel void @A
+
+@addrthief = global [2 x ptr] [ptr @CallCandidate0, ptr @CallCandidate1]
+
+define internal void @HelperA(ptr %call) {
+ call void %call() _MD_
+ ret void
+}
+
+define internal void @CallCandidate0() {
+ ret void
+}
+
+define internal void @CallCandidate1() {
+ ret void
+}
+
+define internal void @HelperB() {
+ ret void
+}
+
+define internal void @HelperC() {
+ ret void
+}
+
+define amdgpu_kernel void @A(ptr %call) {
+ call void @HelperA(ptr %call)
+ ret void
+}
+
+define amdgpu_kernel void @B() {
+ call void @HelperB()
+ ret void
+}
+
+define amdgpu_kernel void @C() {
+ call void @HelperC()
+ 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
|
jmmartinez
reviewed
Sep 16, 2024
shiltian
reviewed
Sep 16, 2024
| } | ||
|
|
||
| Node &N = getNode(Cache, Fn); | ||
| for (const auto *Callee : DirectCallees) | ||
| createEdge(N, getNode(Cache, *Callee), EdgeKind::DirectCall); |
There was a problem hiding this comment.
I have a question (probably has been asked by someone before IIRC). I thought when constructing the call graph, it already takes all potential callees into consideration such that the call edges are already there. It seems like that's not the case.
There was a problem hiding this comment.
These are edges in the SplitGraph node which are entirely different.
The SplitGraph has Edge objects to model those edges while the CG doesn't
jmmartinez
approved these changes
Sep 20, 2024
arsenm
reviewed
Sep 24, 2024
Comment on lines
+567
to
+563
| // If we failed to parse any !callees MD, or some was missing, | ||
| // the entire KnownCallees list is now unreliable. |
There was a problem hiding this comment.
Sounds like the verifier is missing validation on callees entries? You shouldn't really have to worry about parsing it
Pierre-vh
force-pushed
the
callee-md-handling
branch
from
6a4b114 to
76fbf52
Compare
arsenm
reviewed
Oct 9, 2024
| /// there was no MD or if it contained unknown entries and parsing failed. | ||
| /// If this returns false, \p Callees will contain incomplete information | ||
| /// and must not be used. | ||
| static bool handleCalleesMD(const Instruction &I, |
There was a problem hiding this comment.
Still think the call graph analysis should be handling this
There was a problem hiding this comment.
Can it land as is or do you want me to try changing the CG first?
I'm not sure if that'd need a RFC
There was a problem hiding this comment.
Can start with it here, but I've always found it to be a flaw. It's also busted for the CGInSCC order with aliases (which I guess we're now less dependent on)
Pierre-vh
force-pushed
the
callee-md-handling
branch
from
76fbf52 to
fbfdebc
Compare
arsenm
approved these changes
Oct 11, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/3444 Here is the relevant piece of the build log for the reference |
|
Looks like this breaks tests on macOS: http://45.33.8.238/macm1/93862/step_10.txt Please take a look and revert for now if it takes a while to fix. |
nico
added a commit
that referenced
this pull request
Oct 14, 2024
|
Reverted in 140cbca for now. |
Pierre-vh
added a commit
that referenced
this pull request
Oct 15, 2024
(reland with fixed sed command for macos) Handle the `!callees` metadata to further reduce the amount of indirect call cases that end up conservatively assuming that any indirectly callable function is a potential target.
|
relanded as b3a8400 |
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
See llvm#106528 to review the first commit. Handle the `!callees` metadata to further reduce the amount of indirect call cases that end up conservatively assuming that any indirectly callable function is a potential target.
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
This reverts commit 4a0dc3e. Breaks tests, see comments on llvm#108802
DanielCChen
pushed a commit
to DanielCChen/llvm-project
that referenced
this pull request
Oct 16, 2024
(reland with fixed sed command for macos) Handle the `!callees` metadata to further reduce the amount of indirect call cases that end up conservatively assuming that any indirectly callable function is a potential target.
searlmc1
pushed a commit
to ROCm/llvm-project
that referenced
this pull request
Dec 5, 2024
(reland with fixed sed command for macos) Handle the `!callees` metadata to further reduce the amount of indirect call cases that end up conservatively assuming that any indirectly callable function is a potential target. Change-Id: Ibe9d80ca623ee79bec7ee0e1128a7535b9e64f55
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #106528 to review the first commit.
Handle the
!calleesmetadata to further reduce the amount of indirect call cases that end up conservatively assuming that any indirectly callable function is a potential target.