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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

Why write your own deleteRanges? I took great care in writing mine to ensure there were no hidden quadratic complexities, but your simpler version has introduced them. Just take mine, unless there is a problem with it nobody's mentioned. Same goes for mutableData being a less efficient version of mine.

I also don't like making this overly target-specific. We should take the time to get the interface right, not just punt on it and shove it all into target-specific code.



================
Comment at: lld/ELF/Arch/RISCV.cpp:617
+
+    Relocation *rel = std::prev(r);
+    if (r->offset != rel->offset)
----------------
Can we not just keep two iterators around? That feels nicer to me than careful +1s everywhere (e.g. it's not immediately obvious this won't crash on the first element until you look at the start point of the loop).


================
Comment at: lld/ELF/Arch/RISCV.cpp:637-638
+      auto &rels = is->relocations;
+      if (rels.empty())
+        continue;
+
----------------
This condition seems unnecessary


================
Comment at: lld/ELF/Arch/RISCV.cpp:686
+      for (auto &rel : is->relocations) {
+        if (rel.type == R_RISCV_ALIGN && rel.addend > 0) {
+          uint64_t pc = is->getVA(rel.offset) - bytesDeleted;
----------------
When would the addend ever be 0? That seems like invalid input.


================
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())
----------------
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.


================
Comment at: lld/ELF/Arch/RISCV.cpp:740
+
+  relaxAlign();
+}
----------------
Not for relocatable


================
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;
----------------
This looks quadratic


================
Comment at: lld/ELF/InputSection.cpp:216-217
+    auto it = ranges.begin();
+    for (auto *dr : symbols) {
+      for (auto e = ranges.end(); it != e && it->first < endOff(dr); ++it)
+        removedBytes += it->second;
----------------
As does this


================
Comment at: lld/ELF/InputSection.cpp:227-228
+  auto it = ranges.begin();
+  for (auto &rel : relocations) {
+    for (auto e = ranges.end(); it != e && it->first < rel.offset; ++it)
+      removedBytes += it->second;
----------------
And this


================
Comment at: lld/ELF/InputSection.h:162
 
+  MutableArrayRef<uint8_t> mutableData() {
+    if (mutData != data().data()) {
----------------
Make the relevant fields mutable and mark this const, like data()


================
Comment at: lld/ELF/InputSection.h:173
+
+  using DeleteRanges = std::map<uint64_t, uint64_t>;
+  void deleteRanges(const DeleteRanges &deleteRanges);
----------------
std::map seems like a high-overhead way to store what is just a sorted list of intervals.


================
Comment at: lld/ELF/InputSection.h:260
   mutable ArrayRef<uint8_t> rawData;
+  uint8_t *mutData;
 
----------------
Why do we need a whole new pointer when rawData is also being updated?


================
Comment at: lld/ELF/Relocations.cpp:135
+template <RelExpr... Exprs> struct RelExprMaskTester {
+  static inline bool test(RelExpr expr) { return false; }
 };
----------------
(a) this is unrelated to the diff
(b) this completely removes the whole point of oneof


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100835