[Transforms][IPO] Add func suffix in ArgumentPromotion and DeadArgume… #105742

yonghong-song · 2024-08-22T21:58:37Z

…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] #104678

llvmbot · 2024-08-22T21:59:11Z

@llvm/pr-subscribers-backend-amdgpu
@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-lto
@llvm/pr-subscribers-function-specialization

@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

  set_track_update.deadargelim.argpromotion
  pmd_trans_huge_lock.argpromotion
  ...

I got 1058 functions with only deadargelim like

  process_bit0.deadargelim
  pci_io_ecs_init.deadargelim
  ...

I got 3 functions with both argpromotion and deadargelim

  set_track_update.deadargelim.argpromotion
  zero_pud_populate.deadargelim.argpromotion
  zero_pmd_populate.deadargelim.argpromotion

[1] #104678

Full diff: https://github.com/llvm/llvm-project/pull/105742.diff

2 Files Affected:

(modified) llvm/lib/Transforms/IPO/ArgumentPromotion.cpp (+1)
(modified) llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp (+1)

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

yonghong-song · 2024-08-22T21:59:16Z

cc @4ast @eddyz87 @anakryiko

anakryiko · 2024-08-22T22:03:47Z

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");


bikeshedding: '.argpromo' ?

anakryiko · 2024-08-22T22:03:58Z

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");


another bikeshed: '.argelim' ?

yonghong-song · 2024-08-22T22:16:59Z

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.

david-xl

No test cases need to be updated? The naming convention may be depended upon later, so better cover it with a test case.

yonghong-song · 2024-08-22T22:20:47Z

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.

david-xl · 2024-08-22T22:36:40Z

argelim and argprom sound good to me.

yonghong-song · 2024-08-23T03:58:43Z

No test cases need to be updated? The naming convention may be depended upon later, so better cover it with a test case.

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.

david-xl · 2024-08-23T17:49:43Z

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);


Is this needed?

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.

david-xl · 2024-08-23T17:52:55Z

llvm/test/ThinLTO/X86/memprof-funcassigncloning.ll

@@ -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


what is this change about?

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.

Why name change does not happen with distributed thinLTO?

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.

efriedma-quic

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?

efriedma-quic · 2024-08-23T21:13:32Z

llvm/lib/Transforms/IPO/DeadArgumentElimination.cpp

