[PATCH] D68065: Propeller: LLD Support for Basic Block Sections

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