-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[BOLT] Set call to continuation count in pre-aggregated profile #109486
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
Merged
aaupov
merged 12 commits into
main
from
users/aaupov/spr/bolttest-add-callcont-fallthrutest
Nov 8, 2024
Merged
Changes from 5 commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
e713939
[𝘀𝗽𝗿] initial version
aaupov 5f48b92
repurpose for the fix of call cont discontinuity
aaupov 9741297
clang-format
aaupov 9c4effa
Drop changes in doTrace/getFallthroughsInTrace
aaupov 9e4dd66
Handle external origin LBR (non-BAT mode)
aaupov 06fe34d
Added plt call (return from external location) test case
aaupov e8ec9c9
Add test for getFallthroughsInTrace
aaupov 9ac54dd
Use return profile conversion for pre-aggregated profile only
aaupov a8e1c54
address comments
aaupov 94991b2
Drop call check
aaupov 9b3db9b
Drop = from comment
aaupov b23bdb2
clang-format
aaupov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -775,41 +775,85 @@ bool DataAggregator::doInterBranch(BinaryFunction *FromFunc, | |
|
|
||
| bool DataAggregator::doBranch(uint64_t From, uint64_t To, uint64_t Count, | ||
| uint64_t Mispreds) { | ||
| bool IsReturn = false; | ||
| auto handleAddress = [&](uint64_t &Addr, bool IsFrom) -> BinaryFunction * { | ||
| if (BinaryFunction *Func = getBinaryFunctionContainingAddress(Addr)) { | ||
| Addr -= Func->getAddress(); | ||
| if (IsFrom) { | ||
| auto checkReturn = [&](auto MaybeInst) { | ||
| IsReturn = MaybeInst && BC->MIB->isReturn(*MaybeInst); | ||
| }; | ||
| if (Func->hasInstructions()) | ||
| checkReturn(Func->getInstructionAtOffset(Addr)); | ||
| else | ||
| checkReturn(Func->disassembleInstructionAtOffset(Addr)); | ||
| } | ||
| // Returns whether \p Offset in \p Func contains a return instruction. | ||
| auto checkReturn = [&](const BinaryFunction &Func, const uint64_t Offset) { | ||
| auto isReturn = [&](auto MI) { return MI && BC->MIB->isReturn(*MI); }; | ||
| return Func.hasInstructions() | ||
| ? isReturn(Func.getInstructionAtOffset(Offset)) | ||
| : isReturn(Func.disassembleInstructionAtOffset(Offset)); | ||
| }; | ||
|
|
||
| if (BAT) | ||
| Addr = BAT->translate(Func->getAddress(), Addr, IsFrom); | ||
| // Returns whether \p Offset in \p Func corresponds to a call continuation | ||
| // fallthrough block. | ||
| auto checkCallCont = [&](BinaryFunction &Func, const uint64_t Offset) { | ||
aaupov marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| // Note the use of MCInstrAnalysis: no call continuation for a tail call. | ||
| auto isCall = [&](auto MI) { return MI && BC->MIA->isCall(*MI); }; | ||
|
|
||
| // No call continuation at a function start. | ||
| if (!Offset) | ||
| return false; | ||
|
|
||
| // FIXME: support BAT case where the function might be in empty state | ||
aaupov marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // (split fragments declared non-simple). | ||
| if (!Func.hasCFG()) | ||
| return false; | ||
|
|
||
| // The offset should not be an entry point or a landing pad. | ||
| const BinaryBasicBlock *ContBB = Func.getBasicBlockAtOffset(Offset); | ||
| if (!ContBB || ContBB->isEntryPoint() || ContBB->isLandingPad()) | ||
| return false; | ||
|
|
||
| // Check that preceding instruction is a call. | ||
| const BinaryBasicBlock *CallBB = | ||
| Func.getBasicBlockContainingOffset(Offset - 1); | ||
| if (!CallBB || CallBB == ContBB) | ||
| return false; | ||
| return isCall(CallBB->getLastNonPseudoInstr()); | ||
| }; | ||
|
|
||
| if (BinaryFunction *ParentFunc = getBATParentFunction(*Func)) { | ||
| Func = ParentFunc; | ||
| if (IsFrom) | ||
| NumColdSamples += Count; | ||
| } | ||
| // Mutates \p Addr to an offset into the containing function, performing BAT | ||
| // offset translation and parent lookup. | ||
| // | ||
| // Returns the containing function (or BAT parent) and whether the address | ||
| // corresponds to a return (if \p IsFrom) or a call continuation (otherwise). | ||
| auto handleAddress = [&](uint64_t &Addr, bool IsFrom) { | ||
| BinaryFunction *Func = getBinaryFunctionContainingAddress(Addr); | ||
| if (!Func) | ||
| return std::pair{Func, false}; | ||
|
|
||
| return Func; | ||
| } | ||
| return nullptr; | ||
| Addr -= Func->getAddress(); | ||
|
|
||
| bool IsRetOrCallCont = | ||
| IsFrom ? checkReturn(*Func, Addr) : checkCallCont(*Func, Addr); | ||
|
|
||
| if (BAT) | ||
| Addr = BAT->translate(Func->getAddress(), Addr, IsFrom); | ||
|
|
||
| BinaryFunction *ParentFunc = getBATParentFunction(*Func); | ||
| if (!ParentFunc) | ||
| return std::pair{Func, IsRetOrCallCont}; | ||
|
|
||
| if (IsFrom) | ||
| NumColdSamples += Count; | ||
|
|
||
| return std::pair{ParentFunc, IsRetOrCallCont}; | ||
| }; | ||
|
|
||
| BinaryFunction *FromFunc = handleAddress(From, /*IsFrom=*/true); | ||
| uint64_t ToOrig = To; | ||
| auto [FromFunc, IsReturn] = handleAddress(From, /*IsFrom=*/true); | ||
| auto [ToFunc, IsCallCont] = handleAddress(To, /*IsFrom=*/false); | ||
| if (!FromFunc && !ToFunc) | ||
| return false; | ||
|
|
||
| // Record call to continuation trace. | ||
| if (IsCallCont && FromFunc != ToFunc) { | ||
| LBREntry First{ToOrig - 1, ToOrig - 1, false}; | ||
| LBREntry Second{ToOrig, ToOrig, false}; | ||
| return doTrace(First, Second, Count); | ||
| } | ||
| // Ignore returns. | ||
| if (IsReturn) | ||
| return true; | ||
| BinaryFunction *ToFunc = handleAddress(To, /*IsFrom=*/false); | ||
| if (!FromFunc && !ToFunc) | ||
| return false; | ||
|
|
||
| // Treat recursive control transfers as inter-branches. | ||
| if (FromFunc == ToFunc && To != 0) { | ||
|
|
@@ -826,10 +870,19 @@ bool DataAggregator::doTrace(const LBREntry &First, const LBREntry &Second, | |
| BinaryFunction *ToFunc = getBinaryFunctionContainingAddress(Second.From); | ||
| if (!FromFunc || !ToFunc) { | ||
| LLVM_DEBUG({ | ||
| dbgs() << "Out of range trace starting in " << FromFunc->getPrintName() | ||
| << formatv(" @ {0:x}", First.To - FromFunc->getAddress()) | ||
| << " and ending in " << ToFunc->getPrintName() | ||
| << formatv(" @ {0:x}\n", Second.From - ToFunc->getAddress()); | ||
| dbgs() << "Out of range trace starting in "; | ||
| if (FromFunc) | ||
| dbgs() << formatv("{0} @ {1:x}", *FromFunc, | ||
| First.To - FromFunc->getAddress()); | ||
| else | ||
| dbgs() << Twine::utohexstr(First.To); | ||
| dbgs() << " and ending in "; | ||
| if (ToFunc) | ||
| dbgs() << formatv("{0} @ {1:x}", *ToFunc, | ||
| Second.From - ToFunc->getAddress()); | ||
| else | ||
| dbgs() << Twine::utohexstr(Second.From); | ||
| dbgs() << '\n'; | ||
| }); | ||
| NumLongRangeTraces += Count; | ||
| return false; | ||
|
|
@@ -903,24 +956,6 @@ DataAggregator::getFallthroughsInTrace(BinaryFunction &BF, | |
| if (!FromBB || !ToBB) | ||
| return std::nullopt; | ||
|
|
||
| // Adjust FromBB if the first LBR is a return from the last instruction in | ||
|
Contributor
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. For reference to aid in review the code being removed here comes from 5599c01 |
||
| // the previous block (that instruction should be a call). | ||
| if (From == FromBB->getOffset() && !BF.containsAddress(FirstLBR.From) && | ||
| !FromBB->isEntryPoint() && !FromBB->isLandingPad()) { | ||
| const BinaryBasicBlock *PrevBB = | ||
| BF.getLayout().getBlock(FromBB->getIndex() - 1); | ||
| if (PrevBB->getSuccessor(FromBB->getLabel())) { | ||
| const MCInst *Instr = PrevBB->getLastNonPseudoInstr(); | ||
| if (Instr && BC.MIB->isCall(*Instr)) | ||
| FromBB = PrevBB; | ||
| else | ||
| LLVM_DEBUG(dbgs() << "invalid incoming LBR (no call): " << FirstLBR | ||
| << '\n'); | ||
| } else { | ||
| LLVM_DEBUG(dbgs() << "invalid incoming LBR: " << FirstLBR << '\n'); | ||
| } | ||
| } | ||
|
|
||
| // Fill out information for fall-through edges. The From and To could be | ||
| // within the same basic block, e.g. when two call instructions are in the | ||
| // same block. In this case we skip the processing. | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| ## Ensures that a call continuation fallthrough count is set when using | ||
| ## pre-aggregated perf data. | ||
|
|
||
| # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o | ||
| # RUN: %clang %cflags %t.o -o %t.exe -Wl,-q -nostdlib | ||
| # RUN: link_fdata %s %t.exe %t.pa PREAGG | ||
| # RUN: llvm-strip --strip-unneeded %t.exe | ||
| # RUN: llvm-bolt %t.exe --pa -p %t.pa -o %t.out \ | ||
| # RUN: --print-cfg --print-only=main | FileCheck %s | ||
|
|
||
| .globl foo | ||
| .type foo, %function | ||
| foo: | ||
| pushq %rbp | ||
| movq %rsp, %rbp | ||
| popq %rbp | ||
| Lfoo_ret: | ||
| retq | ||
| .size foo, .-foo | ||
|
|
||
| .globl main | ||
| .type main, %function | ||
| main: | ||
| pushq %rbp | ||
| movq %rsp, %rbp | ||
| subq $0x20, %rsp | ||
| movl $0x0, -0x4(%rbp) | ||
| movl %edi, -0x8(%rbp) | ||
| movq %rsi, -0x10(%rbp) | ||
| movq -0x10(%rbp), %rax | ||
| movq 0x8(%rax), %rdi | ||
| movl %eax, -0x14(%rbp) | ||
|
|
||
| Ltmp4: | ||
| cmpl $0x0, -0x14(%rbp) | ||
| je Ltmp0 | ||
|
|
||
| movl $0xa, -0x18(%rbp) | ||
| callq foo | ||
| # PREAGG: B #Lfoo_ret# #Ltmp3# 1 0 | ||
| # CHECK: callq foo | ||
| # CHECK-NEXT: count: 1 | ||
|
|
||
| Ltmp3: | ||
| cmpl $0x0, -0x18(%rbp) | ||
| jmp Ltmp2 | ||
|
|
||
| Ltmp2: | ||
| movl -0x18(%rbp), %eax | ||
| addl $-0x1, %eax | ||
| movl %eax, -0x18(%rbp) | ||
| jmp Ltmp3 | ||
| jmp Ltmp4 | ||
|
|
||
| Ltmp0: | ||
| xorl %eax, %eax | ||
| addq $0x20, %rsp | ||
| popq %rbp | ||
| retq | ||
| .size main, .-main |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.