@@ -876,6 +876,8 @@ bool DeadArgumentEliminationPass::removeDeadStuffFromFunction(Function *F) {
  // it again.
  F->getParent()->getFunctionList().insert(F->getIterator(), NF);
  NF->takeName(F);
+  if (NumArgumentsEliminated)


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.

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.

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.

Okay, I will remove the check and use '.retelim' suffix for cases where only the return value is eliminated.

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.

yonghong-song · 2024-08-24T00:03:41Z

There are two test failures:

 LLVM :: CodeGen/AArch64/arg_promotion.ll
  LLVM :: CodeGen/AMDGPU/internalize.ll

I will fix it in the next revision.

yonghong-song · 2024-08-24T02:31:24Z

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?

We probably should. Currently, I have seen suffix like .argprom.argprom, .argelim.argprom and specialized.1.argelim.
.argprom.argprom is from llvm unit test. .argelim.argprom is from linux kernel O3 build.
.specialized.1.argelim is from linux kernel FULL LTO build.

Multiple suffix are totally possible here and could be longer in special cases. So it is indeed a good idea to shorten
the suffix. Maybe we could just one suffix and ignore the rest? For example, for '.argprom.argprom', we just use '.argprom'. For 'argelim.argprom', we just use '.argelim'? Form '.specialized.1.argelim', juse use '.specialized.1'?

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?

I am not sure about this as my context is linux kernel which is C based. How do I know demangler produce something reasonable?

If you try to set a breakpoint on a function, how will a debugger react to the changed name?

Again, good question. But gcc already have functions like .constprop., .isra.. Do gdb/lldb handle those functions?

efriedma-quic · 2024-08-27T08:00:36Z

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?
I am not sure about this as my context is linux kernel which is C based. How do I know demangler produce something reasonable?

Take a small C++ function that gets optimized, run the resulting symbol through c++filt, see what you get.

If you try to set a breakpoint on a function, how will a debugger react to the changed name?
Again, good question. But gcc already have functions like .constprop., .isra.. Do gdb/lldb handle those functions?

I think maybe we end up doing a wildcard search that finds them? Not sure.

yonghong-song · 2024-08-29T18:40:29Z

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?
I am not sure about this as my context is linux kernel which is C based. How do I know demangler produce something reasonable?

Take a small C++ function that gets optimized, run the resulting symbol through c++filt, see what you get.

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:

$ cat t
_ZN5clang19RecursiveASTVisitorIN12_GLOBAL__N_119PluralMisuseChecker13MethodCrawlerEE14TraverseIfStmtEPNS_6IfStmtEPN4llvm15SmallVectorImplINS7_14PointerIntPairIPNS_4StmtELj1EbNS7_21PointerLikeTypeTraitsISB_EENS7_18PointerIntPairInfoISB_Lj1ESD_EEEEEE.part.0.constprop.0.isra.0

_ZZNK4llvm20AMDGPUTargetLowering29isDesirableToCommuteWithShiftEPKNS_6SDNodeENS_12CombineLevelEENK3$_0clENS_7SDValueES6_.argprom.argelim
$ c++filt < t
clang::RecursiveASTVisitor<(anonymous namespace)::PluralMisuseChecker::MethodCrawler>::TraverseIfStmt(clang::IfStmt*, llvm::SmallVectorImpl<llvm::PointerIntPair<clang::Stmt*, 1u, bool, llvm::PointerLikeTypeTraits<clang::Stmt*>, llvm::PointerIntPairInfo<clang::Stmt*, 1u, llvm::PointerLikeTypeTraits<clang::Stmt*> > > >*) [clone .part.0] [clone .constprop.0] [clone .isra.0]

llvm::AMDGPUTargetLowering::isDesirableToCommuteWithShift(llvm::SDNode const*, llvm::CombineLevel) const::$_0::operator()(llvm::SDValue, llvm::SDValue) const [clone .argprom] [clone .argelim]

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.

If you try to set a breakpoint on a function, how will a debugger react to the changed name?
Again, good question. But gcc already have functions like .constprop., .isra.. Do gdb/lldb handle those functions?

I think maybe we end up doing a wildcard search that finds them? Not sure.

I tried an example on gdb when symbol has a gcc suffix.

[~/tmp5]$ cat main.c
extern int g(int);
int main(void) {
  return g(5);
}
[~/tmp5]$ vi test.c
[~/tmp5]$ cat test.c
__attribute__((noinline)) static int f(int a, int* b) { return a + *b; }
int g(int x) { return f(x + 1, &x); }
[~/tmp5]$ cat main.c
extern int g(int);
int main(void) {
  return g(5);
}
$ gcc -O2 -g test.c main.c && llvm-readelf -s a.out | grep f | grep isra
    47: 0000000000401120     4 FUNC    LOCAL  DEFAULT    12 f.isra.0

With gdb,

(gdb) break f.isra.0
Breakpoint 1 at 0x401120: file test.c, line 1.
(gdb) run
Starting program: /home/yhs/tmp5/a.out 

Breakpoint 1, f (a=6, b=<optimized out>, b=<optimized out>) at test.c:1
1       __attribute__((noinline)) static int f(int a, int* b) { return a + *b; }
(gdb) disassemble 
Dump of assembler code for function f:
=> 0x0000000000401120 <+0>:     lea    (%rdi,%rsi,1),%eax
   0x0000000000401123 <+3>:     retq   
End of assembler dump.
(gdb) s
0x00007ffff7c29590 in __libc_start_call_main () from /lib64/libc.so.6
(gdb) s
Single stepping until exit from function __libc_start_call_main,
which has no line number information.
[Inferior 1 (process 4133944) exited with code 013]
(gdb) 
The program is not being run.
(gdb) del 1
(gdb) break f
Breakpoint 2 at 0x401120: file test.c, line 1.
(gdb) run
Starting program: /home/yhs/tmp5/a.out

Breakpoint 2, f (a=6, b=<optimized out>, b=<optimized out>) at test.c:1
1       __attribute__((noinline)) static int f(int a, int* b) { return a + *b; }
(gdb) disassemble
Dump of assembler code for function f:
=> 0x0000000000401120 <+0>:     lea    (%rdi,%rsi,1),%eax
   0x0000000000401123 <+3>:     retq   
End of assembler dump.
(gdb) s
0x00007ffff7c29590 in __libc_start_call_main () from /lib64/libc.so.6
(gdb) s
Single stepping until exit from function __libc_start_call_main,
which has no line number information.
[Inferior 1 (process 4136424) exited with code 013]

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.

(lldb) b f
Breakpoint 1: where = a.out`f at test.c:1:66, address = 0x0000000000401120
(lldb) b f.isra.0
Breakpoint 2: where = a.out`f at test.c:1:66, address = 0x0000000000401120

So I think newly added suffix feature should work for gdb/lldb as well. Note that llvm already has some suffix likes
<...>.llvm. for thinlto, <...>. for fulllto, <...>.specialized. etc.

…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.

yonghong-song · 2024-09-02T15:56:39Z

rebased on top of latest master and fixed a few more tests due to latest change.

yonghong-song · 2024-09-05T17:28:55Z

@efriedma-quic Any further comments on this pull request?

yonghong-song · 2024-09-11T15:38:30Z

@efriedma-quic ping again. Any comments about how to proceed with this patch?

efriedma-quic

LGTM

llvm-ci · 2024-09-19T09:42:15Z

LLVM Buildbot has detected a new failure on builder premerge-monolithic-linux running on premerge-linux-1 while building llvm at step 7 "test-build-unified-tree-check-all".

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

Step 7 (test-build-unified-tree-check-all) failure: test (failure)
******************** TEST 'cfi-standalone-lld-thinlto-x86_64 :: stats.cpp' FAILED ********************
Exit Code: 1

Command Output (stderr):
--
RUN: at line 1: /build/buildbot/premerge-monolithic-linux/build/./bin/clang   -m64 -fuse-ld=lld -flto=thin  -fsanitize=cfi --driver-mode=g++ -fvisibility=hidden -gline-tables-only -fsanitize-stats -o /build/buildbot/premerge-monolithic-linux/build/runtimes/runtimes-bins/compiler-rt/test/cfi/Standalone-lld-thinlto-x86_64/Output/stats.cpp.tmp /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp
+ /build/buildbot/premerge-monolithic-linux/build/./bin/clang -m64 -fuse-ld=lld -flto=thin -fsanitize=cfi --driver-mode=g++ -fvisibility=hidden -gline-tables-only -fsanitize-stats -o /build/buildbot/premerge-monolithic-linux/build/runtimes/runtimes-bins/compiler-rt/test/cfi/Standalone-lld-thinlto-x86_64/Output/stats.cpp.tmp /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp
RUN: at line 2: env SANITIZER_STATS_PATH=/build/buildbot/premerge-monolithic-linux/build/runtimes/runtimes-bins/compiler-rt/test/cfi/Standalone-lld-thinlto-x86_64/Output/stats.cpp.tmp.stats  /build/buildbot/premerge-monolithic-linux/build/runtimes/runtimes-bins/compiler-rt/test/cfi/Standalone-lld-thinlto-x86_64/Output/stats.cpp.tmp
+ env SANITIZER_STATS_PATH=/build/buildbot/premerge-monolithic-linux/build/runtimes/runtimes-bins/compiler-rt/test/cfi/Standalone-lld-thinlto-x86_64/Output/stats.cpp.tmp.stats /build/buildbot/premerge-monolithic-linux/build/runtimes/runtimes-bins/compiler-rt/test/cfi/Standalone-lld-thinlto-x86_64/Output/stats.cpp.tmp
RUN: at line 3: sanstats /build/buildbot/premerge-monolithic-linux/build/runtimes/runtimes-bins/compiler-rt/test/cfi/Standalone-lld-thinlto-x86_64/Output/stats.cpp.tmp.stats | FileCheck /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp
+ sanstats /build/buildbot/premerge-monolithic-linux/build/runtimes/runtimes-bins/compiler-rt/test/cfi/Standalone-lld-thinlto-x86_64/Output/stats.cpp.tmp.stats
+ FileCheck /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp
/build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp:29:12: error: CHECK: expected string not found in input
 // CHECK: stats.cpp:[[@LINE+1]] {{_?}}dcast cfi-derived-cast 24
           ^
<stdin>:2:129: note: scanning from here
0x0000000000022a55 /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp:25 nvcall cfi-nvcall 51
                                                                                                                                ^
<stdin>:2:129: note: with "@LINE+1" equal to "30"
0x0000000000022a55 /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp:25 nvcall cfi-nvcall 51
                                                                                                                                ^
<stdin>:3:109: note: possible intended match here
0x0000000000022ab6 /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp:30 dcast.retelim cfi-derived-cast 24
                                                                                                            ^

Input file: <stdin>
Check file: /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp

-dump-input=help explains the following input dump.

Input was:
<<<<<<
            1: 0x0000000000022a05 /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp:20 vcall cfi-vcall 37 
            2: 0x0000000000022a55 /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp:25 nvcall cfi-nvcall 51 
check:29'0                                                                                                                                     X error: no match found
check:29'1                                                                                                                                       with "@LINE+1" equal to "30"
            3: 0x0000000000022ab6 /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp:30 dcast.retelim cfi-derived-cast 24 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:29'2                                                                                                                 ?                                  possible intended match
            4: 0x0000000000022b06 /build/buildbot/premerge-monolithic-linux/llvm-project/compiler-rt/test/cfi/stats.cpp:35 ucast.retelim cfi-unrelated-cast 81 
check:29'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>>>>

--

********************

…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.

yonghong-song requested review from nikic and david-xl August 22, 2024 21:58

llvmbot added the llvm:transforms label Aug 22, 2024

anakryiko reviewed Aug 22, 2024

View reviewed changes

david-xl reviewed Aug 22, 2024

View reviewed changes

nikic requested a review from efriedma-quic August 23, 2024 07:50

yonghong-song force-pushed the add-func-suffix branch from 9500f42 to 610ab1f Compare August 23, 2024 17:26

llvmbot added LTO Link time optimization (regular/full LTO or ThinLTO) function-specialization llvm:analysis labels Aug 23, 2024

david-xl reviewed Aug 23, 2024

View reviewed changes

efriedma-quic reviewed Aug 23, 2024

View reviewed changes

llvmbot added backend:AArch64 backend:AMDGPU labels Aug 24, 2024

yonghong-song force-pushed the add-func-suffix branch from 6fa9936 to af691b9 Compare August 30, 2024 02:17

Yonghong Song added 3 commits September 2, 2024 08:37

Fix additional test failures

f412f47

Fixing the following test failures: LLVM :: CodeGen/AArch64/arg_promotion.ll LLVM :: CodeGen/AMDGPU/internalize.ll

Add suffix .retelim if only return value is eliminated

3cfffd2

Also fixed a few tests due to rebase on top of main branch.

yonghong-song force-pushed the add-func-suffix branch from af691b9 to 3cfffd2 Compare September 2, 2024 15:51

efriedma-quic approved these changes Sep 18, 2024

View reviewed changes

yonghong-song merged commit 959448f into llvm:main Sep 19, 2024
8 checks passed

yonghong-song mentioned this pull request Sep 25, 2024

[RFC][Transforms][IPO] Add func suffix in ArgumentPromotion and DeadArgumentElimination #109899

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Transforms][IPO] Add func suffix in ArgumentPromotion and DeadArgume… #105742

