[PATCH] D127581: [ELF] Relax R_RISCV_ALIGN

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I'll leave the RISCV details to the experts. General approach looks good to me.



================
Comment at: lld/ELF/InputSection.h:102
 
+struct SymbolAnchor {
+  uint64_t offset;
----------------
It looks like the definitions are only used in RISCV.cpp as only a pointer is used in the union below. Could these be forward declared here?

I could be missing some use though.


================
Comment at: lld/ELF/InputSection.h:224
+  union {
+    // These are modifiers to jump instructions that are necessary when basic
+    // block sections are enabled.  Basic block sections creates opportunities
----------------
update comment for relaxAux? I assume that the union is because we don't have relaxation and basic block sections simultaneously?


================
Comment at: lld/ELF/Target.h:92
 
+  virtual bool relaxOnce(int pass) const { return false; }
+
----------------
Will be worth a comment like `needsThunk`  to describe what this does, just in case another architecture chooses to do RiscV like relaxations.


================
Comment at: lld/ELF/Writer.cpp:1637
+                                       : target->relaxOnce(tc.pass);
+    ++tc.pass;
 
----------------
Although more source changes. Would it be cleaner to have the passes variable here, and pass it into createThunks as a parameter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127581