From 29a95ae466af4dfa0ed1ced42d44b44fbbeb7559 Mon Sep 17 00:00:00 2001 From: Paul Kirth Date: Thu, 25 Apr 2024 23:10:42 +0000 Subject: [PATCH 1/4] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20ch?= =?UTF-8?q?anges=20to=20main=20this=20commit=20is=20based=20on?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Created using spr 1.3.4 [skip ci] --- .../attr-likelihood-if-vs-builtin-expect.cpp | 4 +- llvm/docs/BranchWeightMetadata.rst | 7 ++ llvm/include/llvm/IR/MDBuilder.h | 11 ++- llvm/include/llvm/IR/ProfDataUtils.h | 28 ++++++- llvm/lib/CodeGen/CodeGenPrepare.cpp | 3 +- llvm/lib/IR/Instruction.cpp | 17 +++- llvm/lib/IR/Instructions.cpp | 6 +- llvm/lib/IR/MDBuilder.cpp | 14 ++-- llvm/lib/IR/ProfDataUtils.cpp | 77 +++++++++++++------ llvm/lib/IR/Verifier.cpp | 9 ++- llvm/lib/Transforms/IPO/SampleProfile.cpp | 7 +- .../ControlHeightReduction.cpp | 2 +- .../Instrumentation/IndirectCallPromotion.cpp | 3 +- .../Instrumentation/PGOInstrumentation.cpp | 5 +- llvm/lib/Transforms/Scalar/JumpThreading.cpp | 4 +- .../Scalar/LowerExpectIntrinsic.cpp | 16 ++-- llvm/lib/Transforms/Utils/Local.cpp | 2 +- llvm/lib/Transforms/Utils/LoopPeel.cpp | 4 +- .../Transforms/Utils/LoopRotationUtils.cpp | 6 +- llvm/lib/Transforms/Utils/MisExpect.cpp | 22 +++--- llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 30 ++++---- .../Transforms/Vectorize/LoopVectorize.cpp | 12 +-- .../Transforms/LowerExpectIntrinsic/basic.ll | 8 +- .../expect-with-probability.ll | 8 +- .../LowerExpectIntrinsic/expect_nonboolean.ll | 5 +- .../LowerExpectIntrinsic/phi_merge.ll | 4 +- .../Transforms/LowerExpectIntrinsic/phi_or.ll | 4 +- .../LowerExpectIntrinsic/phi_tern.ll | 2 +- .../LowerExpectIntrinsic/phi_unexpect.ll | 4 +- 29 files changed, 209 insertions(+), 115 deletions(-) diff --git a/clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp b/clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp index fb236aeb982e0..81d9334356520 100644 --- a/clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp +++ b/clang/test/CodeGenCXX/attr-likelihood-if-vs-builtin-expect.cpp @@ -221,5 +221,5 @@ void tu2(int &i) { } } -// CHECK: [[BW_LIKELY]] = !{!"branch_weights", i32 2000, i32 1} -// CHECK: [[BW_UNLIKELY]] = !{!"branch_weights", i32 1, i32 2000} +// CHECK: [[BW_LIKELY]] = !{!"branch_weights", !"expected", i32 2000, i32 1} +// CHECK: [[BW_UNLIKELY]] = !{!"branch_weights", !"expected", i32 1, i32 2000} diff --git a/llvm/docs/BranchWeightMetadata.rst b/llvm/docs/BranchWeightMetadata.rst index 522f37cdad4fc..62204753e29b0 100644 --- a/llvm/docs/BranchWeightMetadata.rst +++ b/llvm/docs/BranchWeightMetadata.rst @@ -28,11 +28,14 @@ Supported Instructions Metadata is only assigned to the conditional branches. There are two extra operands for the true and the false branch. +We optionally track if the metadata was added by ``__builtin_expect`` or +``__builtin_expect_with_probability`` with an optional field ``!"expected"``. .. code-block:: none !0 = !{ !"branch_weights", + [ !"expected", ] i32 , i32 } @@ -47,6 +50,7 @@ is always case #0). !0 = !{ !"branch_weights", + [ !"expected", ] i32 [ , i32 ... ] } @@ -60,6 +64,7 @@ Branch weights are assigned to every destination. !0 = !{ !"branch_weights", + [ !"expected", ] i32 [ , i32 ... ] } @@ -75,6 +80,7 @@ block and entry counts which may not be accurate with sampling. !0 = !{ !"branch_weights", + [ !"expected", ] i32 } @@ -95,6 +101,7 @@ is used. !0 = !{ !"branch_weights", + [ !"expected", ] i32 [ , i32 ] } diff --git a/llvm/include/llvm/IR/MDBuilder.h b/llvm/include/llvm/IR/MDBuilder.h index 3265589b7c8df..e02ec8f5a3d8b 100644 --- a/llvm/include/llvm/IR/MDBuilder.h +++ b/llvm/include/llvm/IR/MDBuilder.h @@ -59,7 +59,11 @@ class MDBuilder { //===------------------------------------------------------------------===// /// Return metadata containing two branch weights. - MDNode *createBranchWeights(uint32_t TrueWeight, uint32_t FalseWeight); + /// @param TrueWeight the weight of the true branch + /// @param FalseWeight the weight of the false branch + /// @param Do these weights come from __builtin_expect* + MDNode *createBranchWeights(uint32_t TrueWeight, uint32_t FalseWeight, + bool IsExpected = false); /// Return metadata containing two branch weights, with significant bias /// towards `true` destination. @@ -70,7 +74,10 @@ class MDBuilder { MDNode *createUnlikelyBranchWeights(); /// Return metadata containing a number of branch weights. - MDNode *createBranchWeights(ArrayRef Weights); + /// @param Weights the weights of all the branches + /// @param Do these weights come from __builtin_expect* + MDNode *createBranchWeights(ArrayRef Weights, + bool IsExpected = false); /// Return metadata specifying that a branch or switch is unpredictable. MDNode *createUnpredictable(); diff --git a/llvm/include/llvm/IR/ProfDataUtils.h b/llvm/include/llvm/IR/ProfDataUtils.h index c0897408986fb..3c761bdc1bf3e 100644 --- a/llvm/include/llvm/IR/ProfDataUtils.h +++ b/llvm/include/llvm/IR/ProfDataUtils.h @@ -55,6 +55,17 @@ MDNode *getBranchWeightMDNode(const Instruction &I); /// Nullptr otherwise. MDNode *getValidBranchWeightMDNode(const Instruction &I); +/// Check if Branch Weight Metadata has an "expected" field from an llvm.expect* +/// intrinsic +bool hasBranchWeightProvenance(const Instruction &I); + +/// Check if Branch Weight Metadata has an "expected" field from an llvm.expect* +/// intrinsic +bool hasBranchWeightProvenance(const MDNode *ProfileData); + +/// Return the offset to the first branch weight data +unsigned getBranchWeightOffset(const MDNode *ProfileData); + /// Extract branch weights from MD_prof metadata /// /// \param ProfileData A pointer to an MDNode. @@ -65,9 +76,14 @@ bool extractBranchWeights(const MDNode *ProfileData, SmallVectorImpl &Weights); /// Faster version of extractBranchWeights() that skips checks and must only -/// be called with "branch_weights" metadata nodes. -void extractFromBranchWeightMD(const MDNode *ProfileData, - SmallVectorImpl &Weights); +/// be called with "branch_weights" metadata nodes. Supports uint32_t. +void extractFromBranchWeightMD32(const MDNode *ProfileData, + SmallVectorImpl &Weights); + +/// Faster version of extractBranchWeights() that skips checks and must only +/// be called with "branch_weights" metadata nodes. Supports uint64_t. +void extractFromBranchWeightMD64(const MDNode *ProfileData, + SmallVectorImpl &Weights); /// Extract branch weights attatched to an Instruction /// @@ -106,7 +122,11 @@ bool extractProfTotalWeight(const Instruction &I, uint64_t &TotalWeights); /// Create a new `branch_weights` metadata node and add or overwrite /// a `prof` metadata reference to instruction `I`. -void setBranchWeights(Instruction &I, ArrayRef Weights); +/// \param I the Instruction to set branch weights on. +/// \param Weights an array of weights to set on instruction I. +/// \param IsExpected were these weights added from an llvm.expect* intrinsic. +void setBranchWeights(Instruction &I, ArrayRef Weights, + bool IsExpected); /// Scaling the profile data attached to 'I' using the ratio of S/T. void scaleProfData(Instruction &I, uint64_t S, uint64_t T); diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp index 8eaf78157550e..c8429a18a4396 100644 --- a/llvm/lib/CodeGen/CodeGenPrepare.cpp +++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp @@ -8864,7 +8864,8 @@ bool CodeGenPrepare::splitBranchCondition(Function &F, ModifyDT &ModifiedDT) { scaleWeights(NewTrueWeight, NewFalseWeight); Br1->setMetadata(LLVMContext::MD_prof, MDBuilder(Br1->getContext()) - .createBranchWeights(TrueWeight, FalseWeight)); + .createBranchWeights(TrueWeight, FalseWeight, + hasBranchWeightProvenance(*Br1))); NewTrueWeight = TrueWeight; NewFalseWeight = 2 * FalseWeight; diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp index 678edc58ad848..c71a1926258a1 100644 --- a/llvm/lib/IR/Instruction.cpp +++ b/llvm/lib/IR/Instruction.cpp @@ -1266,12 +1266,21 @@ Instruction *Instruction::cloneImpl() const { void Instruction::swapProfMetadata() { MDNode *ProfileData = getBranchWeightMDNode(*this); - if (!ProfileData || ProfileData->getNumOperands() != 3) + if (!isBranchWeightMD(ProfileData)) return; - // The first operand is the name. Fetch them backwards and build a new one. - Metadata *Ops[] = {ProfileData->getOperand(0), ProfileData->getOperand(2), - ProfileData->getOperand(1)}; + SmallVector Ops; + unsigned FirstIdx = getBranchWeightOffset(ProfileData); + unsigned SecondIdx = FirstIdx + 1; + // If there are more weights past the second, we can't swap them + if (ProfileData->getNumOperands() > SecondIdx + 1) + return; + for (unsigned Idx = 0; Idx < FirstIdx; ++Idx) { + Ops.push_back(ProfileData->getOperand(Idx)); + } + // Switch the order of the weights + Ops.push_back(ProfileData->getOperand(SecondIdx)); + Ops.push_back(ProfileData->getOperand(FirstIdx)); setMetadata(LLVMContext::MD_prof, MDNode::get(ProfileData->getContext(), Ops)); } diff --git a/llvm/lib/IR/Instructions.cpp b/llvm/lib/IR/Instructions.cpp index d2babc748731a..650d32ac17fc2 100644 --- a/llvm/lib/IR/Instructions.cpp +++ b/llvm/lib/IR/Instructions.cpp @@ -5165,7 +5165,11 @@ void SwitchInstProfUpdateWrapper::init() { if (!ProfileData) return; - if (ProfileData->getNumOperands() != SI.getNumSuccessors() + 1) { + // FIXME: This check belongs in ProfDataUtils. Its almost equivalent to + // getValidBranchWeightMDNode(), but the need to use llvm_unreachable + // makes them slightly different. + if (ProfileData->getNumOperands() != + SI.getNumSuccessors() + getBranchWeightOffset(ProfileData)) { llvm_unreachable("number of prof branch_weights metadata operands does " "not correspond to number of succesors"); } diff --git a/llvm/lib/IR/MDBuilder.cpp b/llvm/lib/IR/MDBuilder.cpp index 0bf41d7cc7c2c..2a03337724bf4 100644 --- a/llvm/lib/IR/MDBuilder.cpp +++ b/llvm/lib/IR/MDBuilder.cpp @@ -35,8 +35,8 @@ MDNode *MDBuilder::createFPMath(float Accuracy) { } MDNode *MDBuilder::createBranchWeights(uint32_t TrueWeight, - uint32_t FalseWeight) { - return createBranchWeights({TrueWeight, FalseWeight}); + uint32_t FalseWeight, bool IsExpected) { + return createBranchWeights({TrueWeight, FalseWeight}, IsExpected); } MDNode *MDBuilder::createLikelyBranchWeights() { @@ -49,15 +49,19 @@ MDNode *MDBuilder::createUnlikelyBranchWeights() { return createBranchWeights(1, (1U << 20) - 1); } -MDNode *MDBuilder::createBranchWeights(ArrayRef Weights) { +MDNode *MDBuilder::createBranchWeights(ArrayRef Weights, + bool IsExpected) { assert(Weights.size() >= 1 && "Need at least one branch weights!"); - SmallVector Vals(Weights.size() + 1); + unsigned int Offset = IsExpected ? 2 : 1; + SmallVector Vals(Weights.size() + Offset); Vals[0] = createString("branch_weights"); + if (IsExpected) + Vals[1] = createString("expected"); Type *Int32Ty = Type::getInt32Ty(Context); for (unsigned i = 0, e = Weights.size(); i != e; ++i) - Vals[i + 1] = createConstant(ConstantInt::get(Int32Ty, Weights[i])); + Vals[i + Offset] = createConstant(ConstantInt::get(Int32Ty, Weights[i])); return MDNode::get(Context, Vals); } diff --git a/llvm/lib/IR/ProfDataUtils.cpp b/llvm/lib/IR/ProfDataUtils.cpp index dc86f4204b1a1..cd219c22e3dfe 100644 --- a/llvm/lib/IR/ProfDataUtils.cpp +++ b/llvm/lib/IR/ProfDataUtils.cpp @@ -40,9 +40,6 @@ namespace { // We maintain some constants here to ensure that we access the branch weights // correctly, and can change the behavior in the future if the layout changes -// The index at which the weights vector starts -constexpr unsigned WeightsIdx = 1; - // the minimum number of operands for MD_prof nodes with branch weights constexpr unsigned MinBWOps = 3; @@ -65,6 +62,27 @@ bool isTargetMD(const MDNode *ProfData, const char *Name, unsigned MinOps) { return ProfDataName->getString().equals(Name); } +template >> +static void extractFromBranchWeightMD(const MDNode *ProfileData, + SmallVectorImpl &Weights) { + assert(isBranchWeightMD(ProfileData) && "wrong metadata"); + + unsigned NOps = ProfileData->getNumOperands(); + unsigned WeightsIdx = getBranchWeightOffset(ProfileData); + assert(WeightsIdx < NOps && "Weights Index must be less than NOps."); + Weights.resize(NOps - WeightsIdx); + + for (unsigned Idx = WeightsIdx, E = NOps; Idx != E; ++Idx) { + ConstantInt *Weight = + mdconst::dyn_extract(ProfileData->getOperand(Idx)); + assert(Weight && "Malformed branch_weight in MD_prof node"); + assert(Weight->getValue().getActiveBits() <= (sizeof(T) * 8) && + "Too many bits for MD_prof branch_weight"); + Weights[Idx - WeightsIdx] = Weight->getZExtValue(); + } +} + } // namespace namespace llvm { @@ -86,6 +104,25 @@ bool hasValidBranchWeightMD(const Instruction &I) { return getValidBranchWeightMDNode(I); } +bool hasBranchWeightProvenance(const Instruction &I) { + auto *ProfileData = I.getMetadata(LLVMContext::MD_prof); + return hasBranchWeightProvenance(ProfileData); +} + +bool hasBranchWeightProvenance(const MDNode *ProfileData) { + if (!isBranchWeightMD(ProfileData)) + return false; + auto *ProfDataName = dyn_cast(ProfileData->getOperand(1)); + // NOTE: if we ever have more types of branch weight provenance, + // we need to check the string value is "expected". For now, we + // supply a more generic API, and avoid the spurious comparisons. + return ProfDataName; +} + +unsigned getBranchWeightOffset(const MDNode *ProfileData) { + return hasBranchWeightProvenance(ProfileData) ? 2 : 1; +} + MDNode *getBranchWeightMDNode(const Instruction &I) { auto *ProfileData = I.getMetadata(LLVMContext::MD_prof); if (!isBranchWeightMD(ProfileData)) @@ -95,27 +132,21 @@ MDNode *getBranchWeightMDNode(const Instruction &I) { MDNode *getValidBranchWeightMDNode(const Instruction &I) { auto *ProfileData = getBranchWeightMDNode(I); - if (ProfileData && ProfileData->getNumOperands() == 1 + I.getNumSuccessors()) + auto Offset = getBranchWeightOffset(ProfileData); + if (ProfileData && + ProfileData->getNumOperands() == Offset + I.getNumSuccessors()) return ProfileData; return nullptr; } -void extractFromBranchWeightMD(const MDNode *ProfileData, - SmallVectorImpl &Weights) { - assert(isBranchWeightMD(ProfileData) && "wrong metadata"); - - unsigned NOps = ProfileData->getNumOperands(); - assert(WeightsIdx < NOps && "Weights Index must be less than NOps."); - Weights.resize(NOps - WeightsIdx); +void extractFromBranchWeightMD32(const MDNode *ProfileData, + SmallVectorImpl &Weights) { + extractFromBranchWeightMD(ProfileData, Weights); +} - for (unsigned Idx = WeightsIdx, E = NOps; Idx != E; ++Idx) { - ConstantInt *Weight = - mdconst::dyn_extract(ProfileData->getOperand(Idx)); - assert(Weight && "Malformed branch_weight in MD_prof node"); - assert(Weight->getValue().getActiveBits() <= 32 && - "Too many bits for uint32_t"); - Weights[Idx - WeightsIdx] = Weight->getZExtValue(); - } +void extractFromBranchWeightMD64(const MDNode *ProfileData, + SmallVectorImpl &Weights) { + extractFromBranchWeightMD(ProfileData, Weights); } bool extractBranchWeights(const MDNode *ProfileData, @@ -162,7 +193,8 @@ bool extractProfTotalWeight(const MDNode *ProfileData, uint64_t &TotalVal) { return false; if (ProfDataName->getString().equals("branch_weights")) { - for (unsigned Idx = 1; Idx < ProfileData->getNumOperands(); Idx++) { + unsigned Offset = getBranchWeightOffset(ProfileData); + for (unsigned Idx = Offset; Idx < ProfileData->getNumOperands(); ++Idx) { auto *V = mdconst::dyn_extract(ProfileData->getOperand(Idx)); assert(V && "Malformed branch_weight in MD_prof node"); TotalVal += V->getValue().getZExtValue(); @@ -184,9 +216,10 @@ bool extractProfTotalWeight(const Instruction &I, uint64_t &TotalVal) { return extractProfTotalWeight(I.getMetadata(LLVMContext::MD_prof), TotalVal); } -void setBranchWeights(Instruction &I, ArrayRef Weights) { +void setBranchWeights(Instruction &I, ArrayRef Weights, + bool IsExpected) { MDBuilder MDB(I.getContext()); - MDNode *BranchWeights = MDB.createBranchWeights(Weights); + MDNode *BranchWeights = MDB.createBranchWeights(Weights, IsExpected); I.setMetadata(LLVMContext::MD_prof, BranchWeights); } diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index e42cc7e260ef1..4a142be71eec4 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -104,6 +104,7 @@ #include "llvm/IR/Module.h" #include "llvm/IR/ModuleSlotTracker.h" #include "llvm/IR/PassManager.h" +#include "llvm/IR/ProfDataUtils.h" #include "llvm/IR/Statepoint.h" #include "llvm/IR/Type.h" #include "llvm/IR/Use.h" @@ -4786,8 +4787,10 @@ void Verifier::visitProfMetadata(Instruction &I, MDNode *MD) { // Check consistency of !prof branch_weights metadata. if (ProfName.equals("branch_weights")) { + unsigned int Offset = getBranchWeightOffset(I); if (isa(&I)) { - Check(MD->getNumOperands() == 2 || MD->getNumOperands() == 3, + Check(MD->getNumOperands() == (1 + Offset) || + MD->getNumOperands() == (2 + Offset), "Wrong number of InvokeInst branch_weights operands", MD); } else { unsigned ExpectedNumOperands = 0; @@ -4807,10 +4810,10 @@ void Verifier::visitProfMetadata(Instruction &I, MDNode *MD) { CheckFailed("!prof branch_weights are not allowed for this instruction", MD); - Check(MD->getNumOperands() == 1 + ExpectedNumOperands, + Check(MD->getNumOperands() == Offset + ExpectedNumOperands, "Wrong number of operands", MD); } - for (unsigned i = 1; i < MD->getNumOperands(); ++i) { + for (unsigned i = Offset; i < MD->getNumOperands(); ++i) { auto &MDO = MD->getOperand(i); Check(MDO, "second operand should not be null", MD); Check(mdconst::dyn_extract(MDO), diff --git a/llvm/lib/Transforms/IPO/SampleProfile.cpp b/llvm/lib/Transforms/IPO/SampleProfile.cpp index 0b3a6931e779b..a627e11930785 100644 --- a/llvm/lib/Transforms/IPO/SampleProfile.cpp +++ b/llvm/lib/Transforms/IPO/SampleProfile.cpp @@ -1659,7 +1659,8 @@ void SampleProfileLoader::generateMDProfMetadata(Function &F) { else if (OverwriteExistingWeights) I.setMetadata(LLVMContext::MD_prof, nullptr); } else if (!isa(&I)) { - setBranchWeights(I, {static_cast(BlockWeights[BB])}); + setBranchWeights(I, {static_cast(BlockWeights[BB])}, + /*IsExpected=*/false); } } } else if (OverwriteExistingWeights || ProfileSampleBlockAccurate) { @@ -1670,7 +1671,7 @@ void SampleProfileLoader::generateMDProfMetadata(Function &F) { if (cast(I).isIndirectCall()) { I.setMetadata(LLVMContext::MD_prof, nullptr); } else { - setBranchWeights(I, {uint32_t(0)}); + setBranchWeights(I, {uint32_t(0)}, /*IsExpected=*/false); } } } @@ -1751,7 +1752,7 @@ void SampleProfileLoader::generateMDProfMetadata(Function &F) { if (MaxWeight > 0 && (!TI->extractProfTotalWeight(TempWeight) || OverwriteExistingWeights)) { LLVM_DEBUG(dbgs() << "SUCCESS. Found non-zero weights.\n"); - setBranchWeights(*TI, Weights); + setBranchWeights(*TI, Weights, /*IsExpected=*/false); ORE->emit([&]() { return OptimizationRemark(DEBUG_TYPE, "PopularDest", MaxDestInst) << "most popular destination for conditional branches at " diff --git a/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp b/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp index 0a3d8d6000cf4..731104d4fcef0 100644 --- a/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp +++ b/llvm/lib/Transforms/Instrumentation/ControlHeightReduction.cpp @@ -1878,7 +1878,7 @@ void CHR::fixupBranchesAndSelects(CHRScope *Scope, static_cast(CHRBranchBias.scale(1000)), static_cast(CHRBranchBias.getCompl().scale(1000)), }; - setBranchWeights(*MergedBR, Weights); + setBranchWeights(*MergedBR, Weights, /*IsExpected=*/false); CHR_DEBUG(dbgs() << "CHR branch bias " << Weights[0] << ":" << Weights[1] << "\n"); } diff --git a/llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp b/llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp index 23a7c6a20aecb..6db76ca78b218 100644 --- a/llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp +++ b/llvm/lib/Transforms/Instrumentation/IndirectCallPromotion.cpp @@ -259,7 +259,8 @@ CallBase &llvm::pgo::promoteIndirectCall(CallBase &CB, Function *DirectCallee, promoteCallWithIfThenElse(CB, DirectCallee, BranchWeights); if (AttachProfToDirectCall) { - setBranchWeights(NewInst, {static_cast(Count)}); + setBranchWeights(NewInst, {static_cast(Count)}, + /*IsExpected=*/false); } using namespace ore; diff --git a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp index a7b7556685e44..88808e9862c87 100644 --- a/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp +++ b/llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp @@ -1422,7 +1422,8 @@ void PGOUseFunc::populateCoverage(IndexedInstrProfReader *PGOReader) { for (auto *Succ : successors(&BB)) Weights.push_back((Coverage[Succ] || !Coverage[&BB]) ? 1 : 0); if (Weights.size() >= 2) - llvm::setBranchWeights(*BB.getTerminator(), Weights); + llvm::setBranchWeights(*BB.getTerminator(), Weights, + /*IsExpected=*/false); } unsigned NumCorruptCoverage = 0; @@ -2206,7 +2207,7 @@ void llvm::setProfMetadata(Module *M, Instruction *TI, misexpect::checkExpectAnnotations(*TI, Weights, /*IsFrontend=*/false); - setBranchWeights(*TI, Weights); + setBranchWeights(*TI, Weights, /*IsExpected=*/false); if (EmitBranchProbability) { std::string BrCondStr = getBranchCondString(TI); if (BrCondStr.empty()) diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp index ffcb511e6a831..7bb3a9219b94d 100644 --- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp +++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp @@ -231,7 +231,7 @@ static void updatePredecessorProfileMetadata(PHINode *PN, BasicBlock *BB) { Weights[0] = BP.getCompl().getNumerator(); Weights[1] = BP.getNumerator(); } - setBranchWeights(*PredBr, Weights); + setBranchWeights(*PredBr, Weights, hasBranchWeightProvenance(*PredBr)); } } @@ -2610,7 +2610,7 @@ void JumpThreadingPass::updateBlockFreqAndEdgeWeight(BasicBlock *PredBB, Weights.push_back(Prob.getNumerator()); auto TI = BB->getTerminator(); - setBranchWeights(*TI, Weights); + setBranchWeights(*TI, Weights, hasBranchWeightProvenance(*TI)); } } diff --git a/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp b/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp index 6f87e4d91d2c7..17c5a4ee1fd0b 100644 --- a/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp +++ b/llvm/lib/Transforms/Scalar/LowerExpectIntrinsic.cpp @@ -102,7 +102,7 @@ static bool handleSwitchExpect(SwitchInst &SI) { misexpect::checkExpectAnnotations(SI, Weights, /*IsFrontend=*/true); SI.setCondition(ArgValue); - setBranchWeights(SI, Weights); + setBranchWeights(SI, Weights, /*IsExpected=*/true); return true; } @@ -262,11 +262,13 @@ static void handlePhiDef(CallInst *Expect) { if (IsOpndComingFromSuccessor(BI->getSuccessor(1))) BI->setMetadata(LLVMContext::MD_prof, MDB.createBranchWeights(LikelyBranchWeightVal, - UnlikelyBranchWeightVal)); + UnlikelyBranchWeightVal, + /*IsExpected=*/true)); else if (IsOpndComingFromSuccessor(BI->getSuccessor(0))) BI->setMetadata(LLVMContext::MD_prof, MDB.createBranchWeights(UnlikelyBranchWeightVal, - LikelyBranchWeightVal)); + LikelyBranchWeightVal, + /*IsExpected=*/true)); } } @@ -331,12 +333,12 @@ template static bool handleBrSelExpect(BrSelInst &BSI) { SmallVector ExpectedWeights; if ((ExpectedValue->getZExtValue() == ValueComparedTo) == (Predicate == CmpInst::ICMP_EQ)) { - Node = - MDB.createBranchWeights(LikelyBranchWeightVal, UnlikelyBranchWeightVal); + Node = MDB.createBranchWeights( + LikelyBranchWeightVal, UnlikelyBranchWeightVal, /*IsExpected=*/true); ExpectedWeights = {LikelyBranchWeightVal, UnlikelyBranchWeightVal}; } else { - Node = - MDB.createBranchWeights(UnlikelyBranchWeightVal, LikelyBranchWeightVal); + Node = MDB.createBranchWeights(UnlikelyBranchWeightVal, + LikelyBranchWeightVal, /*IsExpected=*/true); ExpectedWeights = {UnlikelyBranchWeightVal, LikelyBranchWeightVal}; } diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp index 5f456092bf4e9..c9b3342225fe9 100644 --- a/llvm/lib/Transforms/Utils/Local.cpp +++ b/llvm/lib/Transforms/Utils/Local.cpp @@ -231,7 +231,7 @@ bool llvm::ConstantFoldTerminator(BasicBlock *BB, bool DeleteDeadConditions, // Remove weight for this case. std::swap(Weights[Idx + 1], Weights.back()); Weights.pop_back(); - setBranchWeights(*SI, Weights); + setBranchWeights(*SI, Weights, hasBranchWeightProvenance(MD)); } // Remove this entry. BasicBlock *ParentBB = SI->getParent(); diff --git a/llvm/lib/Transforms/Utils/LoopPeel.cpp b/llvm/lib/Transforms/Utils/LoopPeel.cpp index f76fa3bb6c611..c0f7dff5fb10a 100644 --- a/llvm/lib/Transforms/Utils/LoopPeel.cpp +++ b/llvm/lib/Transforms/Utils/LoopPeel.cpp @@ -640,7 +640,7 @@ struct WeightInfo { /// To avoid dealing with division rounding we can just multiple both part /// of weights to E and use weight as (F - I * E, E). static void updateBranchWeights(Instruction *Term, WeightInfo &Info) { - setBranchWeights(*Term, Info.Weights); + setBranchWeights(*Term, Info.Weights, /*IsExpected=*/false); for (auto [Idx, SubWeight] : enumerate(Info.SubWeights)) if (SubWeight != 0) // Don't set the probability of taking the edge from latch to loop header @@ -1033,7 +1033,7 @@ bool llvm::peelLoop(Loop *L, unsigned PeelCount, LoopInfo *LI, } for (const auto &[Term, Info] : Weights) { - setBranchWeights(*Term, Info.Weights); + setBranchWeights(*Term, Info.Weights, /*IsExpected=*/false); } // Update Metadata for count of peeled off iterations. diff --git a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp index 0f55af3b6eddf..143677f1d14b0 100644 --- a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp +++ b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp @@ -287,7 +287,7 @@ static void updateBranchWeights(BranchInst &PreHeaderBI, BranchInst &LoopBI, return; SmallVector Weights; - extractFromBranchWeightMD(WeightMD, Weights); + extractFromBranchWeightMD32(WeightMD, Weights); if (Weights.size() != 2) return; uint32_t OrigLoopExitWeight = Weights[0]; @@ -390,13 +390,13 @@ static void updateBranchWeights(BranchInst &PreHeaderBI, BranchInst &LoopBI, SuccsSwapped ? LoopBackWeight : ExitWeight1, SuccsSwapped ? ExitWeight1 : LoopBackWeight, }; - setBranchWeights(LoopBI, LoopBIWeights); + setBranchWeights(LoopBI, LoopBIWeights, /*IsExpected=*/false); if (HasConditionalPreHeader) { const uint32_t PreHeaderBIWeights[] = { SuccsSwapped ? EnterWeight : ExitWeight0, SuccsSwapped ? ExitWeight0 : EnterWeight, }; - setBranchWeights(PreHeaderBI, PreHeaderBIWeights); + setBranchWeights(PreHeaderBI, PreHeaderBIWeights, /*IsExpected=*/false); } } diff --git a/llvm/lib/Transforms/Utils/MisExpect.cpp b/llvm/lib/Transforms/Utils/MisExpect.cpp index 6f5a25a26821b..59e13795f0f24 100644 --- a/llvm/lib/Transforms/Utils/MisExpect.cpp +++ b/llvm/lib/Transforms/Utils/MisExpect.cpp @@ -59,9 +59,10 @@ static cl::opt PGOWarnMisExpect( cl::desc("Use this option to turn on/off " "warnings about incorrect usage of llvm.expect intrinsics.")); +// Command line option for setting the diagnostic tolerance threshold static cl::opt MisExpectTolerance( "misexpect-tolerance", cl::init(0), - cl::desc("Prevents emiting diagnostics when profile counts are " + cl::desc("Prevents emitting diagnostics when profile counts are " "within N% of the threshold..")); } // namespace llvm @@ -150,15 +151,9 @@ void verifyMisExpect(Instruction &I, ArrayRef RealWeights, uint64_t TotalBranchWeight = LikelyBranchWeight + (UnlikelyBranchWeight * NumUnlikelyTargets); - // FIXME: When we've addressed sample profiling, restore the assertion - // - // We cannot calculate branch probability if either of these invariants aren't - // met. However, MisExpect diagnostics should not prevent code from compiling, - // so we simply forgo emitting diagnostics here, and return early. - // assert((TotalBranchWeight >= LikelyBranchWeight) && (TotalBranchWeight > 0) - // && "TotalBranchWeight is less than the Likely branch weight"); - if ((TotalBranchWeight == 0) || (TotalBranchWeight <= LikelyBranchWeight)) - return; + // Failing this assert means that we have corrupted metadata. + assert((TotalBranchWeight >= LikelyBranchWeight) && (TotalBranchWeight > 0) && + "TotalBranchWeight is less than the Likely branch weight"); // To determine our threshold value we need to obtain the branch probability // for the weights added by llvm.expect and use that proportion to calculate @@ -185,6 +180,13 @@ void verifyMisExpect(Instruction &I, ArrayRef RealWeights, void checkBackendInstrumentation(Instruction &I, const ArrayRef RealWeights) { + // Backend checking assumes any existing weight comes from an `llvm.expect` + // intrinsic. However, SampleProfiling + ThinLTO add branch weights multiple + // times, leading to an invalid assumption in our checking. Backend checks + // should only operate on branch weights that carry the "!expected" field, + // since they are guaranteed to be added by the LowerExpectIntrinsic pass. + if (!hasBranchWeightProvenance(I)) + return; SmallVector ExpectedWeights; if (!extractBranchWeights(I, ExpectedWeights)) return; diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp index 4db72461c95e4..b891eea0f3413 100644 --- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp +++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp @@ -861,26 +861,28 @@ static bool ValuesOverlap(std::vector &C1, // Set branch weights on SwitchInst. This sets the metadata if there is at // least one non-zero weight. -static void setBranchWeights(SwitchInst *SI, ArrayRef Weights) { +static void setBranchWeights(SwitchInst *SI, ArrayRef Weights, + bool IsExpected) { // Check that there is at least one non-zero weight. Otherwise, pass // nullptr to setMetadata which will erase the existing metadata. MDNode *N = nullptr; if (llvm::any_of(Weights, [](uint32_t W) { return W != 0; })) - N = MDBuilder(SI->getParent()->getContext()).createBranchWeights(Weights); + N = MDBuilder(SI->getParent()->getContext()) + .createBranchWeights(Weights, IsExpected); SI->setMetadata(LLVMContext::MD_prof, N); } // Similar to the above, but for branch and select instructions that take // exactly 2 weights. static void setBranchWeights(Instruction *I, uint32_t TrueWeight, - uint32_t FalseWeight) { + uint32_t FalseWeight, bool IsExpected) { assert(isa(I) || isa(I)); // Check that there is at least one non-zero weight. Otherwise, pass // nullptr to setMetadata which will erase the existing metadata. MDNode *N = nullptr; if (TrueWeight || FalseWeight) N = MDBuilder(I->getParent()->getContext()) - .createBranchWeights(TrueWeight, FalseWeight); + .createBranchWeights(TrueWeight, FalseWeight, IsExpected); I->setMetadata(LLVMContext::MD_prof, N); } @@ -1066,11 +1068,8 @@ static int ConstantIntSortPredicate(ConstantInt *const *P1, static void GetBranchWeights(Instruction *TI, SmallVectorImpl &Weights) { MDNode *MD = TI->getMetadata(LLVMContext::MD_prof); - assert(MD); - for (unsigned i = 1, e = MD->getNumOperands(); i < e; ++i) { - ConstantInt *CI = mdconst::extract(MD->getOperand(i)); - Weights.push_back(CI->getValue().getZExtValue()); - } + assert(MD && "Invalid branch-weight metadata"); + extractFromBranchWeightMD64(MD, Weights); // If TI is a conditional eq, the default case is the false case, // and the corresponding branch-weight data is at index 2. We swap the @@ -1342,7 +1341,7 @@ bool SimplifyCFGOpt::PerformValueComparisonIntoPredecessorFolding( SmallVector MDWeights(Weights.begin(), Weights.end()); - setBranchWeights(NewSI, MDWeights); + setBranchWeights(NewSI, MDWeights, /*IsExpected=*/false); } EraseTerminatorAndDCECond(PTI); @@ -3835,7 +3834,7 @@ static bool performBranchToCommonDestFolding(BranchInst *BI, BranchInst *PBI, FitWeights(NewWeights); SmallVector MDWeights(NewWeights.begin(), NewWeights.end()); - setBranchWeights(PBI, MDWeights[0], MDWeights[1]); + setBranchWeights(PBI, MDWeights[0], MDWeights[1], /*IsExpected=*/false); // TODO: If BB is reachable from all paths through PredBlock, then we // could replace PBI's branch probabilities with BI's. @@ -4572,7 +4571,7 @@ static bool SimplifyCondBranchToCondBranch(BranchInst *PBI, BranchInst *BI, // Halve the weights if any of them cannot fit in an uint32_t FitWeights(NewWeights); - setBranchWeights(PBI, NewWeights[0], NewWeights[1]); + setBranchWeights(PBI, NewWeights[0], NewWeights[1], /*IsExpected=*/false); } // OtherDest may have phi nodes. If so, add an entry from PBI's @@ -4608,7 +4607,8 @@ static bool SimplifyCondBranchToCondBranch(BranchInst *PBI, BranchInst *BI, FitWeights(NewWeights); - setBranchWeights(NV, NewWeights[0], NewWeights[1]); + setBranchWeights(NV, NewWeights[0], NewWeights[1], + /*IsExpected=*/false); } } } @@ -4671,7 +4671,7 @@ bool SimplifyCFGOpt::SimplifyTerminatorOnSelect(Instruction *OldTerm, // Create a conditional branch sharing the condition of the select. BranchInst *NewBI = Builder.CreateCondBr(Cond, TrueBB, FalseBB); if (TrueWeight != FalseWeight) - setBranchWeights(NewBI, TrueWeight, FalseWeight); + setBranchWeights(NewBI, TrueWeight, FalseWeight, /*IsExpected=*/false); } } else if (KeepEdge1 && (KeepEdge2 || TrueBB == FalseBB)) { // Neither of the selected blocks were successors, so this @@ -5618,7 +5618,7 @@ bool SimplifyCFGOpt::TurnSwitchRangeIntoICmp(SwitchInst *SI, TrueWeight /= 2; FalseWeight /= 2; } - setBranchWeights(NewBI, TrueWeight, FalseWeight); + setBranchWeights(NewBI, TrueWeight, FalseWeight, /*IsExpected=*/false); } } diff --git a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp index 33c4decd58a6c..d26ba1eebac54 100644 --- a/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp +++ b/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp @@ -2151,7 +2151,7 @@ class GeneratedRTChecks { BranchInst &BI = *BranchInst::Create(Bypass, LoopVectorPreHeader, Cond); if (AddBranchWeights) - setBranchWeights(BI, SCEVCheckBypassWeights); + setBranchWeights(BI, SCEVCheckBypassWeights, /*IsExpected=*/false); ReplaceInstWithInst(SCEVCheckBlock->getTerminator(), &BI); return SCEVCheckBlock; } @@ -2179,7 +2179,7 @@ class GeneratedRTChecks { BranchInst &BI = *BranchInst::Create(Bypass, LoopVectorPreHeader, MemRuntimeCheckCond); if (AddBranchWeights) { - setBranchWeights(BI, MemCheckBypassWeights); + setBranchWeights(BI, MemCheckBypassWeights, /*IsExpected=*/false); } ReplaceInstWithInst(MemCheckBlock->getTerminator(), &BI); MemCheckBlock->getTerminator()->setDebugLoc( @@ -2896,7 +2896,7 @@ void InnerLoopVectorizer::emitIterationCountCheck(BasicBlock *Bypass) { BranchInst &BI = *BranchInst::Create(Bypass, LoopVectorPreHeader, CheckMinIters); if (hasBranchWeightMD(*OrigLoop->getLoopLatch()->getTerminator())) - setBranchWeights(BI, MinItersBypassWeights); + setBranchWeights(BI, MinItersBypassWeights, /*IsExpected=*/false); ReplaceInstWithInst(TCCheckBlock->getTerminator(), &BI); LoopBypassBlocks.push_back(TCCheckBlock); } @@ -3136,7 +3136,7 @@ BasicBlock *InnerLoopVectorizer::completeLoopSkeleton() { unsigned TripCount = UF * VF.getKnownMinValue(); assert(TripCount > 0 && "trip count should not be zero"); const uint32_t Weights[] = {1, TripCount - 1}; - setBranchWeights(BI, Weights); + setBranchWeights(BI, Weights, /*IsExpected=*/false); } } @@ -7750,7 +7750,7 @@ EpilogueVectorizerMainLoop::emitIterationCountCheck(BasicBlock *Bypass, BranchInst &BI = *BranchInst::Create(Bypass, LoopVectorPreHeader, CheckMinIters); if (hasBranchWeightMD(*OrigLoop->getLoopLatch()->getTerminator())) - setBranchWeights(BI, MinItersBypassWeights); + setBranchWeights(BI, MinItersBypassWeights, /*IsExpected=*/false); ReplaceInstWithInst(TCCheckBlock->getTerminator(), &BI); return TCCheckBlock; @@ -7908,7 +7908,7 @@ EpilogueVectorizerEpilogueLoop::emitMinimumVectorEpilogueIterCountCheck( unsigned EstimatedSkipCount = std::min(MainLoopStep, EpilogueLoopStep); const uint32_t Weights[] = {EstimatedSkipCount, MainLoopStep - EstimatedSkipCount}; - setBranchWeights(BI, Weights); + setBranchWeights(BI, Weights, /*IsExpected=*/false); } ReplaceInstWithInst(Insert->getTerminator(), &BI); diff --git a/llvm/test/Transforms/LowerExpectIntrinsic/basic.ll b/llvm/test/Transforms/LowerExpectIntrinsic/basic.ll index 0abca5b383224..8e06cd57a1001 100644 --- a/llvm/test/Transforms/LowerExpectIntrinsic/basic.ll +++ b/llvm/test/Transforms/LowerExpectIntrinsic/basic.ll @@ -284,7 +284,7 @@ define i32 @test10(i64 %t6) { declare i1 @llvm.expect.i1(i1, i1) nounwind readnone -; CHECK: !0 = !{!"branch_weights", i32 2000, i32 1} -; CHECK: !1 = !{!"branch_weights", i32 1, i32 2000} -; CHECK: !2 = !{!"branch_weights", i32 1, i32 1, i32 2000} -; CHECK: !3 = !{!"branch_weights", i32 2000, i32 1, i32 1} +; CHECK: !0 = !{!"branch_weights", !"expected", i32 2000, i32 1} +; CHECK: !1 = !{!"branch_weights", !"expected", i32 1, i32 2000} +; CHECK: !2 = !{!"branch_weights", !"expected", i32 1, i32 1, i32 2000} +; CHECK: !3 = !{!"branch_weights", !"expected", i32 2000, i32 1, i32 1} diff --git a/llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll b/llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll index 64293557050c1..40571278ca93f 100644 --- a/llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll +++ b/llvm/test/Transforms/LowerExpectIntrinsic/expect-with-probability.ll @@ -284,7 +284,7 @@ define i32 @test10(i64 %t6) { declare i1 @llvm.expect.with.probability.i1(i1, i1, double) nounwind readnone -; CHECK: !0 = !{!"branch_weights", i32 1717986918, i32 429496731} -; CHECK: !1 = !{!"branch_weights", i32 429496731, i32 1717986918} -; CHECK: !2 = !{!"branch_weights", i32 214748366, i32 214748366, i32 1717986918} -; CHECK: !3 = !{!"branch_weights", i32 1717986918, i32 214748366, i32 214748366} +; CHECK: !0 = !{!"branch_weights", !"expected", i32 1717986918, i32 429496731} +; CHECK: !1 = !{!"branch_weights", !"expected", i32 429496731, i32 1717986918} +; CHECK: !2 = !{!"branch_weights", !"expected", i32 214748366, i32 214748366, i32 1717986918} +; CHECK: !3 = !{!"branch_weights", !"expected", i32 1717986918, i32 214748366, i32 214748366} diff --git a/llvm/test/Transforms/LowerExpectIntrinsic/expect_nonboolean.ll b/llvm/test/Transforms/LowerExpectIntrinsic/expect_nonboolean.ll index 2bcfb1e064be9..458a7758fa970 100644 --- a/llvm/test/Transforms/LowerExpectIntrinsic/expect_nonboolean.ll +++ b/llvm/test/Transforms/LowerExpectIntrinsic/expect_nonboolean.ll @@ -99,6 +99,5 @@ attributes #1 = { nounwind readnone } !0 = !{i32 1, !"wchar_size", i32 4} !1 = !{!"clang version 5.0.0 (trunk 304373)"} -; CHECK: [[LIKELY]] = !{!"branch_weights", i32 2000, i32 1} -; CHECK: [[UNLIKELY]] = !{!"branch_weights", i32 1, i32 2000} - +; CHECK: [[LIKELY]] = !{!"branch_weights", !"expected", i32 2000, i32 1} +; CHECK: [[UNLIKELY]] = !{!"branch_weights", !"expected", i32 1, i32 2000} diff --git a/llvm/test/Transforms/LowerExpectIntrinsic/phi_merge.ll b/llvm/test/Transforms/LowerExpectIntrinsic/phi_merge.ll index 32ae9b0b2f15c..9b9d9a746dd32 100644 --- a/llvm/test/Transforms/LowerExpectIntrinsic/phi_merge.ll +++ b/llvm/test/Transforms/LowerExpectIntrinsic/phi_merge.ll @@ -352,5 +352,5 @@ declare i64 @llvm.expect.i64(i64, i64) !llvm.ident = !{!0} !0 = !{!"clang version 5.0.0 (trunk 302965)"} -; CHECK: [[WEIGHT]] = !{!"branch_weights", i32 2000, i32 1} -; CHECK: [[WEIGHT2]] = !{!"branch_weights", i32 1, i32 2000} +; CHECK: [[WEIGHT]] = !{!"branch_weights", !"expected", i32 2000, i32 1} +; CHECK: [[WEIGHT2]] = !{!"branch_weights", !"expected", i32 1, i32 2000} diff --git a/llvm/test/Transforms/LowerExpectIntrinsic/phi_or.ll b/llvm/test/Transforms/LowerExpectIntrinsic/phi_or.ll index 1efa63241c2c0..e9a843225993a 100644 --- a/llvm/test/Transforms/LowerExpectIntrinsic/phi_or.ll +++ b/llvm/test/Transforms/LowerExpectIntrinsic/phi_or.ll @@ -99,5 +99,5 @@ declare i64 @llvm.expect.i64(i64, i64) !0 = !{!"clang version 5.0.0 (trunk 302965)"} -; CHECK: [[WEIGHT]] = !{!"branch_weights", i32 2000, i32 1} -; CHECK: [[WEIGHT2]] = !{!"branch_weights", i32 1, i32 2000} +; CHECK: [[WEIGHT]] = !{!"branch_weights", !"expected", i32 2000, i32 1} +; CHECK: [[WEIGHT2]] = !{!"branch_weights", !"expected", i32 1, i32 2000} diff --git a/llvm/test/Transforms/LowerExpectIntrinsic/phi_tern.ll b/llvm/test/Transforms/LowerExpectIntrinsic/phi_tern.ll index 9cbaca8d13dc0..13db2c394bab2 100644 --- a/llvm/test/Transforms/LowerExpectIntrinsic/phi_tern.ll +++ b/llvm/test/Transforms/LowerExpectIntrinsic/phi_tern.ll @@ -53,4 +53,4 @@ declare i64 @llvm.expect.i64(i64, i64) !0 = !{!"clang version 5.0.0 (trunk 302965)"} -; CHECK: [[WEIGHT]] = !{!"branch_weights", i32 1, i32 2000} +; CHECK: [[WEIGHT]] = !{!"branch_weights", !"expected", i32 1, i32 2000} diff --git a/llvm/test/Transforms/LowerExpectIntrinsic/phi_unexpect.ll b/llvm/test/Transforms/LowerExpectIntrinsic/phi_unexpect.ll index 2bad66343b761..275731d618895 100644 --- a/llvm/test/Transforms/LowerExpectIntrinsic/phi_unexpect.ll +++ b/llvm/test/Transforms/LowerExpectIntrinsic/phi_unexpect.ll @@ -235,5 +235,5 @@ block5: ret void } -; CHECK: !0 = !{!"branch_weights", i32 2147483647, i32 1} -; CHECK: !1 = !{!"branch_weights", i32 1, i32 2147483647} +; CHECK: !0 = !{!"branch_weights", !"expected", i32 2147483647, i32 1} +; CHECK: !1 = !{!"branch_weights", !"expected", i32 1, i32 2147483647} From 76928786a9fa72e73edc4fdc8845ce0b9c406584 Mon Sep 17 00:00:00 2001 From: Paul Kirth Date: Thu, 6 Jun 2024 19:04:36 +0000 Subject: [PATCH 2/4] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20ch?= =?UTF-8?q?anges=20introduced=20through=20rebase?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Created using spr 1.3.4 [skip ci] --- llvm/lib/IR/ProfDataUtils.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/llvm/lib/IR/ProfDataUtils.cpp b/llvm/lib/IR/ProfDataUtils.cpp index f738d76937c24..af536d2110eac 100644 --- a/llvm/lib/IR/ProfDataUtils.cpp +++ b/llvm/lib/IR/ProfDataUtils.cpp @@ -133,6 +133,7 @@ bool hasBranchWeightProvenance(const MDNode *ProfileData) { // NOTE: if we ever have more types of branch weight provenance, // we need to check the string value is "expected". For now, we // supply a more generic API, and avoid the spurious comparisons. + assert(ProfDataName->getString() == "expected"); return ProfDataName; } From ee0267bc397227423f3c153fd41cf314c9a99675 Mon Sep 17 00:00:00 2001 From: Paul Kirth Date: Thu, 6 Jun 2024 21:13:53 +0000 Subject: [PATCH 3/4] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20ch?= =?UTF-8?q?anges=20introduced=20through=20rebase?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Created using spr 1.3.4 [skip ci] --- llvm/include/llvm/IR/ProfDataUtils.h | 4 ++-- llvm/lib/CodeGen/CodeGenPrepare.cpp | 9 ++++----- llvm/lib/IR/Metadata.cpp | 8 ++++---- llvm/lib/IR/ProfDataUtils.cpp | 14 +++++++------- llvm/lib/IR/Verifier.cpp | 2 +- llvm/lib/Transforms/Scalar/JumpThreading.cpp | 4 ++-- llvm/lib/Transforms/Utils/Local.cpp | 2 +- llvm/lib/Transforms/Utils/MisExpect.cpp | 2 +- 8 files changed, 22 insertions(+), 23 deletions(-) diff --git a/llvm/include/llvm/IR/ProfDataUtils.h b/llvm/include/llvm/IR/ProfDataUtils.h index 3c761bdc1bf3e..1d7c97d9be953 100644 --- a/llvm/include/llvm/IR/ProfDataUtils.h +++ b/llvm/include/llvm/IR/ProfDataUtils.h @@ -57,11 +57,11 @@ MDNode *getValidBranchWeightMDNode(const Instruction &I); /// Check if Branch Weight Metadata has an "expected" field from an llvm.expect* /// intrinsic -bool hasBranchWeightProvenance(const Instruction &I); +bool hasBranchWeightOrigin(const Instruction &I); /// Check if Branch Weight Metadata has an "expected" field from an llvm.expect* /// intrinsic -bool hasBranchWeightProvenance(const MDNode *ProfileData); +bool hasBranchWeightOrigin(const MDNode *ProfileData); /// Return the offset to the first branch weight data unsigned getBranchWeightOffset(const MDNode *ProfileData); diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp index ce11caca38988..0e01080bd75cc 100644 --- a/llvm/lib/CodeGen/CodeGenPrepare.cpp +++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp @@ -8864,11 +8864,10 @@ bool CodeGenPrepare::splitBranchCondition(Function &F, ModifyDT &ModifiedDT) { uint64_t NewTrueWeight = TrueWeight; uint64_t NewFalseWeight = TrueWeight + 2 * FalseWeight; scaleWeights(NewTrueWeight, NewFalseWeight); - Br1->setMetadata( - LLVMContext::MD_prof, - MDBuilder(Br1->getContext()) - .createBranchWeights(TrueWeight, FalseWeight, - hasBranchWeightProvenance(*Br1))); + Br1->setMetadata(LLVMContext::MD_prof, + MDBuilder(Br1->getContext()) + .createBranchWeights(TrueWeight, FalseWeight, + hasBranchWeightOrigin(*Br1))); NewTrueWeight = TrueWeight; NewFalseWeight = 2 * FalseWeight; diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp index b6c932495a145..5f42ce22f72fe 100644 --- a/llvm/lib/IR/Metadata.cpp +++ b/llvm/lib/IR/Metadata.cpp @@ -1196,10 +1196,10 @@ MDNode *MDNode::mergeDirectCallProfMetadata(MDNode *A, MDNode *B, StringRef AProfName = AMDS->getString(); StringRef BProfName = BMDS->getString(); if (AProfName == "branch_weights" && BProfName == "branch_weights") { - ConstantInt *AInstrWeight = - mdconst::dyn_extract(A->getOperand(1)); - ConstantInt *BInstrWeight = - mdconst::dyn_extract(B->getOperand(1)); + ConstantInt *AInstrWeight = mdconst::dyn_extract( + A->getOperand(getBranchWeightOffset(A))); + ConstantInt *BInstrWeight = mdconst::dyn_extract( + B->getOperand(getBranchWeightOffset(B))); assert(AInstrWeight && BInstrWeight && "verified by LLVM verifier"); return MDNode::get(Ctx, {MDHelper.createString("branch_weights"), diff --git a/llvm/lib/IR/ProfDataUtils.cpp b/llvm/lib/IR/ProfDataUtils.cpp index af536d2110eac..c4b1ed55de8a2 100644 --- a/llvm/lib/IR/ProfDataUtils.cpp +++ b/llvm/lib/IR/ProfDataUtils.cpp @@ -121,24 +121,24 @@ bool hasValidBranchWeightMD(const Instruction &I) { return getValidBranchWeightMDNode(I); } -bool hasBranchWeightProvenance(const Instruction &I) { +bool hasBranchWeightOrigin(const Instruction &I) { auto *ProfileData = I.getMetadata(LLVMContext::MD_prof); - return hasBranchWeightProvenance(ProfileData); + return hasBranchWeightOrigin(ProfileData); } -bool hasBranchWeightProvenance(const MDNode *ProfileData) { +bool hasBranchWeightOrigin(const MDNode *ProfileData) { if (!isBranchWeightMD(ProfileData)) return false; auto *ProfDataName = dyn_cast(ProfileData->getOperand(1)); // NOTE: if we ever have more types of branch weight provenance, // we need to check the string value is "expected". For now, we // supply a more generic API, and avoid the spurious comparisons. - assert(ProfDataName->getString() == "expected"); - return ProfDataName; + assert(ProfDataName == nullptr || ProfDataName->getString() == "expected"); + return ProfDataName != nullptr; } unsigned getBranchWeightOffset(const MDNode *ProfileData) { - return hasBranchWeightProvenance(ProfileData) ? 2 : 1; + return hasBranchWeightOrigin(ProfileData) ? 2 : 1; } MDNode *getBranchWeightMDNode(const Instruction &I) { @@ -210,7 +210,7 @@ bool extractProfTotalWeight(const MDNode *ProfileData, uint64_t &TotalVal) { if (!ProfDataName) return false; - if (ProfDataName->getString().equals("branch_weights")) { + if (ProfDataName->getString() == "branch_weights") { unsigned Offset = getBranchWeightOffset(ProfileData); for (unsigned Idx = Offset; Idx < ProfileData->getNumOperands(); ++Idx) { auto *V = mdconst::dyn_extract(ProfileData->getOperand(Idx)); diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp index 39185905a1516..e0fde2b7d90dc 100644 --- a/llvm/lib/IR/Verifier.cpp +++ b/llvm/lib/IR/Verifier.cpp @@ -4808,7 +4808,7 @@ void Verifier::visitProfMetadata(Instruction &I, MDNode *MD) { StringRef ProfName = MDS->getString(); // Check consistency of !prof branch_weights metadata. - if (ProfName.equals("branch_weights")) { + if (ProfName == "branch_weights") { unsigned int Offset = getBranchWeightOffset(MD); if (isa(&I)) { Check(MD->getNumOperands() == (1 + Offset) || diff --git a/llvm/lib/Transforms/Scalar/JumpThreading.cpp b/llvm/lib/Transforms/Scalar/JumpThreading.cpp index 88307b8b074ed..b9583836aea06 100644 --- a/llvm/lib/Transforms/Scalar/JumpThreading.cpp +++ b/llvm/lib/Transforms/Scalar/JumpThreading.cpp @@ -231,7 +231,7 @@ static void updatePredecessorProfileMetadata(PHINode *PN, BasicBlock *BB) { Weights[0] = BP.getCompl().getNumerator(); Weights[1] = BP.getNumerator(); } - setBranchWeights(*PredBr, Weights, hasBranchWeightProvenance(*PredBr)); + setBranchWeights(*PredBr, Weights, hasBranchWeightOrigin(*PredBr)); } } @@ -2618,7 +2618,7 @@ void JumpThreadingPass::updateBlockFreqAndEdgeWeight(BasicBlock *PredBB, Weights.push_back(Prob.getNumerator()); auto TI = BB->getTerminator(); - setBranchWeights(*TI, Weights, hasBranchWeightProvenance(*TI)); + setBranchWeights(*TI, Weights, hasBranchWeightOrigin(*TI)); } } diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp index 8f116c42d3d78..12229123675e7 100644 --- a/llvm/lib/Transforms/Utils/Local.cpp +++ b/llvm/lib/Transforms/Utils/Local.cpp @@ -231,7 +231,7 @@ bool llvm::ConstantFoldTerminator(BasicBlock *BB, bool DeleteDeadConditions, // Remove weight for this case. std::swap(Weights[Idx + 1], Weights.back()); Weights.pop_back(); - setBranchWeights(*SI, Weights, hasBranchWeightProvenance(MD)); + setBranchWeights(*SI, Weights, hasBranchWeightOrigin(MD)); } // Remove this entry. BasicBlock *ParentBB = SI->getParent(); diff --git a/llvm/lib/Transforms/Utils/MisExpect.cpp b/llvm/lib/Transforms/Utils/MisExpect.cpp index 59e13795f0f24..aef9d82db0424 100644 --- a/llvm/lib/Transforms/Utils/MisExpect.cpp +++ b/llvm/lib/Transforms/Utils/MisExpect.cpp @@ -185,7 +185,7 @@ void checkBackendInstrumentation(Instruction &I, // times, leading to an invalid assumption in our checking. Backend checks // should only operate on branch weights that carry the "!expected" field, // since they are guaranteed to be added by the LowerExpectIntrinsic pass. - if (!hasBranchWeightProvenance(I)) + if (!hasBranchWeightOrigin(I)) return; SmallVector ExpectedWeights; if (!extractBranchWeights(I, ExpectedWeights)) From b1f98ba553239ca8ec8026d2f98449df63dd132b Mon Sep 17 00:00:00 2001 From: Philip Reames Date: Mon, 24 Jun 2024 16:03:52 +0000 Subject: [PATCH 4/4] =?UTF-8?q?[=F0=9D=98=80=F0=9D=97=BD=F0=9D=97=BF]=20ch?= =?UTF-8?q?anges=20introduced=20through=20rebase?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Created using spr 1.3.4 [skip ci] --- llvm/lib/Transforms/Utils/MisExpect.cpp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/llvm/lib/Transforms/Utils/MisExpect.cpp b/llvm/lib/Transforms/Utils/MisExpect.cpp index aef9d82db0424..759289384ee06 100644 --- a/llvm/lib/Transforms/Utils/MisExpect.cpp +++ b/llvm/lib/Transforms/Utils/MisExpect.cpp @@ -151,9 +151,15 @@ void verifyMisExpect(Instruction &I, ArrayRef RealWeights, uint64_t TotalBranchWeight = LikelyBranchWeight + (UnlikelyBranchWeight * NumUnlikelyTargets); - // Failing this assert means that we have corrupted metadata. - assert((TotalBranchWeight >= LikelyBranchWeight) && (TotalBranchWeight > 0) && - "TotalBranchWeight is less than the Likely branch weight"); + // FIXME: When we've addressed sample profiling, restore the assertion + // + // We cannot calculate branch probability if either of these invariants aren't + // met. However, MisExpect diagnostics should not prevent code from compiling, + // so we simply forgo emitting diagnostics here, and return early. + // assert((TotalBranchWeight >= LikelyBranchWeight) && (TotalBranchWeight > 0) + // && "TotalBranchWeight is less than the Likely branch weight"); + if ((TotalBranchWeight == 0) || (TotalBranchWeight <= LikelyBranchWeight)) + return; // To determine our threshold value we need to obtain the branch probability // for the weights added by llvm.expect and use that proportion to calculate @@ -180,13 +186,6 @@ void verifyMisExpect(Instruction &I, ArrayRef RealWeights, void checkBackendInstrumentation(Instruction &I, const ArrayRef RealWeights) { - // Backend checking assumes any existing weight comes from an `llvm.expect` - // intrinsic. However, SampleProfiling + ThinLTO add branch weights multiple - // times, leading to an invalid assumption in our checking. Backend checks - // should only operate on branch weights that carry the "!expected" field, - // since they are guaranteed to be added by the LowerExpectIntrinsic pass. - if (!hasBranchWeightOrigin(I)) - return; SmallVector ExpectedWeights; if (!extractBranchWeights(I, ExpectedWeights)) return;