[PATCH] D100835: [WIP][LLD][RISCV] Linker Relaxation

[Chih-Mao Chen via Phabricator via llvm-commits]

PkmX added inline comments.


================
Comment at: lld/ELF/Arch/RISCV.cpp:637-638
+      auto &rels = is->relocations;
+      if (rels.empty())
+        continue;
+
----------------
jrtc27 wrote:
> This condition seems unnecessary
It's necessary as `processRelaxations` needs to access two iterators which would be invalid if the container is empty. I will move this into `processRelaxations` which is where it should be in the next patch.


================
Comment at: lld/ELF/Arch/RISCV.cpp:733-734
+  if (config->relax && !config->relocatable) {
+    // Try to iteratively perform relaxation for up to 5 times.
+    for (int i = 0; i < 5; ++i) {
+      if (!relax())
----------------
jrtc27 wrote:
> Why a limit of 5? It is guaranteed to converge, I don't see why there needs to be a limit, and it'll probably take just 1 or 2 almost all of the time.
Will be removed in the next patch.


================
Comment at: lld/ELF/Arch/RISCV.cpp:740
+
+  relaxAlign();
+}
----------------
jrtc27 wrote:
> Not for relocatable
Will do in the next patch.


================
Comment at: lld/ELF/InputSection.cpp:195-196
+    auto it = ranges.begin();
+    for (auto *dr : symbols) {
+      for (auto e = ranges.end(); it != e && it->first < dr->value; ++it)
+        removedBytes += it->second;
----------------
jrtc27 wrote:
> This looks quadratic
This isn't quadratic.

We are really looping over the symbols in this section while simultaneously accumulating the number of bytes deleted. Since symbols and delete ranges are sorted by offset, if we know X bytes are deleted before offset A, and B > A, we only have to add up delete ranges between A and B to know the total number of bytes deleted before B. In essence every symbol and delete range is only visited once in this loop.


================
Comment at: lld/ELF/InputSection.h:173
+
+  using DeleteRanges = std::map<uint64_t, uint64_t>;
+  void deleteRanges(const DeleteRanges &deleteRanges);
----------------
jrtc27 wrote:
> std::map seems like a high-overhead way to store what is just a sorted list of intervals.
I agree. `std::vector<std::pair<uint64_t, uint64_t>>` should be sufficient although it needs to be sorted for `deleteRanges` as the vector may not sorted with two-pass handling for `PCREL_HI20`.


================
Comment at: lld/ELF/InputSection.h:260
   mutable ArrayRef<uint8_t> rawData;
+  uint8_t *mutData;
 
----------------
jrtc27 wrote:
> Why do we need a whole new pointer when rawData is also being updated?
Will change it to use a `copiedData` bool as in your patch.


================
Comment at: lld/ELF/Relocations.cpp:135
+template <RelExpr... Exprs> struct RelExprMaskTester {
+  static inline bool test(RelExpr expr) { return false; }
 };
----------------
jrtc27 wrote:
> (a) this is unrelated to the diff
> (b) this completely removes the whole point of oneof
As noted in the summary adding more `RelExpr` makes it unable to fit in a 64-bit mask, so this is temporary change to make this patch work for now.

There were already 64 `RelExpr`s before this patch so there is no room for growing and I don't think it is workable in the long run. The only solution I can think of is split them into target specific `RelExpr` with some common exprs like `R_ABS` being shared, so as long as the target doesn't define more than ~20 `RelExpr` it should be fine. However this probably should belong to another patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100835/new/

https://reviews.llvm.org/D100835