<div dir="ltr">I did not benchmark these particular patches as it seems neutral to performance to me. Do you have any concerns? I think as we improved performance in the last few months, we actually have some room to aim for readability and simplicity even if it slows down the linker a little bit.<div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 31, 2017 at 7:28 AM, Rafael Avila de Espindola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
Have you benchmarked any of these patches? I am going over commits and<br>
in the last two weeks I found two very noticeable regressions.<br>
<br>
I don't have the time or the motivation to manually benchmark every<br>
change. That burden has to be on the author until we setup a bot.<br>
<br>
In fact, I think that we must slow down development until we have a<br>
reasonable performance tracking.<br>
<br>
Cheers,<br>
Rafael<br>
<div class="m_-7024970141806714859HOEnZb"><div class="m_-7024970141806714859h5"><br>
Rui Ueyama via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> writes:<br>
<br>
> Author: ruiu<br>
> Date: Thu Oct 26 20:13:39 2017<br>
> New Revision: 316731<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=316731&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=316731&view=rev</a><br>
> Log:<br>
> De-template EhFrameSection. NFC.<br>
><br>
> Modified:<br>
> lld/trunk/ELF/InputSection.h<br>
> lld/trunk/ELF/SyntheticSectio<wbr>ns.cpp<br>
> lld/trunk/ELF/SyntheticSectio<wbr>ns.h<br>
> lld/trunk/ELF/Writer.cpp<br>
><br>
> Modified: lld/trunk/ELF/InputSection.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/InputSection.h?rev=316731&r1=316730&r2=316731&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/lld/trunk/ELF/InputSecti<wbr>on.h?rev=316731&r1=316730&r2=<wbr>316731&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- lld/trunk/ELF/InputSection.h (original)<br>
> +++ lld/trunk/ELF/InputSection.h Thu Oct 26 20:13:39 2017<br>
> @@ -30,7 +30,6 @@ struct SectionPiece;<br>
><br>
> class DefinedRegular;<br>
> class SyntheticSection;<br>
> -template <class ELFT> class EhFrameSection;<br>
> class MergeSyntheticSection;<br>
> template <class ELFT> class ObjFile;<br>
> class OutputSection;<br>
><br>
> Modified: lld/trunk/ELF/SyntheticSection<wbr>s.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/SyntheticSections.cpp?rev=316731&r1=316730&r2=316731&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/lld/trunk/ELF/SyntheticS<wbr>ections.cpp?rev=316731&r1=<wbr>316730&r2=316731&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- lld/trunk/ELF/SyntheticSection<wbr>s.cpp (original)<br>
> +++ lld/trunk/ELF/SyntheticSection<wbr>s.cpp Thu Oct 26 20:13:39 2017<br>
> @@ -397,17 +397,14 @@ void BuildIdSection::writeBuildId(A<wbr>rrayR<br>
> }<br>
> }<br>
><br>
> -template <class ELFT><br>
> -EhFrameSection<ELFT>::EhFrame<wbr>Section()<br>
> +EhFrameSection::EhFrameSectio<wbr>n()<br>
> : SyntheticSection(SHF_ALLOC, SHT_PROGBITS, 1, ".eh_frame") {}<br>
><br>
> // Search for an existing CIE record or create a new one.<br>
> // CIE records from input object files are uniquified by their contents<br>
> // and where their relocations point to.<br>
> -template <class ELFT><br>
> -template <class RelTy><br>
> -CieRecord *EhFrameSection<ELFT>::addCie(<wbr>EhSectionPiece &Cie,<br>
> - ArrayRef<RelTy> Rels) {<br>
> +template <class ELFT, class RelTy><br>
> +CieRecord *EhFrameSection::addCie(EhSect<wbr>ionPiece &Cie, ArrayRef<RelTy> Rels) {<br>
> auto *Sec = cast<EhInputSection>(Cie.Sec);<br>
> if (read32(Cie.data().data() + 4, Config->Endianness) != 0)<br>
> fatal(toString(Sec) + ": CIE expected at beginning of .eh_frame");<br>
> @@ -432,10 +429,8 @@ CieRecord *EhFrameSection<ELFT>::addCie(<br>
><br>
> // There is one FDE per function. Returns true if a given FDE<br>
> // points to a live function.<br>
> -template <class ELFT><br>
> -template <class RelTy><br>
> -bool EhFrameSection<ELFT>::isFdeLiv<wbr>e(EhSectionPiece &Fde,<br>
> - ArrayRef<RelTy> Rels) {<br>
> +template <class ELFT, class RelTy><br>
> +bool EhFrameSection::isFdeLive(EhSe<wbr>ctionPiece &Fde, ArrayRef<RelTy> Rels) {<br>
> auto *Sec = cast<EhInputSection>(Fde.Sec);<br>
> unsigned FirstRelI = Fde.FirstRelocation;<br>
><br>
> @@ -461,10 +456,8 @@ bool EhFrameSection<ELFT>::isFdeLiv<wbr>e(EhS<br>
> // is one CIE record per input object file which is followed by<br>
> // a list of FDEs. This function searches an existing CIE or create a new<br>
> // one and associates FDEs to the CIE.<br>
> -template <class ELFT><br>
> -template <class RelTy><br>
> -void EhFrameSection<ELFT>::addSecti<wbr>onAux(EhInputSection *Sec,<br>
> - ArrayRef<RelTy> Rels) {<br>
> +template <class ELFT, class RelTy><br>
> +void EhFrameSection::addSectionAux(<wbr>EhInputSection *Sec, ArrayRef<RelTy> Rels) {<br>
> DenseMap<size_t, CieRecord *> OffsetToCie;<br>
> for (EhSectionPiece &Piece : Sec->Pieces) {<br>
> // The empty record is the end marker.<br>
> @@ -474,7 +467,7 @@ void EhFrameSection<ELFT>::addSecti<wbr>onAux<br>
> size_t Offset = Piece.InputOff;<br>
> uint32_t ID = read32(Piece.data().data() + 4, Config->Endianness);<br>
> if (ID == 0) {<br>
> - OffsetToCie[Offset] = addCie(Piece, Rels);<br>
> + OffsetToCie[Offset] = addCie<ELFT>(Piece, Rels);<br>
> continue;<br>
> }<br>
><br>
> @@ -483,15 +476,14 @@ void EhFrameSection<ELFT>::addSecti<wbr>onAux<br>
> if (!Rec)<br>
> fatal(toString(Sec) + ": invalid CIE reference");<br>
><br>
> - if (!isFdeLive(Piece, Rels))<br>
> + if (!isFdeLive<ELFT>(Piece, Rels))<br>
> continue;<br>
> Rec->Fdes.push_back(&Piece);<br>
> NumFdes++;<br>
> }<br>
> }<br>
><br>
> -template <class ELFT><br>
> -void EhFrameSection<ELFT>::addSecti<wbr>on(InputSectionBase *C) {<br>
> +template <class ELFT> void EhFrameSection::addSection(Inp<wbr>utSectionBase *C) {<br>
> auto *Sec = cast<EhInputSection>(C);<br>
> Sec->Parent = this;<br>
><br>
> @@ -509,9 +501,9 @@ void EhFrameSection<ELFT>::addSecti<wbr>on(In<br>
> return;<br>
><br>
> if (Sec->AreRelocsRela)<br>
> - addSectionAux(Sec, Sec->template relas<ELFT>());<br>
> + addSectionAux<ELFT>(Sec, Sec->template relas<ELFT>());<br>
> else<br>
> - addSectionAux(Sec, Sec->template rels<ELFT>());<br>
> + addSectionAux<ELFT>(Sec, Sec->template rels<ELFT>());<br>
> }<br>
><br>
> static void writeCieFde(uint8_t *Buf, ArrayRef<uint8_t> D) {<br>
> @@ -526,7 +518,7 @@ static void writeCieFde(uint8_t *Buf, Ar<br>
> write32(Buf, Aligned - 4, Config->Endianness);<br>
> }<br>
><br>
> -template <class ELFT> void EhFrameSection<ELFT>::finalize<wbr>Contents() {<br>
> +void EhFrameSection::finalizeConten<wbr>ts() {<br>
> if (this->Size)<br>
> return; // Already finalized.<br>
><br>
> @@ -554,8 +546,7 @@ template <class ELFT> void EhFrameSectio<br>
> // to get an FDE from an address to which FDE is applied. This function<br>
> // returns a list of such pairs.<br>
> template <class ELFT><br>
> -std::vector<typename EhFrameSection<ELFT>::FdeData><br>
> -EhFrameSection<ELFT>::getFdeD<wbr>ata() const {<br>
> +std::vector<EhFrameSection::F<wbr>deData> EhFrameSection::getFdeData() const {<br>
> uint8_t *Buf = getParent()->Loc + OutSecOff;<br>
> std::vector<FdeData> Ret;<br>
><br>
> @@ -586,9 +577,8 @@ static uint64_t readFdeAddr(uint8_t *Buf<br>
><br>
> // Returns the VA to which a given FDE (on a mmap'ed buffer) is applied to.<br>
> // We need it to create .eh_frame_hdr section.<br>
> -template <class ELFT><br>
> -uint64_t EhFrameSection<ELFT>::getFdePc<wbr>(uint8_t *Buf, size_t FdeOff,<br>
> - uint8_t Enc) const {<br>
> +uint64_t EhFrameSection::getFdePc(uint8<wbr>_t *Buf, size_t FdeOff,<br>
> + uint8_t Enc) const {<br>
> // The starting address to which this FDE applies is<br>
> // stored at FDE + 8 byte.<br>
> size_t Off = FdeOff + 8;<br>
> @@ -600,7 +590,7 @@ uint64_t EhFrameSection<ELFT>::getFdePc<wbr>(<br>
> fatal("unknown FDE size relative encoding");<br>
> }<br>
><br>
> -template <class ELFT> void EhFrameSection<ELFT>::writeTo(<wbr>uint8_t *Buf) {<br>
> +void EhFrameSection::writeTo(uint8_<wbr>t *Buf) {<br>
> // Write CIE and FDE records.<br>
> for (CieRecord *Rec : CieRecords) {<br>
> size_t CieOffset = Rec->Cie->OutputOff;<br>
> @@ -1972,10 +1962,10 @@ EhFrameHeader<ELFT>::EhFrameHe<wbr>ader()<br>
> // the starting PC from where FDEs covers, and the FDE's address.<br>
> // It is sorted by PC.<br>
> template <class ELFT> void EhFrameHeader<ELFT>::writeTo(u<wbr>int8_t *Buf) {<br>
> - typedef typename EhFrameSection<ELFT>::FdeData FdeData;<br>
> + typedef EhFrameSection::FdeData FdeData;<br>
> const endianness E = ELFT::TargetEndianness;<br>
><br>
> - std::vector<FdeData> Fdes = In<ELFT>::EhFrame->getFdeData(<wbr>);<br>
> + std::vector<FdeData> Fdes = InX::EhFrame->getFdeData<ELFT><wbr>();<br>
><br>
> // Sort the FDE list by their PC and uniqueify. Usually there is only<br>
> // one FDE for a PC (i.e. function), but if ICF merges two functions<br>
> @@ -1989,7 +1979,7 @@ template <class ELFT> void EhFrameHeader<br>
> Buf[1] = DW_EH_PE_pcrel | DW_EH_PE_sdata4;<br>
> Buf[2] = DW_EH_PE_udata4;<br>
> Buf[3] = DW_EH_PE_datarel | DW_EH_PE_sdata4;<br>
> - write32<E>(Buf + 4, In<ELFT>::EhFrame->getParent()<wbr>->Addr - this->getVA() - 4);<br>
> + write32<E>(Buf + 4, InX::EhFrame->getParent()->Add<wbr>r - this->getVA() - 4);<br>
> write32<E>(Buf + 8, Fdes.size());<br>
> Buf += 12;<br>
><br>
> @@ -2003,11 +1993,11 @@ template <class ELFT> void EhFrameHeader<br>
><br>
> template <class ELFT> size_t EhFrameHeader<ELFT>::getSize() const {<br>
> // .eh_frame_hdr has a 12 bytes header followed by an array of FDEs.<br>
> - return 12 + In<ELFT>::EhFrame->NumFdes * 8;<br>
> + return 12 + InX::EhFrame->NumFdes * 8;<br>
> }<br>
><br>
> template <class ELFT> bool EhFrameHeader<ELFT>::empty() const {<br>
> - return In<ELFT>::EhFrame->empty();<br>
> + return InX::EhFrame->empty();<br>
> }<br>
><br>
> template <class ELFT><br>
> @@ -2412,6 +2402,7 @@ InputSection *InX::ARMAttributes;<br>
> BssSection *InX::Bss;<br>
> BssSection *InX::BssRelRo;<br>
> BuildIdSection *InX::BuildId;<br>
> +EhFrameSection *InX::EhFrame;<br>
> SyntheticSection *InX::Dynamic;<br>
> StringTableSection *InX::DynStrTab;<br>
> SymbolTableBaseSection *InX::DynSymTab;<br>
> @@ -2435,6 +2426,11 @@ template GdbIndexSection *elf::createGdb<br>
> template GdbIndexSection *elf::createGdbIndex<ELF64LE>(<wbr>);<br>
> template GdbIndexSection *elf::createGdbIndex<ELF64BE>(<wbr>);<br>
><br>
> +template void EhFrameSection::addSection<ELF<wbr>32LE>(InputSectionBase *);<br>
> +template void EhFrameSection::addSection<ELF<wbr>32BE>(InputSectionBase *);<br>
> +template void EhFrameSection::addSection<ELF<wbr>64LE>(InputSectionBase *);<br>
> +template void EhFrameSection::addSection<ELF<wbr>64BE>(InputSectionBase *);<br>
> +<br>
> template void PltSection::addEntry<ELF32LE>(<wbr>SymbolBody &Sym);<br>
> template void PltSection::addEntry<ELF32BE>(<wbr>SymbolBody &Sym);<br>
> template void PltSection::addEntry<ELF64LE>(<wbr>SymbolBody &Sym);<br>
> @@ -2499,8 +2495,3 @@ template class elf::VersionDefinitionSec<br>
> template class elf::VersionDefinitionSection<<wbr>ELF32BE>;<br>
> template class elf::VersionDefinitionSection<<wbr>ELF64LE>;<br>
> template class elf::VersionDefinitionSection<<wbr>ELF64BE>;<br>
> -<br>
> -template class elf::EhFrameSection<ELF32LE>;<br>
> -template class elf::EhFrameSection<ELF32BE>;<br>
> -template class elf::EhFrameSection<ELF64LE>;<br>
> -template class elf::EhFrameSection<ELF64BE>;<br>
><br>
> Modified: lld/trunk/ELF/SyntheticSection<wbr>s.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/SyntheticSections.h?rev=316731&r1=316730&r2=316731&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/lld/trunk/ELF/SyntheticS<wbr>ections.h?rev=316731&r1=<wbr>316730&r2=316731&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- lld/trunk/ELF/SyntheticSection<wbr>s.h (original)<br>
> +++ lld/trunk/ELF/SyntheticSection<wbr>s.h Thu Oct 26 20:13:39 2017<br>
> @@ -62,11 +62,7 @@ struct CieRecord {<br>
> };<br>
><br>
> // Section for .eh_frame.<br>
> -template <class ELFT> class EhFrameSection final : public SyntheticSection {<br>
> - typedef typename ELFT::Shdr Elf_Shdr;<br>
> - typedef typename ELFT::Rel Elf_Rel;<br>
> - typedef typename ELFT::Rela Elf_Rela;<br>
> -<br>
> +class EhFrameSection final : public SyntheticSection {<br>
> public:<br>
> EhFrameSection();<br>
> void writeTo(uint8_t *Buf) override;<br>
> @@ -74,7 +70,7 @@ public:<br>
> bool empty() const override { return Sections.empty(); }<br>
> size_t getSize() const override { return Size; }<br>
><br>
> - void addSection(InputSectionBase *S);<br>
> + template <class ELFT> void addSection(InputSectionBase *S);<br>
><br>
> std::vector<EhInputSection *> Sections;<br>
> size_t NumFdes = 0;<br>
> @@ -84,17 +80,18 @@ public:<br>
> uint32_t FdeVA;<br>
> };<br>
><br>
> - std::vector<FdeData> getFdeData() const;<br>
> + template <class ELFT> std::vector<FdeData> getFdeData() const;<br>
><br>
> private:<br>
> uint64_t Size = 0;<br>
> - template <class RelTy><br>
> +<br>
> + template <class ELFT, class RelTy><br>
> void addSectionAux(EhInputSection *S, llvm::ArrayRef<RelTy> Rels);<br>
><br>
> - template <class RelTy><br>
> + template <class ELFT, class RelTy><br>
> CieRecord *addCie(EhSectionPiece &Piece, ArrayRef<RelTy> Rels);<br>
><br>
> - template <class RelTy><br>
> + template <class ELFT, class RelTy><br>
> bool isFdeLive(EhSectionPiece &Piece, ArrayRef<RelTy> Rels);<br>
><br>
> uint64_t getFdePc(uint8_t *Buf, size_t Off, uint8_t Enc) const;<br>
> @@ -814,6 +811,7 @@ struct InX {<br>
> static BssSection *Bss;<br>
> static BssSection *BssRelRo;<br>
> static BuildIdSection *BuildId;<br>
> + static EhFrameSection *EhFrame;<br>
> static SyntheticSection *Dynamic;<br>
> static StringTableSection *DynStrTab;<br>
> static SymbolTableBaseSection *DynSymTab;<br>
> @@ -835,7 +833,6 @@ struct InX {<br>
><br>
> template <class ELFT> struct In {<br>
> static EhFrameHeader<ELFT> *EhFrameHdr;<br>
> - static EhFrameSection<ELFT> *EhFrame;<br>
> static RelocationSection<ELFT> *RelaDyn;<br>
> static RelocationSection<ELFT> *RelaPlt;<br>
> static RelocationSection<ELFT> *RelaIplt;<br>
> @@ -845,7 +842,6 @@ template <class ELFT> struct In {<br>
> };<br>
><br>
> template <class ELFT> EhFrameHeader<ELFT> *In<ELFT>::EhFrameHdr;<br>
> -template <class ELFT> EhFrameSection<ELFT> *In<ELFT>::EhFrame;<br>
> template <class ELFT> RelocationSection<ELFT> *In<ELFT>::RelaDyn;<br>
> template <class ELFT> RelocationSection<ELFT> *In<ELFT>::RelaPlt;<br>
> template <class ELFT> RelocationSection<ELFT> *In<ELFT>::RelaIplt;<br>
><br>
> Modified: lld/trunk/ELF/Writer.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/ELF/Writer.cpp?rev=316731&r1=316730&r2=316731&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/lld/trunk/ELF/Writer.cpp<wbr>?rev=316731&r1=316730&r2=31673<wbr>1&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- lld/trunk/ELF/Writer.cpp (original)<br>
> +++ lld/trunk/ELF/Writer.cpp Thu Oct 26 20:13:39 2017<br>
> @@ -139,7 +139,7 @@ template <class ELFT> static void combin<br>
> if (!ES || !ES->Live)<br>
> continue;<br>
><br>
> - In<ELFT>::EhFrame->addSection(<wbr>ES);<br>
> + InX::EhFrame->addSection<ELFT><wbr>(ES);<br>
> S = nullptr;<br>
> }<br>
><br>
> @@ -376,8 +376,8 @@ template <class ELFT> void Writer<ELFT>:<br>
> In<ELFT>::EhFrameHdr = make<EhFrameHeader<ELFT>>();<br>
> Add(In<ELFT>::EhFrameHdr);<br>
> }<br>
> - In<ELFT>::EhFrame = make<EhFrameSection<ELFT>>();<br>
> - Add(In<ELFT>::EhFrame);<br>
> + InX::EhFrame = make<EhFrameSection>();<br>
> + Add(InX::EhFrame);<br>
> }<br>
><br>
> if (InX::SymTab)<br>
> @@ -853,7 +853,7 @@ void Writer<ELFT>::forEachRelSec(st<wbr>d::fu<br>
> for (InputSectionBase *IS : InputSections)<br>
> if (IS->Live && isa<InputSection>(IS) && (IS->Flags & SHF_ALLOC))<br>
> Fn(*IS);<br>
> - for (EhInputSection *ES : In<ELFT>::EhFrame->Sections)<br>
> + for (EhInputSection *ES : InX::EhFrame->Sections)<br>
> Fn(*ES);<br>
> }<br>
><br>
> @@ -1252,7 +1252,7 @@ template <class ELFT> void Writer<ELFT>:<br>
> // This responsible for splitting up .eh_frame section into<br>
> // pieces. The relocation scan uses those pieces, so this has to be<br>
> // earlier.<br>
> - applySynthetic({In<ELFT>::EhFr<wbr>ame},<br>
> + applySynthetic({InX::EhFrame},<br>
> [](SyntheticSection *SS) { SS->finalizeContents(); });<br>
><br>
> for (Symbol *S : Symtab->getSymbols())<br>
> @@ -1532,8 +1532,8 @@ template <class ELFT> std::vector<PhdrEn<br>
> Ret.push_back(RelRo);<br>
><br>
> // PT_GNU_EH_FRAME is a special section pointing on .eh_frame_hdr.<br>
> - if (!In<ELFT>::EhFrame->empty() && In<ELFT>::EhFrameHdr &&<br>
> - In<ELFT>::EhFrame->getParent() && In<ELFT>::EhFrameHdr->getParen<wbr>t())<br>
> + if (!InX::EhFrame->empty() && In<ELFT>::EhFrameHdr &&<br>
> + InX::EhFrame->getParent() && In<ELFT>::EhFrameHdr->getParen<wbr>t())<br>
> AddHdr(PT_GNU_EH_FRAME, In<ELFT>::EhFrameHdr->getParen<wbr>t()->getPhdrFlags())<br>
> ->add(In<ELFT>::EhFrameHdr->ge<wbr>tParent());<br>
><br>
><br>
><br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div>