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

[modimo]

Summary

This change modifies the source location formatting from:
LineNumber.Discriminator
to:
LineNumber:ColumnNumber.Discriminator

The motivation here is to enhance location information for inline replay that currently exists for the SampleProfile inliner. This will be leveraged further in inline replay for the CGSCC inliner in the related diff.

The ReplayInlineAdvisor is also modified to read the new format and now takes into account the callee for greater accuracy.

Testing:
ninja check-llvm

[thegameg]

llvm/lib/Analysis/InlineAdvisor.cpp

405–406

Can we use a DebugLoc here like Callee and Caller?

Something like:

- String: 'at callsite '
- CallSite: bar
  DebugLoc: { File: '/tmp/s.c', Line: 1, Column: 10 }

[davidxl]

The proposal in the patch is to use line:col.discriminator (which is reasonable), but the implementation uses line:col:discriminator -- please update the patch to be consistent.

[wenlei]

llvm/lib/Analysis/InlineAdvisor.cpp

405–406

Having location in text output as well (in addition to yaml) makes it easier to consume for replay. If we go with what Caller/Callee use, I guess the location only goes to yaml, but not text remarks?

As for the format of location string in text mark, the current form aligns with the context representation of CSSPGO's profile, so it makes things a bit easier for inlining related investigation, etc, which is intended use case for inline replay scaffolding.

[modimo]

Split changes with D94334 better: now the ReplayInlineAdvisor update which needs to be atomic with the formatting change is moved over to here. Removed ReplayInlineAdvice and modified DefaultInlineAdvice to provide the desired functionality. Fixed up tests.

[modimo]

In D94333#2487829, @davidxl wrote:

The proposal in the patch is to use line:col.discriminator (which is reasonable), but the implementation uses line:col:discriminator -- please update the patch to be consistent.

I might have missed something but the implemented format is line:col.discriminator. See:

remark: calls.cc:4:0: _Z3subii inlined into main to match profiling context with (cost=-5, threshold=337) at callsite _Z3sumii:1:0 @ main:3:0.1;

and

if (Discriminator)
  CallSiteLoc << "." << llvm::utostr(Discriminator);

llvm/lib/Analysis/InlineAdvisor.cpp

405–406

I like the idea! Testing out using the DLoc as an ORE generates:

- CallSite:        'calls.cc:10:0'
  DebugLoc:        { File: calls.cc, Line: 10, Column: 0 }

As compared to:

- String:          main
- String:          ':'
- Line:            '3'
- String:          ':'
- Column:          '0'
- String:          .
- Disc:            '1'
- String:          ';'

We can probably package up the DLoc to output its name better like how Callee gets it automatically with ore::NV("Callee", &Callee). One of the refinements we do here though is to generate function-relative line numbers which are more tolerant of adding/deleting source lines while maintaining inline replay. Also, looking at the implementation DebugLoc doesn't seem to recurse through inlined sites which we need for cases like:

callsite _Z3sumii:1:0 @ main:3:0.1;

If we can extend DebugLoc to have the processed form than I think that's an elegant way to do this.

Additionally I've verified that DebugLoc only shows up in the yaml output which with the current scheme of reading the remarks as plaintext means that won't work. There's discussion on companion diff D94334 on whether it makes more sense to use YAML for these remarks which also informs whether we should go down this route.

[davidxl]

You are right -- the text format is properly implemented.

[mtrofin]

llvm/lib/Analysis/InlineAdvisor.cpp

65

Why add optional remark emission?

[davidxl]

Should we get rid of DefaultInlineAdvice and fold it into the base class InlineAdvice? 'DefaultInlineAdvice' implies it is an advice provided by the default advisor which no longer is true.

Besides the inlineAdvice contains a reference to the inlineAdvisor, so where the advice comes from is discoverable.

@mtrofin

[mtrofin]

In D94333#2488359, @davidxl wrote:

Should we get rid of DefaultInlineAdvice and fold it into the base class InlineAdvice? 'DefaultInlineAdvice' implies it is an advice provided by the default advisor which no longer is true.

Besides the inlineAdvice contains a reference to the inlineAdvisor, so where the advice comes from is discoverable.

@mtrofin

I can look into it, we need to look at its uses on the ML side. Perhaps the logging part can be moved, because the InlineCost stuff is very much default policy-specific.

[modimo]

The modifications to the DefaultInlineAdvice is an attempt to solve the following problem:

