[PATCH] D51089: [LLD] [COFF] [RFC] Add support for creating range extension thunks for ARM

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: COFF/Chunks.cpp:51-52
+void SectionChunk::finalizeContents() {
+  if (!Symbols.empty())
+    return;
+  for (const coff_relocation &Rel : Relocs)
----------------
Can this happen?


================
Comment at: COFF/Chunks.cpp:53-54
+    return;
+  for (const coff_relocation &Rel : Relocs)
+    Symbols.push_back(File->getSymbol(Rel.SymbolTableIndex));
+}
----------------
Since this vector can be very large, it is perhaps better to call `reserve()`.


================
Comment at: COFF/Chunks.cpp:54
+  for (const coff_relocation &Rel : Relocs)
+    Symbols.push_back(File->getSymbol(Rel.SymbolTableIndex));
+}
----------------
mstorsjo wrote:
> Does anyone have an opinion on the mechanism of overriding what symbol an individual reloc points to? Here I provide a full vector of symbols (which can't be initialized directly but after all symbols actually exist) - an alternative would be e.g. a DenseMap to only provide the individual symbols that are overridden. Or something else?
I think this vector should be fine because this vector will be used very heavily and a vector lookup is extremely fast.


================
Comment at: COFF/Chunks.cpp:424
+  size_t Counter = 0;
   for (const coff_relocation &Rel : Relocs) {
     uint8_t Ty = getBaserelType(Rel);
----------------
It is more straightforward to write this loop in as a plain old `for` loop instead of a range-based for loop with `Counter`.


================
Comment at: COFF/Chunks.h:227
   ArrayRef<coff_relocation> Relocs;
+  std::vector<Symbol *> Symbols;
 
----------------
Maybe something like `RelocTargets` is better? `Symbols` looks like it represents symbol table contents.

Please add a comment to explain why we want to cache relocation targets in this table (i.e. we need to modify relocation targets when a relocation is redirected to other symbol due to thunk insertion.)


================
Comment at: COFF/ICF.cpp:150
     }
+    // This doesn't use the potentially remapped Symbol, but at this point,
+    // we shouldn't have done any remapping yet (and using the original
----------------
I'd think this comment is not easy to understand if you don't know about this comment is in the context of ARM thunk support. Maybe we can just omit it?

Does ICF run after ARM thunk creation?


================
Comment at: COFF/ICF.cpp:178
   auto Eq = [&](const coff_relocation &R1, const coff_relocation &R2) {
+    // This doesn't use the potentially remapped Symbol, but at this point,
+    // we shouldn't have done any remapping yet (and using the original
----------------
Ditto.


================
Comment at: COFF/Writer.cpp:339
+  int64_t Diff = S - P - 4;
+  assert(Config->Machine == ARMNT);
+  switch (RelType) {
----------------
nit: move this assert at the beginning of this function.


================
Comment at: COFF/Writer.cpp:360
+  // the existing thunks?
+  for (Defined *Sym : TargetThunks) {
+    if (isInRange(Type, Sym->getRVA(), P))
----------------
I don't know if your above comment is true, but if the one this loop is looking for is likely at the end of the vector, I'd search in the reverse order. I.e.

  for (Defined *Sym : llvm::reverse(TargetThunks))


================
Comment at: COFF/Writer.cpp:362
+    if (isInRange(Type, Sym->getRVA(), P))
+      return std::pair<Defined *, bool>(Sym, false);
+  }
----------------
Can this just be `return {Sym, false}`?


================
Comment at: COFF/Writer.cpp:363
+      return std::pair<Defined *, bool>(Sym, false);
+  }
+  Chunk *C = make<RangeExtensionThunkARM>(Target);
----------------
nit: omit {}


================
Comment at: COFF/Writer.cpp:367
+  Defined *D = dyn_cast_or_null<Defined>(Symtab->addSynthetic(
+      Saver.save("__thunk_" + Target->getName() + "_" + Twine(ThunkCounter++)),
+      C));
----------------
I don't think you need to pass this `ThunkCounter` around; I'd define this as a static local variable in this function.


================
Comment at: COFF/Writer.cpp:370
+  if (!D)
+    fatal("Thunk collision with other symbol?");
+  TargetThunks.push_back(D);
----------------
Error messages should start with a lowercase letter.

Shouldn't this be an `assert`? If this can be triggered by a valid user input, this error message should contain more info as to what is the problem.


================
Comment at: COFF/Writer.cpp:372
+  TargetThunks.push_back(D);
+  return std::pair<Defined *, bool>(D, true);
+}
----------------
Maybe `return {D, true}`


================
Comment at: COFF/Writer.cpp:376
+bool OutputSection::createThunks(size_t &ThunkCounter) {
+  std::map<uint64_t, std::vector<Defined *>> ExistingThunks;
+  bool Changed = false;
----------------
std::map is an ordered map and usually much slower than DenseMap, so please use DenseMap.


================
Comment at: COFF/Writer.cpp:385
+    size_t ThunkInsertionSpot = I + 1;
+    // Try to get a good enough estimate of where new thunks will be placed.
+    // Offset this by the size of the new thunks added so far, to make the
----------------
nit: please insert a blank line before a comment.


================
Comment at: COFF/Writer.cpp:406
+        if (WasNew && Thunk) {
+          // TODO: .insert() in a std::vector will move all later elements
+          // forward - is this a concern?
----------------
Nesting is too deep. Please consider splitting to multiple functions.


================
Comment at: COFF/Writer.cpp:429
+    if (ThunkPass >= 10)
+      fatal("Adding thunks hasn't converged after " + Twine(ThunkPass) +
+            " passes");
----------------
Adding -> adding


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D51089