-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -889,6 +889,10 @@ 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
Just pushed a new revision to introduce '.retelim' suffix for cases where the sole func signature change is to remove return values. |
||
NF->setName(NF->getName() + ".argelim"); | ||
else | ||
NF->setName(NF->getName() + ".retelim"); | ||
NF->IsNewDbgInfoFormat = F->IsNewDbgInfoFormat; | ||
|
||
// Loop over all the callers of the function, transforming the call sites to | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
In original memprof-funcassigncloning.ll, there are two places check prefix IR are used:
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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more.
This is due to the following code in llvm/tools/llvm-lto2/llvm-lto2.cpp:
If ThinLTODistributedIndexes is true, createWriteIndexesThinBackend() is called which did not trigger DeadArgElimination pass. |
||
|
||
|
||
;; Try again but with distributed ThinLTO | ||
|
@@ -283,6 +283,23 @@ attributes #0 = { noinline optnone } | |
; IR: attributes #[[NOTCOLD]] = { "memprof"="notcold" } | ||
; IR: attributes #[[COLD]] = { "memprof"="cold" } | ||
|
||
; IRNODIST: define internal {{.*}} @_Z1EPPcS0_.argelim( | ||
; IRNODIST: call {{.*}} @_Znam(i64 noundef 10) #[[NOTCOLD:[0-9]+]] | ||
; IRNODIST: call {{.*}} @_Znam(i64 noundef 10) #[[NOTCOLD]] | ||
; IRNODIST: define internal {{.*}} @_Z1BPPcS0_( | ||
; IRNODIST: call {{.*}} @_Z1EPPcS0_.argelim( | ||
; IRNODIST: define internal {{.*}} @_Z1CPPcS0_( | ||
; IRNODIST: call {{.*}} @_Z1EPPcS0_.memprof.3.argelim( | ||
; IRNODIST: define internal {{.*}} @_Z1DPPcS0_( | ||
; IRNODIST: call {{.*}} @_Z1EPPcS0_.memprof.2.argelim( | ||
; IRNODIST: define internal {{.*}} @_Z1EPPcS0_.memprof.2.argelim( | ||
; IRNODIST: call {{.*}} @_Znam(i64 noundef 10) #[[COLD:[0-9]+]] | ||
; IRNODIST: call {{.*}} @_Znam(i64 noundef 10) #[[NOTCOLD]] | ||
; IRNODIST: define internal {{.*}} @_Z1EPPcS0_.memprof.3.argelim( | ||
; IRNODIST: call {{.*}} @_Znam(i64 noundef 10) #[[NOTCOLD]] | ||
; IRNODIST: call {{.*}} @_Znam(i64 noundef 10) #[[COLD]] | ||
; IRNODIST: attributes #[[NOTCOLD]] = { "memprof"="notcold" } | ||
; IRNODIST: attributes #[[COLD]] = { "memprof"="cold" } | ||
|
||
; STATS: 2 memprof-context-disambiguation - Number of cold static allocations (possibly cloned) | ||
; STATS-BE: 2 memprof-context-disambiguation - Number of cold static allocations (possibly cloned) during ThinLTO backend | ||
|
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:
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.