From 6304e38281d7f0546a6468dba7415ef52496e4c0 Mon Sep 17 00:00:00 2001 From: Fabian Parzefall Date: Wed, 24 Aug 2022 10:47:35 -0700 Subject: [PATCH] Revert "[BOLT] Track fragment info for all split fragments" This reverts commit 7e254818e49454a53bd00e3737007025b62d0f79. --- bolt/include/bolt/Core/BinaryFunction.h | 56 ++++++-- bolt/include/bolt/Core/FunctionLayout.h | 23 ---- bolt/lib/Core/BinaryFunction.cpp | 76 ++++++----- bolt/lib/Core/FunctionLayout.cpp | 12 +- bolt/lib/Profile/BoltAddressTranslation.cpp | 30 ++-- bolt/lib/Rewrite/RewriteInstance.cpp | 143 ++++++++------------ 6 files changed, 159 insertions(+), 181 deletions(-) diff --git a/bolt/include/bolt/Core/BinaryFunction.h b/bolt/include/bolt/Core/BinaryFunction.h index 6b1ded0ed100..c17662b65c9e 100644 --- a/bolt/include/bolt/Core/BinaryFunction.h +++ b/bolt/include/bolt/Core/BinaryFunction.h @@ -232,6 +232,9 @@ private: /// Size of the function in the output file. uint64_t OutputSize{0}; + /// Offset in the file. + uint64_t FileOffset{0}; + /// Maximum size this function is allowed to have. uint64_t MaxSize{std::numeric_limits::max()}; @@ -342,6 +345,14 @@ private: /// This attribute is only valid when hasCFG() == true. bool HasCanonicalCFG{true}; + /// The address for the code for this function in codegen memory. + /// Used for functions that are emitted in a dedicated section with a fixed + /// address. E.g. for functions that are overwritten in-place. + uint64_t ImageAddress{0}; + + /// The size of the code in memory. + uint64_t ImageSize{0}; + /// Name for the section this function code should reside in. std::string CodeSectionName; @@ -1049,9 +1060,7 @@ public: } /// Return offset of the function body in the binary file. - uint64_t getFileOffset() const { - return getLayout().getMainFragment().getFileOffset(); - } + uint64_t getFileOffset() const { return FileOffset; } /// Return (original) byte size of the function. uint64_t getSize() const { return Size; } @@ -1689,7 +1698,7 @@ public: int64_t NewOffset); BinaryFunction &setFileOffset(uint64_t Offset) { - getLayout().getMainFragment().setFileOffset(Offset); + FileOffset = Offset; return *this; } @@ -1776,24 +1785,20 @@ public: uint16_t getMaxColdAlignmentBytes() const { return MaxColdAlignmentBytes; } BinaryFunction &setImageAddress(uint64_t Address) { - getLayout().getMainFragment().setImageAddress(Address); + ImageAddress = Address; return *this; } /// Return the address of this function' image in memory. - uint64_t getImageAddress() const { - return getLayout().getMainFragment().getImageAddress(); - } + uint64_t getImageAddress() const { return ImageAddress; } BinaryFunction &setImageSize(uint64_t Size) { - getLayout().getMainFragment().setImageSize(Size); + ImageSize = Size; return *this; } /// Return the size of this function' image in memory. - uint64_t getImageSize() const { - return getLayout().getMainFragment().getImageSize(); - } + uint64_t getImageSize() const { return ImageSize; } /// Return true if the function is a secondary fragment of another function. bool isFragment() const { return IsFragment; } @@ -2297,6 +2302,33 @@ public: bool isAArch64Veneer() const; virtual ~BinaryFunction(); + + /// Info for fragmented functions. + class FragmentInfo { + private: + uint64_t Address{0}; + uint64_t ImageAddress{0}; + uint64_t ImageSize{0}; + uint64_t FileOffset{0}; + + public: + uint64_t getAddress() const { return Address; } + uint64_t getImageAddress() const { return ImageAddress; } + uint64_t getImageSize() const { return ImageSize; } + uint64_t getFileOffset() const { return FileOffset; } + + void setAddress(uint64_t VAddress) { Address = VAddress; } + void setImageAddress(uint64_t Address) { ImageAddress = Address; } + void setImageSize(uint64_t Size) { ImageSize = Size; } + void setFileOffset(uint64_t Offset) { FileOffset = Offset; } + }; + + /// Cold fragment of the function. + FragmentInfo ColdFragment; + + FragmentInfo &cold() { return ColdFragment; } + + const FragmentInfo &cold() const { return ColdFragment; } }; inline raw_ostream &operator<<(raw_ostream &OS, diff --git a/bolt/include/bolt/Core/FunctionLayout.h b/bolt/include/bolt/Core/FunctionLayout.h index 3a76126f812c..0917350ef5be 100644 --- a/bolt/include/bolt/Core/FunctionLayout.h +++ b/bolt/include/bolt/Core/FunctionLayout.h @@ -83,20 +83,6 @@ private: unsigned StartIndex; unsigned Size = 0; - /// Output address for the fragment. - uint64_t Address = 0; - - /// The address for the code for this fragment in codegen memory. Used for - /// functions that are emitted in a dedicated section with a fixed address, - /// e.g. for functions that are overwritten in-place. - uint64_t ImageAddress = 0; - - /// The size of the code in memory. - uint64_t ImageSize = 0; - - /// Offset in the file. - uint64_t FileOffset = 0; - FunctionFragment(FunctionLayout &Layout, FragmentNum Num); FunctionFragment(const FunctionFragment &) = default; FunctionFragment(FunctionFragment &&) = default; @@ -111,15 +97,6 @@ public: } bool isSplitFragment() const { return !isMainFragment(); } - uint64_t getAddress() const { return Address; } - void setAddress(uint64_t Value) { Address = Value; } - uint64_t getImageAddress() const { return ImageAddress; } - void setImageAddress(uint64_t Address) { ImageAddress = Address; } - uint64_t getImageSize() const { return ImageSize; } - void setImageSize(uint64_t Size) { ImageSize = Size; } - uint64_t getFileOffset() const { return FileOffset; } - void setFileOffset(uint64_t Offset) { FileOffset = Offset; } - unsigned size() const { return Size; }; bool empty() const { return size() == 0; }; iterator begin(); diff --git a/bolt/lib/Core/BinaryFunction.cpp b/bolt/lib/Core/BinaryFunction.cpp index cde56ff180a6..6bd06f058f4d 100644 --- a/bolt/lib/Core/BinaryFunction.cpp +++ b/bolt/lib/Core/BinaryFunction.cpp @@ -425,19 +425,19 @@ void BinaryFunction::print(raw_ostream &OS, std::string Annotation, Sep = "\n "; } } - OS << "\n Number : " << FunctionNumber; - OS << "\n State : " << CurrentState; - OS << "\n Address : 0x" << Twine::utohexstr(Address); - OS << "\n Size : 0x" << Twine::utohexstr(Size); - OS << "\n MaxSize : 0x" << Twine::utohexstr(MaxSize); - OS << "\n Offset : 0x" << Twine::utohexstr(getFileOffset()); - OS << "\n Section : " << SectionName; - OS << "\n Orc Section : " << getCodeSectionName(); - OS << "\n LSDA : 0x" << Twine::utohexstr(getLSDAAddress()); - OS << "\n IsSimple : " << IsSimple; - OS << "\n IsMultiEntry: " << isMultiEntry(); - OS << "\n IsSplit : " << isSplit(); - OS << "\n BB Count : " << size(); + OS << "\n Number : " << FunctionNumber + << "\n State : " << CurrentState + << "\n Address : 0x" << Twine::utohexstr(Address) + << "\n Size : 0x" << Twine::utohexstr(Size) + << "\n MaxSize : 0x" << Twine::utohexstr(MaxSize) + << "\n Offset : 0x" << Twine::utohexstr(FileOffset) + << "\n Section : " << SectionName + << "\n Orc Section : " << getCodeSectionName() + << "\n LSDA : 0x" << Twine::utohexstr(getLSDAAddress()) + << "\n IsSimple : " << IsSimple + << "\n IsMultiEntry: " << isMultiEntry() + << "\n IsSplit : " << isSplit() + << "\n BB Count : " << size(); if (HasFixedIndirectBranch) OS << "\n HasFixedIndirectBranch : true"; @@ -473,8 +473,8 @@ void BinaryFunction::print(raw_ostream &OS, std::string Annotation, for (const BinaryBasicBlock *BB : Layout.blocks()) OS << LS << BB->getName(); } - if (getImageAddress()) - OS << "\n Image : 0x" << Twine::utohexstr(getImageAddress()); + if (ImageAddress) + OS << "\n Image : 0x" << Twine::utohexstr(ImageAddress); if (ExecutionCount != COUNT_NO_PROFILE) { OS << "\n Exec Count : " << ExecutionCount; OS << "\n Profile Acc : " << format("%.1f%%", ProfileMatchRatio * 100.0f); @@ -4062,7 +4062,7 @@ void BinaryFunction::updateOutputValues(const MCAsmLayout &Layout) { setOutputDataAddress(BaseAddress + DataOffset); } if (isSplit()) { - for (FunctionFragment &FF : getLayout().getSplitFragments()) { + for (const FunctionFragment &FF : getLayout().getSplitFragments()) { ErrorOr ColdSection = getCodeSection(FF.getFragmentNum()); // If fragment is empty, cold section might not exist @@ -4084,8 +4084,8 @@ void BinaryFunction::updateOutputValues(const MCAsmLayout &Layout) { const uint64_t ColdStartOffset = Layout.getSymbolOffset(*ColdStartSymbol); const uint64_t ColdEndOffset = Layout.getSymbolOffset(*ColdEndSymbol); - FF.setAddress(ColdBaseAddress + ColdStartOffset); - FF.setImageSize(ColdEndOffset - ColdStartOffset); + cold().setAddress(ColdBaseAddress + ColdStartOffset); + cold().setImageSize(ColdEndOffset - ColdStartOffset); if (hasConstantIsland()) { const uint64_t DataOffset = Layout.getSymbolOffset(*getFunctionColdConstantIslandLabel()); @@ -4112,39 +4112,44 @@ void BinaryFunction::updateOutputValues(const MCAsmLayout &Layout) { if (getLayout().block_empty()) return; - for (FunctionFragment &FF : getLayout().fragments()) { - if (FF.empty()) - continue; + assert((getLayout().isHotColdSplit() || + (cold().getAddress() == 0 && cold().getImageSize() == 0 && + BC.HasRelocations)) && + "Function must be split two ways or cold fragment must have no " + "address (only in relocation mode)"); + BinaryBasicBlock *PrevBB = nullptr; + for (FunctionFragment &FF : getLayout().fragments()) { const uint64_t FragmentBaseAddress = getCodeSection(isSimple() ? FF.getFragmentNum() : FragmentNum::main()) ->getOutputAddress(); - - BinaryBasicBlock *PrevBB = nullptr; for (BinaryBasicBlock *const BB : FF) { assert(BB->getLabel()->isDefined() && "symbol should be defined"); if (!BC.HasRelocations) { - if (BB->isSplit()) - assert(FragmentBaseAddress == FF.getAddress()); - else + if (BB->isSplit()) { + assert(FragmentBaseAddress == cold().getAddress()); + } else { assert(FragmentBaseAddress == getOutputAddress()); + } } - const uint64_t BBOffset = Layout.getSymbolOffset(*BB->getLabel()); const uint64_t BBAddress = FragmentBaseAddress + BBOffset; BB->setOutputStartAddress(BBAddress); - if (PrevBB) - PrevBB->setOutputEndAddress(BBAddress); + if (PrevBB) { + uint64_t PrevBBEndAddress = BBAddress; + if (BB->isSplit() != PrevBB->isSplit()) + PrevBBEndAddress = getOutputAddress() + getOutputSize(); + PrevBB->setOutputEndAddress(PrevBBEndAddress); + } PrevBB = BB; BB->updateOutputValues(Layout); } - - PrevBB->setOutputEndAddress(PrevBB->isSplit() - ? FF.getAddress() + FF.getImageSize() - : getOutputAddress() + getOutputSize()); } + PrevBB->setOutputEndAddress(PrevBB->isSplit() + ? cold().getAddress() + cold().getImageSize() + : getOutputAddress() + getOutputSize()); } DebugAddressRangesVector BinaryFunction::getOutputAddressRanges() const { @@ -4160,9 +4165,8 @@ DebugAddressRangesVector BinaryFunction::getOutputAddressRanges() const { getOutputAddress() + getOutputSize()); if (isSplit()) { assert(isEmitted() && "split function should be emitted"); - for (const FunctionFragment &FF : getLayout().getSplitFragments()) - OutputRanges.emplace_back(FF.getAddress(), - FF.getAddress() + FF.getImageSize()); + OutputRanges.emplace_back(cold().getAddress(), + cold().getAddress() + cold().getImageSize()); } if (isSimple()) diff --git a/bolt/lib/Core/FunctionLayout.cpp b/bolt/lib/Core/FunctionLayout.cpp index 2ccf82ce7270..d37c43047eb2 100644 --- a/bolt/lib/Core/FunctionLayout.cpp +++ b/bolt/lib/Core/FunctionLayout.cpp @@ -204,14 +204,10 @@ bool FunctionLayout::update(const ArrayRef NewLayout) { void FunctionLayout::clear() { Blocks = BasicBlockListType(); - // If the binary does not have relocations and is not split, the function will - // be written to the output stream at its original file offset (see - // `RewriteInstance::rewriteFile`). Hence, when the layout is cleared, retain - // the main fragment, so that this information is not lost. - std::for_each(Fragments.begin() + 1, Fragments.end(), - [](FunctionFragment *const FF) { delete FF; }); - Fragments = FragmentListType{Fragments.front()}; - getMainFragment().Size = 0; + for (FunctionFragment *const FF : Fragments) + delete FF; + Fragments = FragmentListType(); + addFragment(); } const BinaryBasicBlock * diff --git a/bolt/lib/Profile/BoltAddressTranslation.cpp b/bolt/lib/Profile/BoltAddressTranslation.cpp index f475f173e021..305962391ce3 100644 --- a/bolt/lib/Profile/BoltAddressTranslation.cpp +++ b/bolt/lib/Profile/BoltAddressTranslation.cpp @@ -73,27 +73,29 @@ void BoltAddressTranslation::write(const BinaryContext &BC, raw_ostream &OS) { LLVM_DEBUG(dbgs() << "Function name: " << Function.getPrintName() << "\n"); LLVM_DEBUG(dbgs() << " Address reference: 0x" << Twine::utohexstr(Function.getOutputAddress()) << "\n"); - MapTy Map; - for (const BinaryBasicBlock *const BB : - Function.getLayout().getMainFragment()) + const bool IsSplit = Function.isSplit(); + for (const BinaryBasicBlock *const BB : Function.getLayout().blocks()) { + if (IsSplit && BB->isCold()) + break; writeEntriesForBB(Map, *BB, Function.getOutputAddress()); - Maps.emplace(Function.getOutputAddress(), std::move(Map)); + } + Maps.insert(std::pair(Function.getOutputAddress(), Map)); - if (!Function.isSplit()) + if (!IsSplit) continue; - // Split maps + // Cold map + Map.clear(); LLVM_DEBUG(dbgs() << " Cold part\n"); - for (const FunctionFragment &FF : - Function.getLayout().getSplitFragments()) { - Map.clear(); - for (const BinaryBasicBlock *const BB : FF) - writeEntriesForBB(Map, *BB, FF.getAddress()); - - Maps.emplace(FF.getAddress(), std::move(Map)); - ColdPartSource.emplace(FF.getAddress(), Function.getOutputAddress()); + for (const BinaryBasicBlock *const BB : Function.getLayout().blocks()) { + if (!BB->isCold()) + continue; + writeEntriesForBB(Map, *BB, Function.cold().getAddress()); } + Maps.insert(std::pair(Function.cold().getAddress(), Map)); + ColdPartSource.insert(std::pair( + Function.cold().getAddress(), Function.getOutputAddress())); } const uint32_t NumFuncs = Maps.size(); diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp index 565b3e3c9aef..6b050dbf9d54 100644 --- a/bolt/lib/Rewrite/RewriteInstance.cpp +++ b/bolt/lib/Rewrite/RewriteInstance.cpp @@ -3726,38 +3726,38 @@ void RewriteInstance::mapCodeSections(RuntimeDyld &RTDyld) { if (!Function.isSplit()) continue; - assert(Function.getLayout().isHotColdSplit() && - "Cannot allocate more than two fragments per function in " - "non-relocation mode."); + for (const FunctionFragment &FF : + Function.getLayout().getSplitFragments()) { + ErrorOr ColdSection = + Function.getCodeSection(FF.getFragmentNum()); + assert(ColdSection && "cannot find section for cold part"); + // Cold fragments are aligned at 16 bytes. + NextAvailableAddress = alignTo(NextAvailableAddress, 16); + BinaryFunction::FragmentInfo &ColdPart = Function.cold(); + if (TooLarge) { + // The corresponding FDE will refer to address 0. + ColdPart.setAddress(0); + ColdPart.setImageAddress(0); + ColdPart.setImageSize(0); + ColdPart.setFileOffset(0); + } else { + ColdPart.setAddress(NextAvailableAddress); + ColdPart.setImageAddress(ColdSection->getAllocAddress()); + ColdPart.setImageSize(ColdSection->getOutputSize()); + ColdPart.setFileOffset(getFileOffsetForAddress(NextAvailableAddress)); + ColdSection->setOutputAddress(ColdPart.getAddress()); + } - FunctionFragment &FF = - Function.getLayout().getFragment(FragmentNum::cold()); - ErrorOr ColdSection = - Function.getCodeSection(FF.getFragmentNum()); - assert(ColdSection && "cannot find section for cold part"); - // Cold fragments are aligned at 16 bytes. - NextAvailableAddress = alignTo(NextAvailableAddress, 16); - if (TooLarge) { - // The corresponding FDE will refer to address 0. - FF.setAddress(0); - FF.setImageAddress(0); - FF.setImageSize(0); - FF.setFileOffset(0); - } else { - FF.setAddress(NextAvailableAddress); - FF.setImageAddress(ColdSection->getAllocAddress()); - FF.setImageSize(ColdSection->getOutputSize()); - FF.setFileOffset(getFileOffsetForAddress(NextAvailableAddress)); - ColdSection->setOutputAddress(FF.getAddress()); + LLVM_DEBUG(dbgs() << "BOLT: mapping cold fragment 0x" + << Twine::utohexstr(ColdPart.getImageAddress()) + << " to 0x" << Twine::utohexstr(ColdPart.getAddress()) + << " with size " + << Twine::utohexstr(ColdPart.getImageSize()) << '\n'); + RTDyld.reassignSectionAddress(ColdSection->getSectionID(), + ColdPart.getAddress()); + + NextAvailableAddress += ColdPart.getImageSize(); } - - LLVM_DEBUG( - dbgs() << formatv( - "BOLT: mapping cold fragment {0:x+} to {1:x+} with size {2:x+}\n", - FF.getImageAddress(), FF.getAddress(), FF.getImageSize())); - RTDyld.reassignSectionAddress(ColdSection->getSectionID(), FF.getAddress()); - - NextAvailableAddress += FF.getImageSize(); } // Add the new text section aggregating all existing code sections. @@ -4518,22 +4518,20 @@ void RewriteInstance::updateELFSymbolTable( ICFSymbol.st_shndx = ICFParent->getCodeSection()->getIndex(); Symbols.emplace_back(ICFSymbol); } - if (Function.isSplit()) { + if (Function.isSplit() && Function.cold().getAddress()) { for (const FunctionFragment &FF : Function.getLayout().getSplitFragments()) { - if (FF.getAddress()) { - ELFSymTy NewColdSym = FunctionSymbol; - const SmallString<256> SymbolName = formatv( - "{0}.cold.{1}", cantFail(FunctionSymbol.getName(StringSection)), - FF.getFragmentNum().get() - 1); - NewColdSym.st_name = AddToStrTab(SymbolName); - NewColdSym.st_shndx = - Function.getCodeSection(FF.getFragmentNum())->getIndex(); - NewColdSym.st_value = FF.getAddress(); - NewColdSym.st_size = FF.getImageSize(); - NewColdSym.setBindingAndType(ELF::STB_LOCAL, ELF::STT_FUNC); - Symbols.emplace_back(NewColdSym); - } + ELFSymTy NewColdSym = FunctionSymbol; + const SmallString<256> SymbolName = formatv( + "{0}.cold.{1}", cantFail(FunctionSymbol.getName(StringSection)), + FF.getFragmentNum().get() - 1); + NewColdSym.st_name = AddToStrTab(SymbolName); + NewColdSym.st_shndx = + Function.getCodeSection(FF.getFragmentNum())->getIndex(); + NewColdSym.st_value = Function.cold().getAddress(); + NewColdSym.st_size = Function.cold().getImageSize(); + NewColdSym.setBindingAndType(ELF::STB_LOCAL, ELF::STT_FUNC); + Symbols.emplace_back(NewColdSym); } } if (Function.hasConstantIsland()) { @@ -4658,26 +4656,11 @@ void RewriteInstance::updateELFSymbolTable( NewSymbol.st_value = OutputAddress; // Force secondary entry points to have zero size. NewSymbol.st_size = 0; - - // Find fragment containing entrypoint - FunctionLayout::fragment_const_iterator FF = llvm::find_if( - Function->getLayout().fragments(), [&](const FunctionFragment &FF) { - uint64_t Lo = FF.getAddress(); - uint64_t Hi = Lo + FF.getImageSize(); - return Lo <= OutputAddress && OutputAddress < Hi; - }); - - if (FF == Function->getLayout().fragment_end()) { - assert( - OutputAddress >= Function->getCodeSection()->getOutputAddress() && - OutputAddress < (Function->getCodeSection()->getOutputAddress() + - Function->getCodeSection()->getOutputSize()) && - "Cannot locate fragment containg secondary entrypoint"); - FF = Function->getLayout().fragment_begin(); - } - NewSymbol.st_shndx = - Function->getCodeSection(FF->getFragmentNum())->getIndex(); + OutputAddress >= Function->cold().getAddress() && + OutputAddress < Function->cold().getImageSize() + ? Function->getCodeSection(FragmentNum::cold())->getIndex() + : Function->getCodeSection()->getIndex(); } else { // Check if the symbol belongs to moved data object and update it. BinaryData *BD = opts::ReorderData.empty() @@ -4782,10 +4765,8 @@ void RewriteInstance::updateELFSymbolTable( SmallVector Buf; NewColdSym.st_name = AddToStrTab( Twine(Function->getPrintName()).concat(".cold.0").toStringRef(Buf)); - const FunctionFragment &ColdFF = - Function->getLayout().getFragment(FragmentNum::cold()); - NewColdSym.st_value = ColdFF.getAddress(); - NewColdSym.st_size = ColdFF.getImageSize(); + NewColdSym.st_value = Function->cold().getAddress(); + NewColdSym.st_size = Function->cold().getImageSize(); Symbols.emplace_back(NewColdSym); } } @@ -5311,21 +5292,9 @@ void RewriteInstance::rewriteFile() { continue; } - const auto HasAddress = [](const FunctionFragment &FF) { - return FF.empty() || - (FF.getImageAddress() != 0 && FF.getImageSize() != 0); - }; - const bool SplitFragmentsHaveAddress = - llvm::all_of(Function->getLayout().getSplitFragments(), HasAddress); - if (Function->isSplit() && !SplitFragmentsHaveAddress) { - const auto HasNoAddress = [](const FunctionFragment &FF) { - return FF.getImageAddress() == 0 && FF.getImageSize() == 0; - }; - assert(llvm::all_of(Function->getLayout().getSplitFragments(), - HasNoAddress) && - "Some split fragments have an address while others do not"); + if (Function->isSplit() && (Function->cold().getImageAddress() == 0 || + Function->cold().getImageSize() == 0)) continue; - } OverwrittenScore += Function->getFunctionScore(); // Overwrite function in the output file. @@ -5357,14 +5326,12 @@ void RewriteInstance::rewriteFile() { // Write cold part if (opts::Verbosity >= 2) - outs() << formatv("BOLT: rewriting function \"{0}\" (split parts)\n", - *Function); + outs() << "BOLT: rewriting function \"" << *Function + << "\" (cold part)\n"; - for (const FunctionFragment &FF : - Function->getLayout().getSplitFragments()) { - OS.pwrite(reinterpret_cast(FF.getImageAddress()), - FF.getImageSize(), FF.getFileOffset()); - } + OS.pwrite(reinterpret_cast(Function->cold().getImageAddress()), + Function->cold().getImageSize(), + Function->cold().getFileOffset()); ++CountOverwrittenFunctions; if (opts::MaxFunctions && CountOverwrittenFunctions == opts::MaxFunctions) {