[Transforms][IPO] Add func suffix in ArgumentPromotion and DeadArgume… #105742

yonghong-song commented Aug 22, 2024 •

edited

Loading

llvmbot commented Aug 22, 2024 •

edited

Loading

yonghong-song commented Aug 22, 2024

anakryiko Aug 22, 2024

anakryiko Aug 22, 2024

yonghong-song commented Aug 22, 2024

david-xl left a comment

yonghong-song commented Aug 22, 2024

david-xl commented Aug 22, 2024

yonghong-song commented Aug 23, 2024

david-xl Aug 23, 2024

yonghong-song Aug 23, 2024

david-xl Aug 23, 2024

yonghong-song Aug 24, 2024

david-xl Aug 24, 2024

yonghong-song Aug 25, 2024

efriedma-quic left a comment

efriedma-quic Aug 23, 2024

yonghong-song Aug 24, 2024

efriedma-quic Aug 29, 2024

yonghong-song Aug 29, 2024

yonghong-song Aug 30, 2024

yonghong-song commented Aug 24, 2024

yonghong-song commented Aug 24, 2024

efriedma-quic commented Aug 27, 2024

yonghong-song commented Aug 29, 2024

yonghong-song commented Sep 2, 2024

yonghong-song commented Sep 5, 2024

yonghong-song commented Sep 11, 2024