1. In the SampleProfile inliner it emits remarks through the "legacy" interface with emitInlinedInto. For replay if advice generates remarks you'll get duplicate remarks but if it doesn't you get a single set (which is what's happening today and correct).
2. In the CGSCC inliner in newPM it emits remarks through InlineAdvice via recordInlining and nowhere else. For replay if InlineAdvice generates remarks you're good but if it doesn't you get no remarks.

So to solve this issue I have the ReplayAdvisor use DefaultInlineAdvice with modifications to suppress remarks for the SampleProfile inliner.

In D94333#2488360, @mtrofin wrote:

I can look into it, we need to look at its uses on the ML side. Perhaps the logging part can be moved, because the InlineCost stuff is very much default policy-specific.

This seems like a cleaner approach than my current solution. I'll need to move the EmitRemarks over to the base class which looks to be okay given it can be defaulted to true. Thoughts?

llvm/lib/Analysis/InlineAdvisor.cpp

65

The SampleProfile.cpp inliner emits its own remarks via the emitInlinedInto interface. Without this you would get double remarks when dumping the SampleProfile.cpp inliner which I want to avoid. The rest of the comments suggests there's a better holistic solution so I'll add my thoughts to a below comment.

[mtrofin]

In D94333#2491205, @modimo wrote:

The modifications to the DefaultInlineAdvice is an attempt to solve the following problem:

1. In the SampleProfile inliner it emits remarks through the "legacy" interface with emitInlinedInto. For replay if advice generates remarks you'll get duplicate remarks but if it doesn't you get a single set (which is what's happening today and correct).
2. In the CGSCC inliner in newPM it emits remarks through InlineAdvice via recordInlining and nowhere else. For replay if InlineAdvice generates remarks you're good but if it doesn't you get no remarks.

So to solve this issue I have the ReplayAdvisor use DefaultInlineAdvice with modifications to suppress remarks for the SampleProfile inliner.

In D94333#2488360, @mtrofin wrote:

I can look into it, we need to look at its uses on the ML side. Perhaps the logging part can be moved, because the InlineCost stuff is very much default policy-specific.

This seems like a cleaner approach than my current solution. I'll need to move the EmitRemarks over to the base class which looks to be okay given it can be defaulted to true. Thoughts?

I wouldn't block this patch on that - we can do the refactoring subsequently.

[mtrofin]

LGTM; but so I understand something better: the ability to replay is somewhat 'best effort' if the compiler changes (or, of course, the compiled code). May be worth adding a comment about that?

[modimo]

In D94333#2491835, @mtrofin wrote:

I wouldn't block this patch on that - we can do the refactoring subsequently.

Sounds good.

In D94333#2491836, @mtrofin wrote:

LGTM; but so I understand something better: the ability to replay is somewhat 'best effort' if the compiler changes (or, of course, the compiled code). May be worth adding a comment about that?

Thanks for signing off! Your take here is correct, this is operating under a best effort approach to replay. In D94334 I note that testing on mysqld the current solution for CGSCC inline replay is ~94% accurate in replaying when comparing remarks base vs. replay.

Added a comment in ReplayInlineAdvisor describing the best effort approach here.

[modimo]

Add comment about best effort approach of replay.

[wenlei]

llvm/lib/Analysis/InlineAdvisor.cpp

412

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

[modimo]

llvm/lib/Analysis/InlineAdvisor.cpp

412

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.

[wenlei]

llvm/lib/Analysis/InlineAdvisor.cpp

412

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.

[modimo]

llvm/lib/Analysis/InlineAdvisor.cpp

412

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.

[sylvestre.ledru]

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 ../../lib/libLLVMPowerPCDisassembler.a ../../lib/libLLVMPowerPCDesc.a ../../lib/libLLVMPowerPCInfo.a ../../lib/libLLVMRISCVCodeGen.a ../../lib/libLLVMRISCVAsmParser.a ../../lib/libLLVMRISCVDisassembler.a ../../lib/libLLVMRISCVDesc.a ../../lib/libLLVMRISCVInfo.a ../../lib/libLLVMSparcCodeGen.a ../../lib/libLLVMSparcAsmParser.a ../../lib/libLLVMSparcDisassembler.a ../../lib/libLLVMSparcDesc.a ../../lib/libLLVMSparcInfo.a ../../lib/libLLVMSystemZCodeGen.a ../../lib/libLLVMSystemZAsmParser.a ../../lib/libLLVMSystemZDisassembler.a ../../lib/libLLVMSystemZDesc.a ../../lib/libLLVMSystemZInfo.a ../../lib/libLLVMWebAssemblyCodeGen.a ../../lib/libLLVMWebAssemblyAsmParser.a ../../lib/libLLVMWebAssemblyDisassembler.a ../../lib/libLLVMWebAssemblyDesc.a ../../lib/libLLVMWebAssemblyInfo.a ../../lib/libLLVMWebAssemblyUtils.a ../../lib/libLLVMX86CodeGen.a ../../lib/libLLVMX86AsmParser.a ../../lib/libLLVMX86Disassembler.a ../../lib/libLLVMX86Desc.a ../../lib/libLLVMX86Info.a ../../lib/libLLVMXCoreCodeGen.a ../../lib/libLLVMXCoreDisassembler.a ../../lib/libLLVMXCoreDesc.a ../../lib/libLLVMXCoreInfo.a ../../lib/libLLVMM68kCodeGen.a ../../lib/libLLVMM68kInfo.a ../../lib/libLLVMM68kDesc.a ../../lib/libLLVMM68kAsmParser.a ../../lib/libLLVMM68kDisassembler.a ../../lib/libLLVMAsmParser.a ../../lib/libLLVMLineEditor.a ../../lib/libLLVMProfileData.a ../../lib/libLLVMCoverage.a ../../lib/libLLVMPasses.a ../../lib/libLLVMTextAPI.a ../../lib/libLLVMDlltoolDriver.a ../../lib/libLLVMLibDriver.a ../../lib/libLLVMXRay.a ../../lib/libLLVMWindowsManifest.a -Wl,--no-whole-archive ../../lib/libLLVMExtensions.a ../../lib/libPolly.a ../../lib/libPollyISL.a ../../lib/libLLVMDebugInfoPDB.a /usr/lib/s390x-linux-gnu/libffi.so ../../lib/libLLVMJITLink.a ../../lib/libLLVMExecutionEngine.a ../../lib/libLLVMOrcTargetProcess.a ../../lib/libLLVMOrcShared.a ../../lib/libLLVMRuntimeDyld.a ../../lib/libLLVMMIRParser.a ../../lib/libLLVMPasses.a ../../lib/libLLVMObjCARCOpts.a ../../lib/libLLVMCoroutines.a ../../lib/libLLVMMSP430Desc.a ../../lib/libLLVMMSP430Info.a ../../lib/libLLVMipo.a ../../lib/libLLVMIRReader.a ../../lib/libLLVMAsmParser.a ../../lib/libLLVMFrontendOpenMP.a ../../lib/libLLVMInstrumentation.a ../../lib/libLLVMLinker.a ../../lib/libLLVMVectorize.a ../../lib/libLLVMCFGuard.a ../../lib/libLLVMM68kCodeGen.a ../../lib/libLLVMAsmPrinter.a ../../lib/libLLVMDebugInfoDWARF.a ../../lib/libLLVMDebugInfoMSF.a ../../lib/libLLVMGlobalISel.a ../../lib/libLLVMSelectionDAG.a ../../lib/libLLVMCodeGen.a ../../lib/libLLVMBitWriter.a ../../lib/libLLVMScalarOpts.a ../../lib/libLLVMAggressiveInstCombine.a ../../lib/libLLVMInstCombine.a ../../lib/libLLVMTransformUtils.a ../../lib/libLLVMTarget.a ../../lib/libLLVMAnalysis.a ../../lib/libLLVMM68kDesc.a ../../lib/libLLVMMCDisassembler.a ../../lib/libLLVMM68kInfo.a -ledit ../../lib/libLLVMProfileData.a ../../lib/libLLVMOption.a ../../lib/libLLVMObject.a ../../lib/libLLVMBitReader.a ../../lib/libLLVMCore.a ../../lib/libLLVMRemarks.a ../../lib/libLLVMBitstreamReader.a ../../lib/libLLVMMCParser.a ../../lib/libLLVMMC.a ../../lib/libLLVMDebugInfoCodeView.a ../../lib/libLLVMTextAPI.a ../../lib/libLLVMBinaryFormat.a ../../lib/libLLVMSupport.a ../../lib/libLLVMDemangle.a -lrt -ldl -lpthread -lm /usr/lib/s390x-linux-gnu/libz.so /usr/lib/s390x-linux-gnu/libtinfo.so 
> /usr/bin/ld: ../../lib/libLLVMM68kCodeGen.a(M68kSubtarget.cpp.o): in function `llvm::M68kGenRegisterBankInfo::M68kGenRegisterBankInfo()':
> ./build-llvm/lib/Target/M68k/./llvm/include/llvm/ADT/SmallVector.h:580: multiple definition of `llvm::M68kGenRegisterBankInfo::M68kGenRegisterBankInfo()'; ../../lib/libLLVMM68kCodeGen.a(M68kRegisterBankInfo.cpp.o):./build-llvm/lib/Target/M68k/./build-llvm/lib/Target/M68k/M68kGenRegisterBank.inc:49: first defined here
> /usr/bin/ld: ../../lib/libLLVMM68kCodeGen.a(M68kSubtarget.cpp.o): in function `llvm::M68kGenRegisterBankInfo::M68kGenRegisterBankInfo()':
> ./build-llvm/lib/Target/M68k/./llvm/include/llvm/ADT/SmallVector.h:580: multiple definition of `llvm::M68kGenRegisterBankInfo::M68kGenRegisterBankInfo()'; ../../lib/libLLVMM68kCodeGen.a(M68kRegisterBankInfo.cpp.o):./build-llvm/lib/Target/M68k/./build-llvm/lib/Target/M68k/M68kGenRegisterBank.inc:49: first defined here
> /usr/bin/ld: ../../lib/libLLVMM68kCodeGen.a(M68kSubtarget.cpp.o): in function `llvm::M68kRegisterBankInfo::M68kRegisterBankInfo(llvm::TargetRegisterInfo const&)':
> ./build-llvm/lib/Target/M68k/./llvm/include/llvm/ADT/SmallVector.h:254: multiple definition of `llvm::M68kRegisterBankInfo::M68kRegisterBankInfo(llvm::TargetRegisterInfo const&)'; ../../lib/libLLVMM68kCodeGen.a(M68kRegisterBankInfo.cpp.o):./build-llvm/lib/Target/M68k/./llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.cpp:26: first defined here
> /usr/bin/ld: ../../lib/libLLVMM68kCodeGen.a(M68kSubtarget.cpp.o): in function `llvm::M68kRegisterBankInfo::M68kRegisterBankInfo(llvm::TargetRegisterInfo const&)':
> ./build-llvm/lib/Target/M68k/./llvm/include/llvm/ADT/SmallVector.h:254: multiple definition of `llvm::M68kRegisterBankInfo::M68kRegisterBankInfo(llvm::TargetRegisterInfo const&)'; ../../lib/libLLVMM68kCodeGen.a(M68kRegisterBankInfo.cpp.o):./build-llvm/lib/Target/M68k/./llvm/lib/Target/M68k/GlSel/M68kRegisterBankInfo.cpp:26: first defined here
> /usr/bin/ld: ../../lib/libLLVMM68kCodeGen.a(M68kSubtarget.cpp.o):(.data.rel.local._ZN4llvm23M68kGenRegisterBankInfo8RegBanksE+0x0): multiple definition of `llvm::M68kGenRegisterBankInfo::RegBanks'; ../../lib/libLLVMM68kCodeGen.a(M68kRegisterBankInfo.cpp.o):(.data.rel.local._ZN4llvm23M68kGenRegisterBankInfo8RegBanksE+0x0): first defined here
> /usr/bin/ld: ../../lib/libLLVMM68kCodeGen.a(M68kSubtarget.cpp.o):(.bss._ZN4llvm4M68k10GPRRegBankE+0x0): multiple definition of `llvm::M68k::GPRRegBank'; ../../lib/libLLVMM68kCodeGen.a(M68kRegisterBankInfo.cpp.o):(.bss._ZN4llvm4M68k10GPRRegBankE+0x0): first defined here

Could you please have a look?

[sylvestre.ledru]

I think this change broke apt.llvm.org

Confirmed: reverting this change fixed the link issue

[modimo]

In D94333#2829233, @sylvestre.ledru wrote:

I think this change broke apt.llvm.org

Confirmed: reverting this change fixed the link issue

What build command/Cmake setting 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?

clang/test/Frontend/optimization-remark-with-hotness-new-pm.c

 
 void bar(int x) {
   // HOTNESS_OFF: foo inlined into bar
   // HOTNESS_OFF-NOT: hotness:
   // THRESHOLD-NOT: inlined
   // THRESHOLD-NOT: hotness
   // NO_PGO: '-fdiagnostics-show-hotness' requires profile-guided optimization information
   // NO_PGO: '-fdiagnostics-hotness-threshold=' requires profile-guided optimization information
-  // expected-remark@+1 {{foo inlined into bar with (cost=always): always inline attribute at callsite bar:8 (hotness:}}
+  // expected-remark@+1 {{foo inlined into bar with (cost=always): always inline attribute at callsite bar:8:10; (hotness:}}
   sum += foo(x, x - 2);
 }
 
 int main(int argc, const char *argv[]) {
   for (int i = 0; i < 30; i++)
     // expected-remark@+1 {{bar inlined into main with}}
     bar(argc);
   return sum;
 }

clang/test/Frontend/optimization-remark-with-hotness.c

 
 void bar(int x) {
   // HOTNESS_OFF: foo inlined into bar
   // HOTNESS_OFF-NOT: hotness:
   // THRESHOLD-NOT: inlined
   // THRESHOLD-NOT: hotness
   // NO_PGO: '-fdiagnostics-show-hotness' requires profile-guided optimization information
   // NO_PGO: '-fdiagnostics-hotness-threshold=' requires profile-guided optimization information
-  // expected-remark@+1 {{foo inlined into bar with (cost=always): always inliner at callsite bar:8 (hotness:}}
+  // expected-remark@+1 {{foo inlined into bar with (cost=always): always inliner at callsite bar:8:10; (hotness:}}
   sum += foo(x, x - 2);
 }
 
 int main(int argc, const char *argv[]) {
   for (int i = 0; i < 30; i++)
     // expected-remark@+1 {{bar not inlined into main because it should never be inlined (cost=never): no alwaysinline attribute (hotness:}}
     bar(argc);
   return sum;
 }

llvm/include/llvm/Analysis/InlineAdvisor.h

private:
   void markRecorded() {
     assert(!Recorded && "Recording should happen exactly once");
     Recorded = true;
   }
 
   bool Recorded = false;
 };
 
+class DefaultInlineAdvice : public InlineAdvice {
+public:
+  DefaultInlineAdvice(InlineAdvisor *Advisor, CallBase &CB,
+                      Optional<InlineCost> OIC, OptimizationRemarkEmitter &ORE,
+                      bool EmitRemarks = true)
+      : InlineAdvice(Advisor, CB, ORE, OIC.hasValue()), OriginalCB(&CB),
+        OIC(OIC), EmitRemarks(EmitRemarks) {}
+
+private:
+  void recordUnsuccessfulInliningImpl(const InlineResult &Result) override;
+  void recordInliningWithCalleeDeletedImpl() override;
+  void recordInliningImpl() override;
+
+private:
+  CallBase *const OriginalCB;
+  Optional<InlineCost> OIC;
+  bool EmitRemarks;
+};
+
 /// Interface for deciding whether to inline a call site or not.
 class InlineAdvisor {
 public:
   InlineAdvisor(InlineAdvisor &&) = delete;
   virtual ~InlineAdvisor() { freeDeletedFunctions(); }
 
   /// Get an InlineAdvice containing a recommendation on whether to
   /// inline or not. \p CB is assumed to be a direct call. \p FAM is assumed to

llvm/include/llvm/Analysis/ReplayInlineAdvisor.h

 class Module;
 class OptimizationRemarkEmitter;
 
 /// Replay inline advisor that uses optimization remarks from inlining of
 /// previous build to guide current inlining. This is useful for inliner tuning.
 class ReplayInlineAdvisor : public InlineAdvisor {
 public:
   ReplayInlineAdvisor(FunctionAnalysisManager &FAM, LLVMContext &Context,
-                      StringRef RemarksFile);
+                      StringRef RemarksFile, bool EmitRemarks);
   std::unique_ptr<InlineAdvice> getAdvice(CallBase &CB) override;
   bool areReplayRemarksLoaded() const { return HasReplayRemarks; }
 
 private:
   StringSet<> InlineSitesFromRemarks;
   bool HasReplayRemarks = false;
+  bool EmitRemarks = false;
 };
 } // namespace llvm
 #endif // LLVM_ANALYSIS_REPLAYINLINEADVISOR_H

