-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[Transforms][IPO] Add func suffix in ArgumentPromotion and DeadArgume… #105742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-llvm-transforms Author: None (yonghong-song) Changes…ntElimination ArgumentPromotion and DeadArgumentElimination passes could change function signatures but the function name remains the same as before the transformation. This makes it hard for tracing with bpf programs where user tends to use function signature in the source. See discussion [1] for details. This patch added suffix to functions whose signatures are changed. The suffix lets users know that function signature has changed and they need to impact the IR or binary to find modified signature before tracing those functions. The suffix for ArgumentPromotion is ".argpromotion" and the suffix for DeadArgumentElimination is ".deadargelim". The suffix also gives user hints about what kind of transformation has been done. With this patch, I built a recent linux kernel with full LTO enabled. I got 4 functions with only argpromotion like
I got 1058 functions with only deadargelim like
I got 3 functions with both argpromotion and deadargelim
[1] #104678 Full diff: https://github.com/llvm/llvm-project/pull/105742.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
index 452fff7898d0ea..f3cbf15b46a09c 100644
--- a/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
+++ b/llvm/lib/Transforms/IPO/ArgumentPromotion.cpp
@@ -200,6 +200,7 @@ doPromotion(Function *F, FunctionAnalysisManager &FAM,
F->getParent()->getFunctionList().insert(F->getIterator(), NF);
NF->takeName(F);
+ NF->setName(NF->getName() + ".argpromotion");
// Loop over all the callers of the function, transforming the call sites to
// pass in the loaded pointers.
diff --git a/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp b/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
index f5a7ab26a49e96..b54d3b3fd7ad0d 100644
--- a/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
+++ b/llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp
@@ -876,6 +876,7 @@ bool DeadArgumentEliminationPass::removeDeadStuffFromFunction(Function *F) {
// it again.
F->getParent()->getFunctionList().insert(F->getIterator(), NF);
NF->takeName(F);
+ NF->setName(NF->getName() + ".deadargelim");
NF->IsNewDbgInfoFormat = F->IsNewDbgInfoFormat;
// Loop over all the callers of the function, transforming the call sites to
|
@@ -200,6 +200,7 @@ doPromotion(Function *F, FunctionAnalysisManager &FAM, | |||
|
|||
F->getParent()->getFunctionList().insert(F->getIterator(), NF); | |||
NF->takeName(F); | |||
NF->setName(NF->getName() + ".argpromotion"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bikeshedding: '.argpromo' ?
@@ -876,6 +876,7 @@ bool DeadArgumentEliminationPass::removeDeadStuffFromFunction(Function *F) { | |||
// it again. | |||
F->getParent()->getFunctionList().insert(F->getIterator(), NF); | |||
NF->takeName(F); | |||
NF->setName(NF->getName() + ".deadargelim"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another bikeshed: '.argelim' ?
Yes. the suffix is just a placeholder. I do not have a good idea about what suffix should use. Should we use the same suffix as gcc (constprop, isra) or llvm specific one? Any suggestion will be good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No test cases need to be updated? The naming convention may be depended upon later, so better cover it with a test case.
I ran through both clang and llvm test. All tests passed. I can add a selftest shortly. But it would be good to get some suggestions about what suffix we should use. |
argelim and argprom sound good to me. |
Sorry, it is my fault. Somehow I may run with a different llvm build (I have one directly from upstream and another is forked), so the test failure is not captured. You are right, quite some tests need an update. Will update with new suffix and and make change to those failed tests. |
9500f42
to
610ab1f
Compare
@@ -200,6 +200,7 @@ doPromotion(Function *F, FunctionAnalysisManager &FAM, | |||
|
|||
F->getParent()->getFunctionList().insert(F->getIterator(), NF); | |||
NF->takeName(F); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following is takeName() implementation:
void Value::takeName(Value *V) {
assert(V != this && "Illegal call to this->takeName(this)!");
ValueSymbolTable *ST = nullptr;
// If this value has a name, drop it.
if (hasName()) {
// Get the symtab this is in.
if (getSymTab(this, ST)) {
// We can't set a name on this value, but we need to clear V's name if
// it has one.
if (V->hasName()) V->setName("");
return; // Cannot set a name on this value (e.g. constant).
}
// Remove old name.
if (ST)
ST->removeValueName(getValueName());
destroyValueName();
}
// Now we know that this has no name.
// If V has no name either, we're done.
if (!V->hasName()) return;
// Get this's symtab if we didn't before.
if (!ST) {
if (getSymTab(this, ST)) {
// Clear V's name.
V->setName("");
return; // Cannot set a name on this value (e.g. constant).
}
}
// Get V's ST, this should always succeed, because V has a name.
ValueSymbolTable *VST;
bool Failure = getSymTab(V, VST);
assert(!Failure && "V has a name, so it should have a ST!"); (void)Failure;
// If these values are both in the same symtab, we can do this very fast.
// This works even if both values have no symtab yet.
if (ST == VST) {
// Take the name!
setValueName(V->getValueName());
V->setValueName(nullptr);
getValueName()->setValue(this);
return;
}
// Otherwise, things are slightly more complex. Remove V's name from VST and
// then reinsert it into ST.
if (VST)
VST->removeValueName(V->getValueName());
setValueName(V->getValueName());
V->setValueName(nullptr);
getValueName()->setValue(this);
if (ST)
ST->reinsertValue(this);
}
I think this is needed since it is a little bit complicated e.g. checking duplicated symbol etc. NF->takeName(F) provides the new func name and we just need to add suffix on top of that.
@@ -61,7 +61,7 @@ | |||
; RUN: -o %t.out 2>&1 | FileCheck %s --check-prefix=DUMP \ | |||
; RUN: --check-prefix=STATS --check-prefix=STATS-BE --check-prefix=REMARKS | |||
|
|||
; RUN: llvm-dis %t.out.1.4.opt.bc -o - | FileCheck %s --check-prefix=IR | |||
; RUN: llvm-dis %t.out.1.4.opt.bc -o - | FileCheck %s --check-prefix=IRNODIST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this change about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this change about?
In original memprof-funcassigncloning.ll, there are two places check prefix IR are used:
52 ; RUN: opt -thinlto-bc %s >%t.o
53 ; RUN: llvm-lto2 run %t.o -enable-memprof-context-disambiguation \
54 ; RUN: -supports-hot-cold-new \
55 ; RUN: -r=%t.o,main,plx \
56 ; RUN: -r=%t.o,_ZdaPv, \
57 ; RUN: -r=%t.o,sleep, \
58 ; RUN: -r=%t.o,_Znam, \
59 ; RUN: -memprof-verify-ccg -memprof-verify-nodes -memprof-dump-ccg \
60 ; RUN: -stats -pass-remarks=memprof-context-disambiguation -save-temps \
61 ; RUN: -o %t.out 2>&1 | FileCheck %s --check-prefix=DUMP \
62 ; RUN: --check-prefix=STATS --check-prefix=STATS-BE --check-prefix=REMARKS
63
**64 ; RUN: llvm-dis %t.out.1.4.opt.bc -o - | FileCheck %s --check-prefix=IR**
65
66
67 ;; Try again but with distributed ThinLTO
68 ; RUN: llvm-lto2 run %t.o -enable-memprof-context-disambiguation \
69 ; RUN: -supports-hot-cold-new \
70 ; RUN: -thinlto-distributed-indexes \
71 ; RUN: -r=%t.o,main,plx \
72 ; RUN: -r=%t.o,_ZdaPv, \
73 ; RUN: -r=%t.o,sleep, \
74 ; RUN: -r=%t.o,_Znam, \
75 ; RUN: -memprof-verify-ccg -memprof-verify-nodes -memprof-dump-ccg \
76 ; RUN: -stats -pass-remarks=memprof-context-disambiguation \
77 ; RUN: -o %t2.out 2>&1 | FileCheck %s --check-prefix=DUMP \
78 ; RUN: --check-prefix=STATS
79
80 ;; Run ThinLTO backend
81 ; RUN: opt -passes=memprof-context-disambiguation \
82 ; RUN: -memprof-import-summary=%t.o.thinlto.bc \
83 ; RUN: -stats -pass-remarks=memprof-context-disambiguation \
**84 ; RUN: %t.o -S 2>&1 | FileCheck %s --check-prefix=IR \**
85 ; RUN: --check-prefix=STATS-BE --check-prefix=REMARKS
With added suffix, the function signature will change for one 'IR' check but not another. That is why I renamed one IR to IRNODIST where the IRNODIST flavor has func signature change to minimize the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why name change does not happen with distributed thinLTO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why name change does not happen with distributed thinLTO?
This is due to the following code in llvm/tools/llvm-lto2/llvm-lto2.cpp:
if (ThinLTODistributedIndexes)
Backend = createWriteIndexesThinBackend(/*OldPrefix=*/"",
/*NewPrefix=*/"",
/*NativeObjectPrefix=*/"",
ThinLTOEmitImports,
/*LinkedObjectsFile=*/nullptr,
/*OnWrite=*/{});
else
Backend = createInProcessThinBackend(
llvm::heavyweight_hardware_concurrency(Threads),
/* OnWrite */ {}, ThinLTOEmitIndexes, ThinLTOEmitImports);
If ThinLTODistributedIndexes is true, createWriteIndexesThinBackend() is called which did not trigger DeadArgElimination pass.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The general idea of changing the name seems reasonable; in these cases, we know the program doesn't actually depend on the name of the symbol, so the name is just for the sake of debugging.
Should we try to prevent accumulating more than one suffix on a function?
If the name is using C++ mangling, does the demangler produce something reasonable? If not, can we modify the name in a demangler-friendly way?
If you try to set a breakpoint on a function, how will a debugger react to the changed name?
@@ -876,6 +876,8 @@ bool DeadArgumentEliminationPass::removeDeadStuffFromFunction(Function *F) { | |||
// it again. | |||
F->getParent()->getFunctionList().insert(F->getIterator(), NF); | |||
NF->takeName(F); | |||
if (NumArgumentsEliminated) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What cases are we excluding with the "if" here? If we don't change the signature, we don't recreate the function in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What cases are we excluding with the "if" here? If we don't change the signature, we don't recreate the function in the first place.
IIUC, there is another metric NumRetValsEliminated. It is possible that NumRetValsEliminated > 0 and NumArgumentsEliminated = 0. I added the above check since we really care function arguments in bpf tracing and func return value is not that important. But I can remove the above check so we do not limit the case only to bpf tracing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can imagine wanting a different suffix for the case where we only eliminate a return value, but I think it makes sense to have some suffix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I will remove the check and use '.retelim' suffix for cases where only the return value is eliminated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can imagine wanting a different suffix for the case where we only eliminate a return value, but I think it makes sense to have some suffix.
Just pushed a new revision to introduce '.retelim' suffix for cases where the sole func signature change is to remove return values.
There are two test failures:
I will fix it in the next revision. |
We probably should. Currently, I have seen suffix like .argprom.argprom, .argelim.argprom and specialized.1.argelim. Multiple suffix are totally possible here and could be longer in special cases. So it is indeed a good idea to shorten
I am not sure about this as my context is linux kernel which is C based. How do I know demangler produce something reasonable?
Again, good question. But gcc already have functions like .constprop., .isra.. Do gdb/lldb handle those functions? |
Take a small C++ function that gets optimized, run the resulting symbol through c++filt, see what you get.
I think maybe we end up doing a wildcard search that finds them? Not sure. |
Instead of creating a C++ function to trigger desired optimization, I actually tried to build clang itself with gcc (11.4.1) and clang (with adding func suffix as in this pull request). The following are two examples, a symbol generated from gcc and another symbol generated from clang with adding func suffix:
So I think we should be okay for C++ symbols. The suffix itself seems also reasonable, short enough to explain what transformation it has done.
I tried an example on gdb when symbol has a gcc suffix.
With gdb,
So in gdb, you can provide the final symbol name in symbol table, or you can provide the original source-code level symbol name and gdb seems trying to find it with '.<...>' suffix as you suggested. lldb seems able to do the same thing as well.
So I think newly added suffix feature should work for gdb/lldb as well. Note that llvm already has some suffix likes |
6fa9936
to
af691b9
Compare
…ntElimination ArgumentPromotion and DeadArgumentElimination passes could change function signatures but the function name remains the same as before the transformation. This makes it hard for tracing with bpf programs where user tends to use function signature in the source. See discussion [1] for details. This patch added suffix to functions whose signatures are changed. The suffix lets users know that function signature has changed and they need to impact the IR or binary to find modified signature before tracing those functions. The suffix for ArgumentPromotion is ".argprom" and the suffix for DeadArgumentElimination is ".argelim". The suffix also gives user hints about what kind of transformation has been done. With this patch, I built a recent linux kernel with full LTO enabled. I got 4 functions with only argpromotion like set_track_update.argelim.argprom pmd_trans_huge_lock.argprom ... I got 1058 functions with only deadargelim like process_bit0.argelim pci_io_ecs_init.argelim ... I got 3 functions with both argpromotion and deadargelim set_track_update.argelim.argprom zero_pud_populate.argelim.argprom zero_pmd_populate.argelim.argprom [1] llvm#104678
Fixing the following test failures: LLVM :: CodeGen/AArch64/arg_promotion.ll LLVM :: CodeGen/AMDGPU/internalize.ll
Also fixed a few tests due to rebase on top of main branch.
af691b9
to
3cfffd2
Compare
rebased on top of latest master and fixed a few more tests due to latest change. |
@efriedma-quic Any further comments on this pull request? |
@efriedma-quic ping again. Any comments about how to proceed with this patch? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/9389 Here is the relevant piece of the build log for the reference
|
…adArgume… (#105742)" This reverts commit 959448f. Reverting because multiple test failures e.g. https://lab.llvm.org/buildbot/#/builders/187/builds/1290 https://lab.llvm.org/buildbot/#/builders/153/builds/9389 and maybe a few others.
llvm#105742) …ntElimination ArgumentPromotion and DeadArgumentElimination passes could change function signatures but the function name remains the same as before the transformation. This makes it hard for tracing with bpf programs where user tends to use function signature in the source. See discussion [1] for details. This patch added suffix to functions whose signatures are changed. The suffix lets users know that function signature has changed and they need to impact the IR or binary to find modified signature before tracing those functions. The suffix for ArgumentPromotion is ".argprom" and the suffixes for DeadArgumentElimination are ".argelim" and ".retelim". The suffix also gives user hints about what kind of transformation has been done. With this patch, I built a recent linux kernel with full LTO enabled. I got 4 functions with only argpromotion like ``` set_track_update.argelim.argprom pmd_trans_huge_lock.argprom ... ``` I got 1058 functions with only deadargelim like ``` process_bit0.argelim pci_io_ecs_init.argelim ... ``` I got 3 functions with both argpromotion and deadargelim ``` set_track_update.argelim.argprom zero_pud_populate.argelim.argprom zero_pmd_populate.argelim.argprom ``` [1] llvm#104678
…adArgume… (llvm#105742)" This reverts commit 959448f. Reverting because multiple test failures e.g. https://lab.llvm.org/buildbot/#/builders/187/builds/1290 https://lab.llvm.org/buildbot/#/builders/153/builds/9389 and maybe a few others.
…ntElimination
ArgumentPromotion and DeadArgumentElimination passes could change function signatures but the function name remains the same as before the transformation. This makes it hard for tracing with bpf programs where user tends to use function signature in the source. See discussion [1] for details.
This patch added suffix to functions whose signatures are changed. The suffix lets users know that function signature has changed and they need to impact the IR or binary to find modified signature before tracing those functions.
The suffix for ArgumentPromotion is ".argprom" and the suffixes for DeadArgumentElimination are ".argelim" and ".retelim". The suffix also gives user hints about what kind of transformation has been done.
With this patch, I built a recent linux kernel with full LTO enabled. I got 4 functions with only argpromotion like
I got 1058 functions with only deadargelim like
I got 3 functions with both argpromotion and deadargelim
[1] #104678