efriedma-quic left a comment

llvm-ci commented Sep 19, 2024

		@@ -200,6 +200,7 @@ doPromotion(Function *F, FunctionAnalysisManager &FAM,

		F->getParent()->getFunctionList().insert(F->getIterator(), NF);
		NF->takeName(F);

[Transforms][IPO] Add func suffix in ArgumentPromotion and DeadArgume… #105742

[Transforms][IPO] Add func suffix in ArgumentPromotion and DeadArgume… #105742

Conversation

yonghong-song commented Aug 22, 2024 • edited Loading

llvmbot commented Aug 22, 2024 • edited Loading

yonghong-song commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonghong-song commented Aug 22, 2024

david-xl left a comment

Choose a reason for hiding this comment

yonghong-song commented Aug 22, 2024

david-xl commented Aug 22, 2024

yonghong-song commented Aug 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efriedma-quic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yonghong-song commented Aug 24, 2024

yonghong-song commented Aug 24, 2024

efriedma-quic commented Aug 27, 2024

yonghong-song commented Aug 29, 2024

yonghong-song commented Sep 2, 2024

yonghong-song commented Sep 5, 2024

yonghong-song commented Sep 11, 2024

efriedma-quic left a comment

Choose a reason for hiding this comment

llvm-ci commented Sep 19, 2024

yonghong-song commented Aug 22, 2024 •

edited

Loading

llvmbot commented Aug 22, 2024 •

edited

Loading