[PATCH] D68065: Propeller: LLD Support for Basic Block Sections
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 26 00:23:26 PDT 2019
MaskRay added a subscriber: PkmX.
MaskRay added a comment.
The shrinking section nature may be similar to R_RISCV_RELAX as used by RISC-V. The RISC-V port of binutils ld.bfd has a similar routine to change section sizes and update symbol values. @PkmX and I haven't implemented the relaxation in the lld RISC-V work. This patch inspires me to explore more on the possibility to implement section shrinking and make it general. I don't like the binutils implementation because it is quadratic about several variables.
================
Comment at: lld/ELF/Arch/X86_64.cpp:511
+
+ unsigned RIndex = getRelocationWithOffset(IS, (IS.getSize() - 1));
+
----------------
Delete ()
================
Comment at: lld/ELF/Arch/X86_64.cpp:513
+
+ if (RIndex == IS.relocations.size()){
+ if (IS.getSize() < SizeOfDirectNearJmpInsn){
----------------
Delete superfluous `{}` and blank lines.
================
Comment at: lld/ELF/Arch/X86_64.cpp:564
+
+ if (BytesGrown) {
+ IS.push_back(BytesGrown);
----------------
Delete `if (BytesGrown)`
================
Comment at: lld/ELF/Arch/X86_64.cpp:852
+ case J_JE_32:
+ if (Size == 4) {*(Loc-1) = 0x0f; *Loc = 0x84; } else *Loc = 0x74;
+ break;
----------------
loc[-1]
================
Comment at: lld/ELF/Arch/X86_64.cpp:882
+ default:
+ error(getErrorLocation(Loc) + "unrecognized jump reloc " + Twine(Type));
+ }
----------------
llvm_unreachable if this is unreachable
================
Comment at: lld/ELF/OutputSections.cpp:257
+ return;
+ if (SFiller.at(remaining-1).size() != remaining)
+ fatal("Failed padding with special filler.");
----------------
`at` -> `operator[]`
at has a bound checking overhead.
================
Comment at: lld/ELF/OutputSections.cpp:258
+ if (SFiller.at(remaining-1).size() != remaining)
+ fatal("Failed padding with special filler.");
+ memcpy(Buf + I, SFiller.at(remaining-1).data(), remaining);
----------------
`failed ...` and no full stop
================
Comment at: lld/ELF/SyntheticSections.h:589
ArrayRef<SymbolTableEntry> getSymbols() const { return symbols; }
+ std::map<uint64_t, std::pair<uint64_t, uint32_t>> EndsMap;
----------------
unordered_map if the ordered property is not needed.
================
Comment at: lld/ELF/Target.h:96
+
+ virtual unsigned growJmpInsn(InputSection &IS, InputFile *File,
+ uint32_t MaxAlignment) const
----------------
If growJmpInsn is X86 specific. Can it be moved to Arch/X86_64.cpp?
================
Comment at: lld/ELF/Writer.cpp:49
+using std::chrono::system_clock;
+using std::chrono::duration;
----------------
Delete these.
================
Comment at: lld/ELF/Writer.cpp:559
// finalizeSections does that.
+ //auto startFinalizeSectionTime = system_clock::now();
finalizeSections();
----------------
Delete unneeded comments.
================
Comment at: lld/ELF/Writer.cpp:644
+ StringRef name = sym.getName();
+
+ if (name.empty() && sym.type == llvm::ELF::STT_NOTYPE &&
----------------
Delete the blank line.
================
Comment at: lld/ELF/Writer.cpp:646
+ if (name.empty() && sym.type == llvm::ELF::STT_NOTYPE &&
+ sym.binding == llvm::ELF::STB_LOCAL) {
+ return false;
----------------
Delete surrounding `{}`
================
Comment at: lld/ELF/Writer.cpp:652
+ lld::propeller::SymbolEntry::isBBSymbol(name)) {
+ if (config->propellerKeepNamedSymbols ||
+ propeller::PropLeg.shouldKeepBBSymbol(name))
----------------
return condition
rather than
if condition
return true
else
return false
================
Comment at: lld/ELF/Writer.cpp:705
ObjFile<ELFT> *f = cast<ObjFile<ELFT>>(file);
+ std::list<Symbol *> localNonBBSymbols;
+ std::list<Symbol *> localBBSymbols;
----------------
Use vectors and move them outside the loop.
================
Comment at: lld/ELF/Writer.cpp:732
+ // Add BB symbols to SymTab first.
+ for (auto *S : localBBSymbols) {
+ in.symTab->addSymbol(S);
----------------
Delete {}
auto -> Symbol
================
Comment at: lld/ELF/Writer.cpp:1653
+
+ const auto *Sec = Def->section;
+ if (!Sec)
----------------
auto->SectionBase
================
Comment at: lld/ELF/Writer.cpp:1663
+
+ if (Def->value > NewSize) {
+ LLVM_DEBUG(llvm::dbgs() << "Moving symbol " << Sym->getName() <<
----------------
This is a dangerous operation. Are you arbitrarily changing st_value/st_size? In what conditions can this be triggered?
================
Comment at: lld/ELF/Writer.cpp:1685
+// relaxation pass does that.
+template <class ELFT> void Writer<ELFT>::optimizeBasicBlockJumps() {
+ if (!config->optimizeBBJumps || !ELFT::Is64Bits)
----------------
optimizeBasicBlockJumps should be placed in finalizeAddressDependentContent to accommodate thunks and linker script symbol changes.
================
Comment at: lld/ELF/Writer.cpp:1686
+template <class ELFT> void Writer<ELFT>::optimizeBasicBlockJumps() {
+ if (!config->optimizeBBJumps || !ELFT::Is64Bits)
+ return;
----------------
Why !ELFT::Is64Bits
================
Comment at: lld/ELF/Writer.cpp:1693
+ std::vector<InputSection *> Sections = getInputSections(OS);
+ std::vector<bool> Result(Sections.size());
+ // Delete all fall through jump instructions.
----------------
vector<bool> is thread hostile because it is represented as a bit vector.
See SyntheticSections.cpp:createSymbols for an example how to parallel correctly.
================
Comment at: lld/ELF/Writer.cpp:1708
+
+ auto MaxIt = config->shrinkJumpsAggressively ? Sections.end() :
+ std::max_element(Sections.begin(), Sections.end(),
----------------
MaxIt is not needed.
```
MaxAlign = 0;
if !config->shrinkJumpsAggressively && !sections.empty()
MaxAlign = max_element(...)->alignment;
```
================
Comment at: lld/ELF/Writer.cpp:1715
+
+ // Shrink jump Instructions optimistically
+ std::vector<unsigned> Shrunk(Sections.size(), 0);
----------------
More comments on how sections are shrunk.
================
Comment at: lld/ELF/Writer.cpp:1717
+ std::vector<unsigned> Shrunk(Sections.size(), 0);
+ std::vector<bool> Changed(Sections.size(), 0);
+ bool AnyChanged = false;
----------------
vector<bool> is thread hostile.
================
Comment at: lld/ELF/Writer.cpp:1724
+ unsigned BytesShrunk = target->shrinkJmpInsn(IS, IS.getFile<ELFT>(), MaxAlign);
+ Changed[I] = (BytesShrunk > 0);
+ Shrunk[I] += BytesShrunk;
----------------
Supefluous `()`
================
Comment at: lld/ELF/Writer.cpp:1732
+ Num += std::count_if(Shrunk.begin(), Shrunk.end(),
+ [] (int e) { return e > 4; });
+ if (Num > 0)
----------------
Magic 4 here seems very x86 specific.
================
Comment at: lld/ELF/Writer.cpp:1747
+ InputSection &IS = *Sections[I];
+ unsigned BytesGrown = target->growJmpInsn(IS, IS.getFile<ELFT>(), MaxAlign);
+ Changed[I] = (BytesGrown > 0);
----------------
ruiu wrote:
> Is growing different from undoing? I'm not quite sure why you can't simply revert all changes we made to a section if we have to grow it.
Another related question: why does **shrink**JumpsAggressively call growJmpInsn?
================
Comment at: lld/ELF/Writer.cpp:1769
+ for (InputSection * IS: Sections)
+ IS->trim();
+ }
----------------
Try not adding another field InputSectionBase::Trimmed. Instead, moving it here.
================
Comment at: lld/ELF/Writer.cpp:2118
+ // sections.
+ //auto startOptBBJumpTime = system_clock::now();
+ optimizeBasicBlockJumps();
----------------
Delete unrelated comments.
Repository:
rLLD LLVM Linker
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68065/new/
https://reviews.llvm.org/D68065
More information about the llvm-commits
mailing list