llvm/lib/Analysis/InlineAdvisor.cpp

 
 // An integer used to limit the cost of inline deferral.  The default negative
 // number tells shouldBeDeferred to only take the secondary cost into account.
 static cl::opt<int>
     InlineDeferralScale("inline-deferral-scale",
                         cl::desc("Scale to limit the cost of inline deferral"),
                         cl::init(2), cl::Hidden);
 
-namespace {
-class DefaultInlineAdvice : public InlineAdvice {
-public:
-  DefaultInlineAdvice(DefaultInlineAdvisor *Advisor, CallBase &CB,
-                      Optional<InlineCost> OIC, OptimizationRemarkEmitter &ORE)
-      : InlineAdvice(Advisor, CB, ORE, OIC.hasValue()), OriginalCB(&CB),
-        OIC(OIC) {}
-
-private:
-  void recordUnsuccessfulInliningImpl(const InlineResult &Result) override {
-    using namespace ore;
-    llvm::setInlineRemark(*OriginalCB, std::string(Result.getFailureReason()) +
-                                           "; " + inlineCostStr(*OIC));
-    ORE.emit([&]() {
-      return OptimizationRemarkMissed(DEBUG_TYPE, "NotInlined", DLoc, Block)
-             << NV("Callee", Callee) << " will not be inlined into "
-             << NV("Caller", Caller) << ": "
-             << NV("Reason", Result.getFailureReason());
-    });
-  }
+void DefaultInlineAdvice::recordUnsuccessfulInliningImpl(
+    const InlineResult &Result) {
 
-  void recordInliningWithCalleeDeletedImpl() override {
+void DefaultInlineAdvice::recordInliningWithCalleeDeletedImpl() {
+  if (EmitRemarks)
     emitInlinedInto(ORE, DLoc, Block, *Callee, *Caller, *OIC);
-  }
 
-  void recordInliningImpl() override {
+void DefaultInlineAdvice::recordInliningImpl() {
+  if (EmitRemarks)
     emitInlinedInto(ORE, DLoc, Block, *Callee, *Caller, *OIC);
-  }
 
-private:
-  CallBase *const OriginalCB;
-  Optional<InlineCost> OIC;
-};
-
-} // namespace
-
 llvm::Optional<llvm::InlineCost> static getDefaultInlineAdvice(
     CallBase &CB, FunctionAnalysisManager &FAM, const InlineParams &Params) {
   Function &Caller = *CB.getCaller();
   ProfileSummaryInfo *PSI =
       FAM.getResult<ModuleAnalysisManagerFunctionProxy>(Caller)
           .getCachedResult<ProfileSummaryAnalysis>(
               *CB.getParent()->getParent()->getParent());
 
for (DILocation *DIL = DLoc.get(); DIL; DIL = DIL->getInlinedAt()) {
     // unsigned int to match line offset representation in remarks so
     // it's directly consumable by relay advisor.
     uint32_t Offset =
         DIL->getLine() - DIL->getScope()->getSubprogram()->getLine();
     uint32_t Discriminator = DIL->getBaseDiscriminator();
     StringRef Name = DIL->getScope()->getSubprogram()->getLinkageName();
     if (Name.empty())
       Name = DIL->getScope()->getSubprogram()->getName();
-    CallSiteLoc << Name.str() << ":" << llvm::utostr(Offset);
-    if (Discriminator) {
+    CallSiteLoc << Name.str() << ":" << llvm::utostr(Offset) << ":"
+                << llvm::utostr(DIL->getColumn());
+    if (Discriminator)
       CallSiteLoc << "." << llvm::utostr(Discriminator);
-    }
     First = false;
   }
 
   return CallSiteLoc.str();
 }
 
 void llvm::addLocationToRemarks(OptimizationRemark &Remark, DebugLoc DLoc) {
   if (!DLoc.get()) {
     return;
   }
 
   bool First = true;
   Remark << " at callsite ";
   for (DILocation *DIL = DLoc.get(); DIL; DIL = DIL->getInlinedAt()) {
     if (!First)
       Remark << " @ ";
     unsigned int Offset = DIL->getLine();
     Offset -= DIL->getScope()->getSubprogram()->getLine();
     unsigned int Discriminator = DIL->getBaseDiscriminator();
     StringRef Name = DIL->getScope()->getSubprogram()->getLinkageName();
     if (Name.empty())
       Name = DIL->getScope()->getSubprogram()->getName();
-    Remark << Name << ":" << ore::NV("Line", Offset);
+    Remark << Name << ":" << ore::NV("Line", Offset) << ":"
+           << ore::NV("Column", DIL->getColumn());
     if (Discriminator)
       Remark << "." << ore::NV("Disc", Discriminator);
     First = false;
   }
+
+  Remark << ";";
 }
 
 void llvm::emitInlinedInto(OptimizationRemarkEmitter &ORE, DebugLoc DLoc,
                            const BasicBlock *Block, const Function &Callee,
                            const Function &Caller, const InlineCost &IC,
                            bool ForProfileContext, const char *PassName) {
   ORE.emit([&]() {
     bool AlwaysInline = IC.isAlways();

llvm/lib/Analysis/ReplayInlineAdvisor.cpp

 #include "llvm/Support/LineIterator.h"
 
 using namespace llvm;
 
 #define DEBUG_TYPE "inline-replay"
 
 ReplayInlineAdvisor::ReplayInlineAdvisor(FunctionAnalysisManager &FAM,
                                          LLVMContext &Context,
-                                         StringRef RemarksFile)
-    : InlineAdvisor(FAM), HasReplayRemarks(false) {
+                                         StringRef RemarksFile,
+                                         bool EmitRemarks)
+    : InlineAdvisor(FAM), HasReplayRemarks(false), EmitRemarks(EmitRemarks) {
   auto BufferOrErr = MemoryBuffer::getFileOrSTDIN(RemarksFile);
   std::error_code EC = BufferOrErr.getError();
   if (EC) {
     Context.emitError("Could not open remarks file: " + EC.message());
     return;
   }
 
   // Example for inline remarks to parse:
-  //   _Z3subii inlined into main [details] at callsite sum:1 @ main:3.1
+  //   main:3:1.1: _Z3subii inlined into main at callsite sum:1 @ main:3:1.1
   // We use the callsite string after `at callsite` to replay inlining.
   line_iterator LineIt(*BufferOrErr.get(), /*SkipBlanks=*/true);
   for (; !LineIt.is_at_eof(); ++LineIt) {
     StringRef Line = *LineIt;
     auto Pair = Line.split(" at callsite ");
-    if (Pair.second.empty())
+
+    auto Callee = Pair.first.split(" inlined into").first.rsplit(": ").second;
+
+    auto CallSite = Pair.second.split(";").first;
+
+    if (Callee.empty() || CallSite.empty())
       continue;
-    InlineSitesFromRemarks.insert(Pair.second);
+
+    std::string Combined = (Callee + CallSite).str();
+    InlineSitesFromRemarks.insert(Combined);
   }
+
   HasReplayRemarks = true;
 }
 
 std::unique_ptr<InlineAdvice> ReplayInlineAdvisor::getAdvice(CallBase &CB) {
   assert(HasReplayRemarks);
 
   Function &Caller = *CB.getCaller();
   auto &ORE = FAM.getResult<OptimizationRemarkEmitterAnalysis>(Caller);
 
   if (InlineSitesFromRemarks.empty())
-    return std::make_unique<InlineAdvice>(this, CB, ORE, false);
+    return std::make_unique<DefaultInlineAdvice>(this, CB, None, ORE,
+                                                 EmitRemarks);
 
   std::string CallSiteLoc = getCallSiteLocation(CB.getDebugLoc());
-  bool InlineRecommended = InlineSitesFromRemarks.count(CallSiteLoc) > 0;
-  return std::make_unique<InlineAdvice>(this, CB, ORE, InlineRecommended);
+  StringRef Callee = CB.getCalledFunction()->getName();
+  std::string Combined = (Callee + CallSiteLoc).str();
+  auto Iter = InlineSitesFromRemarks.find(Combined);
+
+  Optional<InlineCost> InlineRecommended = None;
+  if (Iter != InlineSitesFromRemarks.end()) {
+    InlineRecommended = llvm::InlineCost::getAlways("found in replay");
+  }
+
+  return std::make_unique<DefaultInlineAdvice>(this, CB, InlineRecommended, ORE,
+                                               EmitRemarks);
 }

llvm/lib/Transforms/IPO/SampleProfile.cpp

bool SampleProfileLoader::doInitialization(Module &M,
   if (ProfAccForSymsInList) {
     NamesInProfile.clear();
     if (auto NameTable = Reader->getNameTable())
       NamesInProfile.insert(NameTable->begin(), NameTable->end());
   }
 
   if (FAM && !ProfileInlineReplayFile.empty()) {
     ExternalInlineAdvisor = std::make_unique<ReplayInlineAdvisor>(
-        *FAM, Ctx, ProfileInlineReplayFile);
+        *FAM, Ctx, ProfileInlineReplayFile, /*EmitRemarks=*/false);
     if (!ExternalInlineAdvisor->areReplayRemarksLoaded())
       ExternalInlineAdvisor.reset();
   }
 
   // Apply tweaks if context-sensitive profile is available.
   if (Reader->profileIsCS()) {
     ProfileIsCS = true;
     FunctionSamples::ProfileIsCS = true;

llvm/test/Transforms/Inline/optimization-remarks-passed-yaml.ll

 ; is the input:
 
 ;  1     int foo() { return 1; }
 ;  2
 ;  3     int bar() {
 ;  4       return foo();
 ;  5     }
 
-; CHECK: remark: /tmp/s.c:4:10: foo inlined into bar with (cost={{[0-9\-]+}}, threshold={{[0-9]+}}) at callsite bar:1 (hotness: 30)
+; CHECK: remark: /tmp/s.c:4:10: foo inlined into bar with (cost={{[0-9\-]+}}, threshold={{[0-9]+}}) at callsite bar:1:10; (hotness: 30)
 
 ; YAML:      --- !Passed
 ; YAML-NEXT: Pass:            inline
 ; YAML-NEXT: Name:            Inlined
 ; YAML-NEXT: DebugLoc:        { File: '/tmp/s.c', Line: 4, Column: 10 }
 ; YAML-NEXT: Function:        bar
 ; YAML-NEXT: Hotness:         30
 ; YAML-NEXT: Args:
 ; YAML-NEXT:   - Callee: foo
 ; YAML-NEXT:     DebugLoc:        { File: '/tmp/s.c', Line: 1, Column: 0 }
 ; YAML-NEXT:   - String: ' inlined into '
 ; YAML-NEXT:   - Caller: bar
 ; YAML-NEXT:     DebugLoc:        { File: '/tmp/s.c', Line: 3, Column: 0 }
 ; YAML-NEXT:   - String: ' with '
 ; YAML-NEXT:   - String: '(cost='
 ; YAML-NEXT:   - Cost: '{{[0-9\-]+}}'
 ; YAML-NEXT:   - String: ', threshold='
 ; YAML-NEXT:   - Threshold: '{{[0-9]+}}'
 ; YAML-NEXT:   - String: ')'
 ; YAML-NEXT:   - String:          ' at callsite '
 ; YAML-NEXT:   - String:          bar
 ; YAML-NEXT:   - String:          ':'
 ; YAML-NEXT:   - Line:            '1'
+; YAML-NEXT:   - String:          ':'
+; YAML-NEXT:   - Column:          '10'
+; YAML-NEXT:   - String:          ';'
 ; YAML-NEXT: ...
 
 ; ModuleID = '/tmp/s.c'
 source_filename = "/tmp/s.c"
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 
 ; Function Attrs: nounwind ssp uwtable

llvm/test/Transforms/SampleProfile/Inputs/inline-replay.txt

-remark: calls.cc:10:0: _Z3sumii inlined into main to match profiling context with (cost=45, threshold=337) at callsite main:3.1
-remark: calls.cc:4:0: _Z3subii inlined into main to match profiling context with (cost=-5, threshold=337) at callsite _Z3sumii:1 @ main:3.1
+remark: calls.cc:10:0: _Z3sumii inlined into main to match profiling context with (cost=45, threshold=337) at callsite main:3:0.1;
+remark: calls.cc:4:0: _Z3subii inlined into main to match profiling context with (cost=-5, threshold=337) at callsite _Z3sumii:1:0 @ main:3:0.1;

llvm/test/Transforms/SampleProfile/inline-replay.ll

 
 
 ; DEFAULT: _Z3sumii inlined into main
 ; DEFAULT: _Z3subii inlined into _Z3sumii
 ; DEFAULT-NOT: _Z3subii inlined into main
 
 ; REPLAY: _Z3sumii inlined into main
 ; REPLAY: _Z3subii inlined into main
-; REPLA-NOT: _Z3subii inlined into _Z3sumii
+; REPLAY-NOT: _Z3subii inlined into _Z3sumii

llvm/test/Transforms/SampleProfile/remarks-hotness.ll

 
 ; YAML-MISS:      --- !Missed
 ; YAML-MISS-NEXT: Pass:            inline
 ; YAML-MISS-NEXT: Name:            NeverInline
 ; YAML-MISS-NEXT: DebugLoc:        { File: remarks-hotness.cpp, Line: 14, Column: 10 }
 ; 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'
 source_filename = "remarks-hotness.cpp"
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 ; Function Attrs: use-sample-profile

 !12 = distinct !DISubprogram(name: "callee2", linkageName: "_Z7callee2v", scope: !1, file: !1, line: 5, type: !8, scopeLine: 5, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
 !13 = !DILocation(line: 6, column: 3, scope: !12)
 !14 = distinct !DISubprogram(name: "caller1", linkageName: "_Z7caller1v", scope: !1, file: !1, line: 9, type: !8, scopeLine: 9, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !2)
 !15 = !DILocation(line: 10, column: 10, scope: !14)
 !16 = !DILocation(line: 10, column: 3, scope: !14)
 !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)
-

llvm/test/Transforms/SampleProfile/remarks.ll

 ;     9          sum += -i * rand();
 ;    10      return sum;
 ;    11    }
 ;    12
 ;    13    int main() { return foo() > 0; }
 
 ; 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
 
 ; The predicate almost always chooses the 'else' branch.
 ; CHECK: remark: remarks.cc:9:15: most popular destination for conditional branches at remarks.cc:6:9
 
 ; Checking to see if YAML file is generated and contains remarks

 ;YAML-NEXT:    - Cost:            '130'
 ;YAML-NEXT:    - String:          ', threshold='
 ;YAML-NEXT:    - Threshold:       '225'
 ;YAML-NEXT:    - String:          ')'
 ;YAML-NEXT:    - String:          ' at callsite '
 ;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
 ;YAML-NEXT:  Name:            AlwaysInline
 ;YAML-NEXT:  DebugLoc:        { File: remarks.cc, Line: 9, Column: 19 }
 ;YAML-NEXT:  Function:        main
 ;YAML-NEXT:  Args:
 ;YAML-NEXT:    - Callee:          rand
 ;YAML-NEXT:      DebugLoc:        { File: remarks.cc, Line: 90, Column: 0 }
 ;YAML-NEXT:    - String:          ' inlined into '
 ;YAML-NEXT:    - Caller:          main
 ;YAML-NEXT:      DebugLoc:        { File: remarks.cc, Line: 13, Column: 0 }
 ;YAML-NEXT:    - String:          ' to match profiling context'
 ;YAML-NEXT:    - String:          ' with '
 ;YAML-NEXT:    - String:          '(cost=always)'
 ;YAML-NEXT:    - String:          ': '
 ;YAML-NEXT:    - Reason:          always inline attribute
 ;YAML-NEXT:    - String:          ' at callsite '
 ;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
 ;YAML-NEXT:  DebugLoc:        { File: remarks.cc, Line: 5, Column: 8 }
 ;YAML-NEXT:  Function:        main
 ;YAML-NEXT:  Args:
 ;YAML-NEXT:    - String:          'Applied '
 ;YAML-NEXT:    - NumSamples:      '18305'