[PATCH] D94333: [Inliner] Change inline remark format and update ReplayInlineAdvisor to use it

[Di Mo via Phabricator via cfe-commits]

modimo added a comment.

In D94333#2829233 , @sylvestre.ledru 
wrote:

>> I think this change broke apt.llvm.org
>
> Confirmed: reverting this change fixed the link issue

What exact commit/download package and build command repros this? 
M68kSubtarget.cpp is completely untouched by this change I'm surprised that 
reverting this fixes it. Did you bisect it down to this commit?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94333/new/

https://reviews.llvm.org/D94333

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94333: [Inliner] Change inline remark format and update ReplayInlineAdvisor to use it

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.

> I think this change broke apt.llvm.org

Confirmed: reverting this change fixed the link issue


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94333/new/

https://reviews.llvm.org/D94333

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94333: [Inliner] Change inline remark format and update ReplayInlineAdvisor to use it

[Sylvestre Ledru via Phabricator via cfe-commits]

sylvestre.ledru added a comment.
Herald added a subscriber: ormris.

I think this change broke apt.llvm.org

  > [100%] Linking CXX shared library ../../lib/libLLVM-13.so
  > cd 
"/build/llvm-toolchain-snapshot-13~++20210619083511+1605fce6c307/build-llvm/tools/llvm-shlib"
 && /usr/bin/cmake -E cmake_link_script CMakeFiles/LLVM.dir/link.txt --verbose=1
  > /usr/lib/ccache/g++-10 -fPIC -g -O2 
-fdebug-prefix-map=/build/llvm-toolchain-snapshot-13~++20210619083511+1605fce6c307=.
 -fstack-protector-strong -Wformat -Werror=format-security -fPIC 
-fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall 
-Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual 
-Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough 
-Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move 
-Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor 
-Wsuggest-override -Wno-comment -Wmisleading-indentation -ffunction-sections 
-fdata-sections -O2 -DNDEBUG -g1  
-Wl,-rpath-link,/build/llvm-toolchain-snapshot-13~++20210619083511+1605fce6c307/build-llvm/./lib
  -Wl,-O3 -Wl,--gc-sections -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions 
-Wl,-z,relro -Wl,-z,defs -Wl,-z,nodelete -shared -Wl,-soname,libLLVM-13.so.1 -o 
../../lib/libLLVM-13.so.1 CMakeFiles/LLVM.dir/libllvm.cpp.o  
-Wl,-rpath,"\$ORIGIN/../lib" 
-Wl,--version-script,/build/llvm-toolchain-snapshot-13~++20210619083511+1605fce6c307/build-llvm/./lib/tools/llvm-shlib/simple_version_script.map
 -Wl,--whole-archive ../../lib/libLLVMDemangle.a ../../lib/libLLVMSupport.a 
