llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-bolt Author: Amir Ayupov (aaupov) <details> <summary>Changes</summary> 9d0754ada5dbbc0c009bcc2f7824488419cc5530 dropped MC support required for macro-fusion alignment in BOLT. Remove the support in BOLT. Test Plan: macro-fusion alignment was never upstreamed, so no upstream tests are affected. --- Full diff: https://github.com/llvm/llvm-project/pull/97358.diff 11 Files Affected: - (modified) bolt/include/bolt/Core/BinaryBasicBlock.h (-9) - (modified) bolt/include/bolt/Core/BinaryContext.h (-4) - (modified) bolt/include/bolt/Core/BinaryFunction.h (-4) - (modified) bolt/include/bolt/Core/MCPlusBuilder.h (-7) - (modified) bolt/lib/Core/BinaryBasicBlock.cpp (-39) - (modified) bolt/lib/Core/BinaryEmitter.cpp (-37) - (modified) bolt/lib/Core/BinaryFunction.cpp (-25) - (modified) bolt/lib/Passes/BinaryPasses.cpp (-20) - (modified) bolt/lib/Rewrite/RewriteInstance.cpp (-22) - (modified) bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp (-4) - (modified) bolt/lib/Target/X86/X86MCPlusBuilder.cpp (-34) ``````````diff diff --git a/bolt/include/bolt/Core/BinaryBasicBlock.h b/bolt/include/bolt/Core/BinaryBasicBlock.h index a57b70714fe38..9a9d7b8735d71 100644 --- a/bolt/include/bolt/Core/BinaryBasicBlock.h +++ b/bolt/include/bolt/Core/BinaryBasicBlock.h @@ -842,15 +842,6 @@ class BinaryBasicBlock { bool analyzeBranch(const MCSymbol *&TBB, const MCSymbol *&FBB, MCInst *&CondBranch, MCInst *&UncondBranch); - /// Return true if iterator \p I is pointing to the first instruction in - /// a pair that could be macro-fused. - bool isMacroOpFusionPair(const_iterator I) const; - - /// If the basic block has a pair of instructions suitable for macro-fusion, - /// return iterator to the first instruction of the pair. - /// Otherwise return end(). - const_iterator getMacroOpFusionPair() const; - /// Printer required for printing dominator trees. void printAsOperand(raw_ostream &OS, bool PrintType = true) { if (PrintType) diff --git a/bolt/include/bolt/Core/BinaryContext.h b/bolt/include/bolt/Core/BinaryContext.h index 4ec3de3da1bf8..73932c4ca2fb3 100644 --- a/bolt/include/bolt/Core/BinaryContext.h +++ b/bolt/include/bolt/Core/BinaryContext.h @@ -698,10 +698,6 @@ class BinaryContext { /// Binary-wide aggregated stats. struct BinaryStats { - /// Stats for macro-fusion. - uint64_t MissedMacroFusionPairs{0}; - uint64_t MissedMacroFusionExecCount{0}; - /// Stats for stale profile matching: /// the total number of basic blocks in the profile uint32_t NumStaleBlocks{0}; diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h index 3c641581e247a..d318dfbcbabe7 100644 --- a/bolt/include/bolt/Core/BinaryFunction.h +++ b/bolt/include/bolt/Core/BinaryFunction.h @@ -835,10 +835,6 @@ class BinaryFunction { /// them. void calculateLoopInfo(); - /// Calculate missed macro-fusion opportunities and update BinaryContext - /// stats. - void calculateMacroOpFusionStats(); - /// Returns if BinaryDominatorTree has been constructed for this function. bool hasDomTree() const { return BDT != nullptr; } diff --git a/bolt/include/bolt/Core/MCPlusBuilder.h b/bolt/include/bolt/Core/MCPlusBuilder.h index a5fb3901428d9..584848a8601fd 100644 --- a/bolt/include/bolt/Core/MCPlusBuilder.h +++ b/bolt/include/bolt/Core/MCPlusBuilder.h @@ -917,13 +917,6 @@ class MCPlusBuilder { /// Return true if the instruction is encoded using EVEX (AVX-512). virtual bool hasEVEXEncoding(const MCInst &Inst) const { return false; } - /// Return true if a pair of instructions represented by \p Insts - /// could be fused into a single uop. - virtual bool isMacroOpFusionPair(ArrayRef<MCInst> Insts) const { - llvm_unreachable("not implemented"); - return false; - } - struct X86MemOperand { unsigned BaseRegNum; int64_t ScaleImm; diff --git a/bolt/lib/Core/BinaryBasicBlock.cpp b/bolt/lib/Core/BinaryBasicBlock.cpp index a4b9a7f558cd8..2a2192b79bb4b 100644 --- a/bolt/lib/Core/BinaryBasicBlock.cpp +++ b/bolt/lib/Core/BinaryBasicBlock.cpp @@ -404,45 +404,6 @@ bool BinaryBasicBlock::analyzeBranch(const MCSymbol *&TBB, const MCSymbol *&FBB, CondBranch, UncondBranch); } -bool BinaryBasicBlock::isMacroOpFusionPair(const_iterator I) const { - auto &MIB = Function->getBinaryContext().MIB; - ArrayRef<MCInst> Insts = Instructions; - return MIB->isMacroOpFusionPair(Insts.slice(I - begin())); -} - -BinaryBasicBlock::const_iterator -BinaryBasicBlock::getMacroOpFusionPair() const { - if (!Function->getBinaryContext().isX86()) - return end(); - - if (getNumNonPseudos() < 2 || succ_size() != 2) - return end(); - - auto RI = getLastNonPseudo(); - assert(RI != rend() && "cannot have an empty block with 2 successors"); - - BinaryContext &BC = Function->getBinaryContext(); - - // Skip instruction if it's an unconditional branch following - // a conditional one. - if (BC.MIB->isUnconditionalBranch(*RI)) - ++RI; - - if (!BC.MIB->isConditionalBranch(*RI)) - return end(); - - // Start checking with instruction preceding the conditional branch. - ++RI; - if (RI == rend()) - return end(); - - auto II = std::prev(RI.base()); // convert to a forward iterator - if (isMacroOpFusionPair(II)) - return II; - - return end(); -} - MCInst *BinaryBasicBlock::getTerminatorBefore(MCInst *Pos) { BinaryContext &BC = Function->getBinaryContext(); auto Itr = rbegin(); diff --git a/bolt/lib/Core/BinaryEmitter.cpp b/bolt/lib/Core/BinaryEmitter.cpp index 5793963f9b80d..f6dfa249f9a9f 100644 --- a/bolt/lib/Core/BinaryEmitter.cpp +++ b/bolt/lib/Core/BinaryEmitter.cpp @@ -38,19 +38,6 @@ extern cl::opt<bool> PreserveBlocksAlignment; cl::opt<bool> AlignBlocks("align-blocks", cl::desc("align basic blocks"), cl::cat(BoltOptCategory)); -cl::opt<MacroFusionType> -AlignMacroOpFusion("align-macro-fusion", - cl::desc("fix instruction alignment for macro-fusion (x86 relocation mode)"), - cl::init(MFT_HOT), - cl::values(clEnumValN(MFT_NONE, "none", - "do not insert alignment no-ops for macro-fusion"), - clEnumValN(MFT_HOT, "hot", - "only insert alignment no-ops on hot execution paths (default)"), - clEnumValN(MFT_ALL, "all", - "always align instructions to allow macro-fusion")), - cl::ZeroOrMore, - cl::cat(BoltRelocCategory)); - static cl::list<std::string> BreakFunctionNames("break-funcs", cl::CommaSeparated, @@ -453,20 +440,7 @@ void BinaryEmitter::emitFunctionBody(BinaryFunction &BF, FunctionFragment &FF, Streamer.emitLabel(EntrySymbol); } - // Check if special alignment for macro-fusion is needed. - bool MayNeedMacroFusionAlignment = - (opts::AlignMacroOpFusion == MFT_ALL) || - (opts::AlignMacroOpFusion == MFT_HOT && BB->getKnownExecutionCount()); - BinaryBasicBlock::const_iterator MacroFusionPair; - if (MayNeedMacroFusionAlignment) { - MacroFusionPair = BB->getMacroOpFusionPair(); - if (MacroFusionPair == BB->end()) - MayNeedMacroFusionAlignment = false; - } - SMLoc LastLocSeen; - // Remember if the last instruction emitted was a prefix. - bool LastIsPrefix = false; for (auto I = BB->begin(), E = BB->end(); I != E; ++I) { MCInst &Instr = *I; @@ -479,16 +453,6 @@ void BinaryEmitter::emitFunctionBody(BinaryFunction &BF, FunctionFragment &FF, continue; } - // Handle macro-fusion alignment. If we emitted a prefix as - // the last instruction, we should've already emitted the associated - // alignment hint, so don't emit it twice. - if (MayNeedMacroFusionAlignment && !LastIsPrefix && - I == MacroFusionPair) { - // This assumes the second instruction in the macro-op pair will get - // assigned to its own MCRelaxableFragment. Since all JCC instructions - // are relaxable, we should be safe. - } - if (!EmitCodeOnly) { // A symbol to be emitted before the instruction to mark its location. MCSymbol *InstrLabel = BC.MIB->getInstLabel(Instr); @@ -525,7 +489,6 @@ void BinaryEmitter::emitFunctionBody(BinaryFunction &BF, FunctionFragment &FF, } Streamer.emitInstruction(Instr, *BC.STI); - LastIsPrefix = BC.MIB->isPrefix(Instr); } } diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp index d13e28999a05c..a316b8ee802ad 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -2276,8 +2276,6 @@ void BinaryFunction::postProcessCFG() { postProcessBranches(); } - calculateMacroOpFusionStats(); - // The final cleanup of intermediate structures. clearList(IgnoredBranches); @@ -2294,29 +2292,6 @@ void BinaryFunction::postProcessCFG() { "invalid CFG detected after post-processing"); } -void BinaryFunction::calculateMacroOpFusionStats() { - if (!getBinaryContext().isX86()) - return; - for (const BinaryBasicBlock &BB : blocks()) { - auto II = BB.getMacroOpFusionPair(); - if (II == BB.end()) - continue; - - // Check offset of the second instruction. - // FIXME: arch-specific. - const uint32_t Offset = BC.MIB->getOffsetWithDefault(*std::next(II), 0); - if (!Offset || (getAddress() + Offset) % 64) - continue; - - LLVM_DEBUG(dbgs() << "\nmissed macro-op fusion at address 0x" - << Twine::utohexstr(getAddress() + Offset) - << " in function " << *this << "; executed " - << BB.getKnownExecutionCount() << " times.\n"); - ++BC.Stats.MissedMacroFusionPairs; - BC.Stats.MissedMacroFusionExecCount += BB.getKnownExecutionCount(); - } -} - void BinaryFunction::removeTagsFromProfile() { for (BinaryBasicBlock *BB : BasicBlocks) { if (BB->ExecutionCount == BinaryBasicBlock::COUNT_NO_PROFILE) diff --git a/bolt/lib/Passes/BinaryPasses.cpp b/bolt/lib/Passes/BinaryPasses.cpp index 2810f723719d0..64aa312fd5114 100644 --- a/bolt/lib/Passes/BinaryPasses.cpp +++ b/bolt/lib/Passes/BinaryPasses.cpp @@ -44,7 +44,6 @@ namespace opts { extern cl::OptionCategory BoltCategory; extern cl::OptionCategory BoltOptCategory; -extern cl::opt<bolt::MacroFusionType> AlignMacroOpFusion; extern cl::opt<unsigned> Verbosity; extern cl::opt<bool> EnableBAT; extern cl::opt<unsigned> ExecutionCountThreshold; @@ -1635,25 +1634,6 @@ Error PrintProgramStats::runOnFunctions(BinaryContext &BC) { } } - // Print information on missed macro-fusion opportunities seen on input. - if (BC.Stats.MissedMacroFusionPairs) { - BC.outs() << format( - "BOLT-INFO: the input contains %zu (dynamic count : %zu)" - " opportunities for macro-fusion optimization", - BC.Stats.MissedMacroFusionPairs, BC.Stats.MissedMacroFusionExecCount); - switch (opts::AlignMacroOpFusion) { - case MFT_NONE: - BC.outs() << ". Use -align-macro-fusion to fix.\n"; - break; - case MFT_HOT: - BC.outs() << ". Will fix instances on a hot path.\n"; - break; - case MFT_ALL: - BC.outs() << " that are going to be fixed\n"; - break; - } - } - // Collect and print information about suboptimal code layout on input. if (opts::ReportBadLayout) { std::vector<BinaryFunction *> SuboptimalFuncs; diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp index 1a3a8af21d81b..e2925f4093746 100644 --- a/bolt/lib/Rewrite/RewriteInstance.cpp +++ b/bolt/lib/Rewrite/RewriteInstance.cpp @@ -76,7 +76,6 @@ extern cl::opt<bool> X86AlignBranchWithin32BBoundaries; namespace opts { -extern cl::opt<MacroFusionType> AlignMacroOpFusion; extern cl::list<std::string> HotTextMoveSections; extern cl::opt<bool> Hugify; extern cl::opt<bool> Instrument; @@ -1972,12 +1971,6 @@ void RewriteInstance::adjustCommandLineOptions() { if (RuntimeLibrary *RtLibrary = BC->getRuntimeLibrary()) RtLibrary->adjustCommandLineOptions(*BC); - if (opts::AlignMacroOpFusion != MFT_NONE && !BC->isX86()) { - BC->outs() - << "BOLT-INFO: disabling -align-macro-fusion on non-x86 platform\n"; - opts::AlignMacroOpFusion = MFT_NONE; - } - if (BC->isX86() && BC->MAB->allowAutoPadding()) { if (!BC->HasRelocations) { BC->errs() @@ -1988,13 +1981,6 @@ void RewriteInstance::adjustCommandLineOptions() { BC->outs() << "BOLT-WARNING: using mitigation for Intel JCC erratum, layout " "may take several minutes\n"; - opts::AlignMacroOpFusion = MFT_NONE; - } - - if (opts::AlignMacroOpFusion != MFT_NONE && !BC->HasRelocations) { - BC->outs() << "BOLT-INFO: disabling -align-macro-fusion in non-relocation " - "mode\n"; - opts::AlignMacroOpFusion = MFT_NONE; } if (opts::SplitEH && !BC->HasRelocations) { @@ -2016,14 +2002,6 @@ void RewriteInstance::adjustCommandLineOptions() { opts::StrictMode = true; } - if (BC->isX86() && BC->HasRelocations && - opts::AlignMacroOpFusion == MFT_HOT && !ProfileReader) { - BC->outs() - << "BOLT-INFO: enabling -align-macro-fusion=all since no profile " - "was specified\n"; - opts::AlignMacroOpFusion = MFT_ALL; - } - if (!BC->HasRelocations && opts::ReorderFunctions != ReorderFunctions::RT_NONE) { BC->errs() << "BOLT-ERROR: function reordering only works when " diff --git a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp index a74eda8e4a566..a21cf8f1cc244 100644 --- a/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp +++ b/bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp @@ -141,10 +141,6 @@ class AArch64MCPlusBuilder : public MCPlusBuilder { *AArch64ExprB.getSubExpr(), Comp); } - bool isMacroOpFusionPair(ArrayRef<MCInst> Insts) const override { - return false; - } - bool shortenInstruction(MCInst &, const MCSubtargetInfo &) const override { return false; } diff --git a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp index e350e701c7b7b..263e642e692eb 100644 --- a/bolt/lib/Target/X86/X86MCPlusBuilder.cpp +++ b/bolt/lib/Target/X86/X86MCPlusBuilder.cpp @@ -661,40 +661,6 @@ class X86MCPlusBuilder : public MCPlusBuilder { return (Desc.TSFlags & X86II::EncodingMask) == X86II::EVEX; } - bool isMacroOpFusionPair(ArrayRef<MCInst> Insts) const override { - const auto *I = Insts.begin(); - while (I != Insts.end() && isPrefix(*I)) - ++I; - if (I == Insts.end()) - return false; - - const MCInst &FirstInst = *I; - ++I; - while (I != Insts.end() && isPrefix(*I)) - ++I; - if (I == Insts.end()) - return false; - const MCInst &SecondInst = *I; - - if (!isConditionalBranch(SecondInst)) - return false; - // Cannot fuse if the first instruction uses RIP-relative memory. - if (hasPCRelOperand(FirstInst)) - return false; - - const X86::FirstMacroFusionInstKind CmpKind = - X86::classifyFirstOpcodeInMacroFusion(FirstInst.getOpcode()); - if (CmpKind == X86::FirstMacroFusionInstKind::Invalid) - return false; - - X86::CondCode CC = static_cast<X86::CondCode>(getCondCode(SecondInst)); - X86::SecondMacroFusionInstKind BranchKind = - X86::classifySecondCondCodeInMacroFusion(CC); - if (BranchKind == X86::SecondMacroFusionInstKind::Invalid) - return false; - return X86::isMacroFused(CmpKind, BranchKind); - } - std::optional<X86MemOperand> evaluateX86MemoryOperand(const MCInst &Inst) const override { int MemOpNo = getMemoryOperandNo(Inst); `````````` </details> https://github.com/llvm/llvm-project/pull/97358 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
