[PATCH] D125036: [LLD][RISCV] Alignment relaxation

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Minor style comments.  Not really my area, so I can't really comment on how this fits within the existing structure.



================
Comment at: lld/ELF/Arch/RISCV.cpp:498
+        uint64_t alignment = PowerOf2Ceil(rel.addend + 2);
+        uint64_t nopBytes = alignTo(pc, alignment) - pc;
+        if (nopBytes > (uint64_t)rel.addend) {
----------------
the name nopBytes is slightly ambiguous here.  Maybe requiredNopBytes?  To distinguish it on first glance from the number available.  


================
Comment at: lld/ELF/Arch/RISCV.cpp:519
+
+        // Rewrite the NOP sequence
+        uint8_t *buf = is->mutableData().data() + rel.offset;
----------------
Extend this comment to indicate why this is required - i.e. split a 4 byte into a 2-byte remainder case + not wanting to assume the compiler emitted nops at all.  


================
Comment at: lld/ELF/Arch/RISCV.cpp:526
+            buf += 4;
+          } else if (rvc && nopBytes == 2) {
+            write16le(buf, 0x0001); // c.nop
----------------
The rvc part of this check is redundant with the checks above.  Maybe drop it from the else if clause, and turn it into an assert within the block?


================
Comment at: lld/ELF/InputSection.cpp:179
+    uint64_t removedBytes = 0;
+    const auto *r = ranges.begin(), *rend = ranges.end();
+    for (auto *dr : symbols) {
----------------
This code seems to assume that ranges appear in order of increasing start address.  I believe that is true in the code, but could easily be asserted more heavily to prevent future subtle mistakes.  


================
Comment at: lld/ELF/InputSection.cpp:183
+        removedBytes += r->size;
+
+      dr->value -= removedBytes;
----------------
You should probably assert that no range contains the symbol address.  


================
Comment at: lld/ELF/InputSection.h:154
 
+  mutable bool copiedData = false;
+  mutable ArrayRef<uint8_t> rawData;
----------------
Having variables defined between functions looks odd to my eye, but I see this already exists in the class.  Maybe do a pre-nfc to group all the data together, and then add your new ones there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125036