../../lib/libLLVMCore.a ../../lib/libLLVMFuzzMutate.a 
../../lib/libLLVMFileCheck.a ../../lib/libLLVMInterfaceStub.a 
../../lib/libLLVMIRReader.a ../../lib/libLLVMCodeGen.a 
../../lib/libLLVMSelectionDAG.a ../../lib/libLLVMAsmPrinter.a 
../../lib/libLLVMMIRParser.a ../../lib/libLLVMGlobalISel.a 
../../lib/libLLVMBinaryFormat.a ../../lib/libLLVMBitReader.a 
../../lib/libLLVMBitWriter.a ../../lib/libLLVMBitstreamReader.a 
../../lib/libLLVMDWARFLinker.a ../../lib/libLLVMExtensions.a 
../../lib/libLLVMFrontendOpenACC.a ../../lib/libLLVMFrontendOpenMP.a 
../../lib/libLLVMTransformUtils.a ../../lib/libLLVMInstrumentation.a 
../../lib/libLLVMAggressiveInstCombine.a ../../lib/libLLVMInstCombine.a 
../../lib/libLLVMScalarOpts.a ../../lib/libLLVMipo.a 
../../lib/libLLVMVectorize.a ../../lib/libLLVMObjCARCOpts.a 
../../lib/libLLVMCoroutines.a ../../lib/libLLVMCFGuard.a 
../../lib/libLLVMLinker.a ../../lib/libLLVMAnalysis.a ../../lib/libLLVMLTO.a 
../../lib/libLLVMMC.a ../../lib/libLLVMMCParser.a 
../../lib/libLLVMMCDisassembler.a ../../lib/libLLVMMCA.a 
../../lib/libLLVMObject.a ../../lib/libLLVMObjectYAML.a 
../../lib/libLLVMOption.a ../../lib/libLLVMRemarks.a 
../../lib/libLLVMDebugInfoDWARF.a ../../lib/libLLVMDebugInfoGSYM.a 
../../lib/libLLVMDebugInfoMSF.a ../../lib/libLLVMDebugInfoCodeView.a 
../../lib/libLLVMDebugInfoPDB.a ../../lib/libLLVMSymbolize.a 
../../lib/libLLVMExecutionEngine.a ../../lib/libLLVMInterpreter.a 
../../lib/libLLVMJITLink.a ../../lib/libLLVMMCJIT.a ../../lib/libLLVMOrcJIT.a 
../../lib/libLLVMOrcShared.a ../../lib/libLLVMOrcTargetProcess.a 
../../lib/libLLVMRuntimeDyld.a ../../lib/libLLVMPerfJITEvents.a 
../../lib/libLLVMTarget.a ../../lib/libLLVMAArch64CodeGen.a 
../../lib/libLLVMAArch64AsmParser.a ../../lib/libLLVMAArch64Disassembler.a 
../../lib/libLLVMAArch64Desc.a ../../lib/libLLVMAArch64Info.a 
../../lib/libLLVMAArch64Utils.a ../../lib/libLLVMAMDGPUCodeGen.a 
../../lib/libLLVMAMDGPUAsmParser.a ../../lib/libLLVMAMDGPUDisassembler.a 
../../lib/libLLVMAMDGPUDesc.a ../../lib/libLLVMAMDGPUInfo.a 
../../lib/libLLVMAMDGPUUtils.a ../../lib/libLLVMARMCodeGen.a 
../../lib/libLLVMARMAsmParser.a ../../lib/libLLVMARMDisassembler.a 
../../lib/libLLVMARMDesc.a ../../lib/libLLVMARMInfo.a 
../../lib/libLLVMARMUtils.a ../../lib/libLLVMAVRCodeGen.a 
../../lib/libLLVMAVRAsmParser.a ../../lib/libLLVMAVRDisassembler.a 
../../lib/libLLVMAVRDesc.a ../../lib/libLLVMAVRInfo.a 
../../lib/libLLVMBPFCodeGen.a ../../lib/libLLVMBPFAsmParser.a 
../../lib/libLLVMBPFDisassembler.a ../../lib/libLLVMBPFDesc.a 
../../lib/libLLVMBPFInfo.a ../../lib/libLLVMHexagonCodeGen.a 
../../lib/libLLVMHexagonAsmParser.a ../../lib/libLLVMHexagonDisassembler.a 
../../lib/libLLVMHexagonDesc.a ../../lib/libLLVMHexagonInfo.a 
../../lib/libLLVMLanaiCodeGen.a ../../lib/libLLVMLanaiAsmParser.a 
../../lib/libLLVMLanaiDisassembler.a ../../lib/libLLVMLanaiDesc.a 
../../lib/libLLVMLanaiInfo.a ../../lib/libLLVMMipsCodeGen.a 
../../lib/libLLVMMipsAsmParser.a ../../lib/libLLVMMipsDisassembler.a 
../../lib/libLLVMMipsDesc.a ../../lib/libLLVMMipsInfo.a 
../../lib/libLLVMMSP430CodeGen.a ../../lib/libLLVMMSP430Desc.a 
../../lib/libLLVMMSP430Info.a ../../lib/libLLVMMSP430AsmParser.a 
../../lib/libLLVMMSP430Disassembler.a ../../lib/libLLVMNVPTXCodeGen.a 
../../lib/libLLVMNVPTXDesc.a ../../lib/libLLVMNVPTXInfo.a 
../../lib/libLLVMPowerPCCodeGen.a ../../lib/libLLVMPowerPCAsmParser.a 

[PATCH] D94333: [Inliner] Change inline remark format and update ReplayInlineAdvisor to use it

[Di Mo via Phabricator via cfe-commits]

modimo added inline comments.



Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:412
+
+  Remark << ";";
 }

wenlei wrote:
> modimo wrote:
> > wenlei wrote:
> > > nit: any special reason for adding this? doesn't seem consistent with 
> > > other remarks we have.
> > If you grab the remark outputs via `-Rpass=inline` you'll get additional 
> > suffix information:
> > ```
> > inline.cpp:8:12: remark: _Z3foov inlined into main with (cost=0, 
> > threshold=375) at callsite main:2:12; [-Rpass=inline]
> > return foo();
> > ```
> > 
> > The semicolon is to separate the remark from any additional output at the 
> > end so when replaying we can match the correct callsite. Something like 
> > this would be unneeded for yaml replay but for the current implementation 
> > it's necessary for correctness.
> > 
> By correctness, did you mean the fact that you rely on `split(";")` in 
> parsing, or something else?
> 
> This is not a big deal, but if no other remarks end with `;`, it would be 
> good to be consistent. Using `split(";")` for parsing is just one way of 
> implementing it, and IMO could be changed to favor consistency in remarks 
> output.
> By correctness, did you mean the fact that you rely on `split(";")` in 
> parsing, or something else?

