From db29f20fdd4f715553f663f21021330cb4497e00 Mon Sep 17 00:00:00 2001 From: Amir Ayupov Date: Wed, 8 May 2024 12:02:18 -0700 Subject: [PATCH] [BOLT] Ignore returns in DataAggregator Returns are ignored in perf/pre-aggregated/fdata profile reader (see DataReader::convertBranchData). They are also omitted in YAMLProfileWriter by virtue of not having the profile attached to them in the reader, and YAMLProfileWriter converting the profile attached to BinaryFunctions. Thus, return profile is universally ignored across all profile types except BAT YAML. To make returns ignored for YAML produced in BAT mode, we can: 1) ignore them in YAMLProfileReader, 2) omit them from YAML profile in profile conversion/writing. The first option is prone to profile staleness issue, where the profiled binary doesn't match the one to be optimized, and thus returns in the profile can no longer be reliably detected (as we don't distinguish them from calls in the profile). The second option is robust to staleness but requires disassembling the branch source instruction. Test Plan: Updated bolt-address-translation-yaml.test Reviewers: rafaelauler, dcci, ayermolo, maksfb Reviewed By: maksfb Pull Request: https://github.com/llvm/llvm-project/pull/90807 --- bolt/include/bolt/Core/BinaryFunction.h | 2 ++ bolt/lib/Core/BinaryFunction.cpp | 15 +++++++++++++++ bolt/lib/Profile/DataAggregator.cpp | 13 +++++++++++++ bolt/test/X86/bolt-address-translation-yaml.test | 5 +++-- 4 files changed, 33 insertions(+), 2 deletions(-) diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h index 26d2d01f8626..3c641581e247 100644 --- a/bolt/include/bolt/Core/BinaryFunction.h +++ b/bolt/include/bolt/Core/BinaryFunction.h @@ -930,6 +930,8 @@ public: return const_cast(this)->getInstructionAtOffset(Offset); } + std::optional disassembleInstructionAtOffset(uint64_t Offset) const; + /// Return offset for the first instruction. If there is data at the /// beginning of a function then offset of the first instruction could /// be different from 0 diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp index 1fa96dfaabde..de34421ebeb0 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -1167,6 +1167,21 @@ void BinaryFunction::handleAArch64IndirectCall(MCInst &Instruction, } } +std::optional +BinaryFunction::disassembleInstructionAtOffset(uint64_t Offset) const { + assert(CurrentState == State::Empty && "Function should not be disassembled"); + assert(Offset < MaxSize && "Invalid offset"); + ErrorOr> FunctionData = getData(); + assert(FunctionData && "Cannot get function as data"); + MCInst Instr; + uint64_t InstrSize = 0; + const uint64_t InstrAddress = getAddress() + Offset; + if (BC.DisAsm->getInstruction(Instr, InstrSize, FunctionData->slice(Offset), + InstrAddress, nulls())) + return Instr; + return std::nullopt; +} + Error BinaryFunction::disassemble() { NamedRegionTimer T("disassemble", "Disassemble function", "buildfuncs", "Build Binary Functions", opts::TimeBuild); diff --git a/bolt/lib/Profile/DataAggregator.cpp b/bolt/lib/Profile/DataAggregator.cpp index 5108392c824c..d02e4499014e 100644 --- a/bolt/lib/Profile/DataAggregator.cpp +++ b/bolt/lib/Profile/DataAggregator.cpp @@ -773,9 +773,19 @@ 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)); + } if (BAT) Addr = BAT->translate(Func->getAddress(), Addr, IsFrom); @@ -792,6 +802,9 @@ bool DataAggregator::doBranch(uint64_t From, uint64_t To, uint64_t Count, }; BinaryFunction *FromFunc = handleAddress(From, /*IsFrom=*/true); + // Ignore returns. + if (IsReturn) + return true; BinaryFunction *ToFunc = handleAddress(To, /*IsFrom=*/false); if (!FromFunc && !ToFunc) return false; diff --git a/bolt/test/X86/bolt-address-translation-yaml.test b/bolt/test/X86/bolt-address-translation-yaml.test index af24c3d84a0f..b3d8a8839450 100644 --- a/bolt/test/X86/bolt-address-translation-yaml.test +++ b/bolt/test/X86/bolt-address-translation-yaml.test @@ -13,7 +13,7 @@ RUN: llvm-bolt %t.exe -data %t.fdata -w %t.yaml-fdata -o /dev/null RUN: FileCheck --input-file %t.yaml-fdata --check-prefix YAML-BAT-CHECK %s # Test resulting YAML profile with the original binary (no-stale mode) -RUN: llvm-bolt %t.exe -data %t.yaml -o %t.null -dyno-stats \ +RUN: llvm-bolt %t.exe -data %t.yaml -o %t.null -dyno-stats 2>&1 \ RUN: | FileCheck --check-prefix CHECK-BOLT-YAML %s WRITE-BAT-CHECK: BOLT-INFO: Wrote 5 BAT maps @@ -63,7 +63,8 @@ YAML-BAT-CHECK-NEXT: blocks: YAML-BAT-CHECK: - bid: 1 YAML-BAT-CHECK-NEXT: insns: [[#]] YAML-BAT-CHECK-NEXT: hash: 0xD70DC695320E0010 -YAML-BAT-CHECK-NEXT: succ: {{.*}} { bid: 2, cnt: [[#]] } +YAML-BAT-CHECK-NEXT: succ: {{.*}} { bid: 2, cnt: [[#]] CHECK-BOLT-YAML: pre-processing profile using YAML profile reader CHECK-BOLT-YAML-NEXT: 5 out of 16 functions in the binary (31.2%) have non-empty execution profile +CHECK-BOLT-YAML-NOT: invalid (possibly stale) profile