Yeah, without that we would store the callsite from remarks as `main:2:12 
[-Rpass=inline]` which would not match the actual callsite string `main:2:12` 
that we query the map with which causes replay to never inline.

> This is not a big deal, but if no other remarks end with `;`, it would be 
> good to be consistent. Using `split(";")` for parsing is just one way of 
> implementing it, and IMO could be changed to favor consistency in remarks 
> output.

Doing a search query for `OptimizationRemarkAnalysis` I see vectorizer ORE uses 
"." for their terminator so switching to that is better consistency. I'll make 
the change in an upcoming patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94333/new/

https://reviews.llvm.org/D94333

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94333: [Inliner] Change inline remark format and update ReplayInlineAdvisor to use it

[Wenlei He via Phabricator via cfe-commits]

wenlei added inline comments.



Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:412
+
+  Remark << ";";
 }

modimo wrote:
> wenlei wrote:
> > nit: any special reason for adding this? doesn't seem consistent with other 
> > remarks we have.
> If you grab the remark outputs via `-Rpass=inline` you'll get additional 
> suffix information:
> ```
> inline.cpp:8:12: remark: _Z3foov inlined into main with (cost=0, 
> threshold=375) at callsite main:2:12; [-Rpass=inline]
> return foo();
> ```
> 
> The semicolon is to separate the remark from any additional output at the end 
> so when replaying we can match the correct callsite. Something like this 
> would be unneeded for yaml replay but for the current implementation it's 
> necessary for correctness.
> 
By correctness, did you mean the fact that you rely on `split(";")` in parsing, 
or something else?

This is not a big deal, but if no other remarks end with `;`, it would be good 
to be consistent. Using `split(";")` for parsing is just one way of 
implementing it, and IMO could be changed to favor consistency in remarks 
output.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94333/new/

https://reviews.llvm.org/D94333

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94333: [Inliner] Change inline remark format and update ReplayInlineAdvisor to use it

[Di Mo via Phabricator via cfe-commits]

modimo added inline comments.



Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:412
+
+  Remark << ";";
 }

wenlei wrote:
> nit: any special reason for adding this? doesn't seem consistent with other 
> remarks we have.
If you grab the remark outputs via `-Rpass=inline` you'll get additional suffix 
information:
```
inline.cpp:8:12: remark: _Z3foov inlined into main with (cost=0, threshold=375) 
at callsite main:2:12; [-Rpass=inline]
return foo();
```

The semicolon is to separate the remark from any additional output at the end 
so when replaying we can match the correct callsite. Something like this would 
be unneeded for yaml replay but for the current implementation it's necessary 
for correctness.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94333/new/

https://reviews.llvm.org/D94333

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94333: [Inliner] Change inline remark format and update ReplayInlineAdvisor to use it

[Wenlei He via Phabricator via cfe-commits]

wenlei added inline comments.



Comment at: llvm/lib/Analysis/InlineAdvisor.cpp:412
+
+  Remark << ";";
 }

nit: any special reason for adding this? doesn't seem consistent with other 
remarks we have.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94333/new/

https://reviews.llvm.org/D94333

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D94333: [Inliner] Change inline remark format and update ReplayInlineAdvisor to use it

[Di Mo via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2a49b7c64a33: [Inliner] Change inline remark format and 
update ReplayInlineAdvisor to use it (authored by modimo).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94333/new/

https://reviews.llvm.org/D94333

Files:
  clang/test/Frontend/optimization-remark-with-hotness-new-pm.c
  clang/test/Frontend/optimization-remark-with-hotness.c
  llvm/include/llvm/Analysis/InlineAdvisor.h
  llvm/include/llvm/Analysis/ReplayInlineAdvisor.h
  llvm/lib/Analysis/InlineAdvisor.cpp
  llvm/lib/Analysis/ReplayInlineAdvisor.cpp
  llvm/lib/Transforms/IPO/SampleProfile.cpp
  llvm/test/Transforms/Inline/optimization-remarks-passed-yaml.ll
  llvm/test/Transforms/SampleProfile/Inputs/inline-replay.txt
  llvm/test/Transforms/SampleProfile/inline-replay.ll
  llvm/test/Transforms/SampleProfile/remarks-hotness.ll
  llvm/test/Transforms/SampleProfile/remarks.ll

Index: llvm/test/Transforms/SampleProfile/remarks.ll
===
--- llvm/test/Transforms/SampleProfile/remarks.ll
+++ llvm/test/Transforms/SampleProfile/remarks.ll
@@ -21,8 +21,8 @@
 
 ; We are expecting foo() to be inlined in main() (almost all the cycles are
 ; spent inside foo).
-; CHECK: remark: remarks.cc:13:21: _Z3foov inlined into main to match profiling context with (cost=130, threshold=225) at callsite main:0
-; CHECK: remark: remarks.cc:9:19: rand inlined into main to match profiling context with (cost=always): always inline attribute at callsite _Z3foov:6 @ main:0
+; CHECK: remark: remarks.cc:13:21: _Z3foov inlined into main to match profiling context with (cost=130, threshold=225) at callsite main:0:21;
+; CHECK: remark: remarks.cc:9:19: rand inlined into main to match profiling context with (cost=always): always inline attribute at callsite _Z3foov:6:19 @ main:0:21;
 
 ; The back edge for the loop is the hottest edge in the loop subgraph.
 ; CHECK: remark: remarks.cc:6:9: most popular destination for conditional branches at remarks.cc:5:3
@@ -53,6 +53,9 @@
 ;YAML-NEXT:- String:  main
 ;YAML-NEXT:- String:  ':'
 ;YAML-NEXT:- Line:'0'
+;YAML-NEXT:- String:  ':'
+;YAML-NEXT:- Column:  '21'
+;YAML-NEXT:- String:  ';'
 ;YAML-NEXT:  ...
 ;YAML:   --- !Passed
 ;YAML-NEXT:  Pass:sample-profile-inline
@@ -74,10 +77,15 @@
 ;YAML-NEXT:- String:  _Z3foov
 ;YAML-NEXT:- String:  ':'
 ;YAML-NEXT:- Line:'6'
+;YAML-NEXT:- String:  ':'
+;YAML-NEXT:- Column:  '19'
 ;YAML-NEXT:- String:  ' @ '
 ;YAML-NEXT:- String:  main
 ;YAML-NEXT:- String:  ':'
 ;YAML-NEXT:- Line:'0'
+;YAML-NEXT:- String:  ':'
+;YAML-NEXT:- Column:  '21'
+;YAML-NEXT:- String:  ';'
 ;YAML:  --- !Analysis
 ;YAML-NEXT:  Pass:sample-profile
 ;YAML-NEXT:  Name:AppliedSamples
Index: llvm/test/Transforms/SampleProfile/remarks-hotness.ll
===
--- llvm/test/Transforms/SampleProfile/remarks-hotness.ll
+++ llvm/test/Transforms/SampleProfile/remarks-hotness.ll
@@ -36,7 +36,7 @@
 ; YAML-MISS-NEXT: Function:_Z7caller2v
 ; YAML-MISS-NEXT: Hotness: 2
 
-; CHECK-RPASS: _Z7callee1v inlined into _Z7caller1v with (cost=-30, threshold=4500) at callsite _Z7caller1v:1 (hotness: 401)
+; CHECK-RPASS: _Z7callee1v inlined into _Z7caller1v with (cost=-30, threshold=4500) at callsite _Z7caller1v:1:10; (hotness: 401)
 ; CHECK-RPASS-NOT: _Z7callee2v not inlined into _Z7caller2v because it should never be inlined (cost=never): noinline function attribute (hotness: 2)
 
 ; ModuleID = 'remarks-hotness.cpp'
@@ -93,4 +93,3 @@
 !17 = distinct !DISubprogram(name: "caller2", linkageName: "_Z7caller2v", scope: !1, file: !1, line: 13, type: !8, scopeLine: 13, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
 !18 = !DILocation(line: 14, column: 10, scope: !17)
 !19 = !DILocation(line: 14, column: 3, scope: !17)
-
Index: llvm/test/Transforms/SampleProfile/inline-replay.ll
===
--- llvm/test/Transforms/SampleProfile/inline-replay.ll
+++ llvm/test/Transforms/SampleProfile/inline-replay.ll
@@ -119,4 +119,4 @@
 
 ; REPLAY: _Z3sumii inlined into main
 ; REPLAY: _Z3subii inlined into main
-; REPLA-NOT: _Z3subii inlined into _Z3sumii
+; REPLAY-NOT: _Z3subii inlined into _Z3sumii
Index: llvm/test/Transforms/SampleProfile/Inputs/inline-replay.txt
===
--- llvm/test/Transforms/SampleProfile/Inputs/inline-replay.txt
+++ llvm/test/Transforms/SampleProfile/Inputs/inline-replay.txt
@@ -1,2 +1,2 @@
-remark: