[PATCH] D149526: [JITLink][RISCV] Implement linker relaxation

[Job Noorman via Phabricator via llvm-commits]

jobnoorman added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/riscv.h:205-211
+
+  /// Marks another relocation at the same offset as eligible for linker
+  /// relaxation.
+  R_RISCV_RELAX,
+
+  /// Encodes alignment requirement of the instruction at Fixup + Addend
+  R_RISCV_ALIGN,
----------------
Hahnfeld wrote:
> Please keep this sorted based on the enum values (see comment at the top)
These edge kinds will be removed in the next version of this patch.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:523
+
+        assert(PrevEdge && "R_RISCV_RELAX without previous edge");
+        BlockAux.RelaxEdges.push_back(PrevEdge);
----------------
Hahnfeld wrote:
> Should this error more gracefully than `assert`ing, also in Release builds?
It should indeed.

I'm going to make a small modification to the implementation based on a comment in another review:

>>! In D149524#4308261, @lhames wrote:
> For `R_RISCV_RELAX` we should add relaxable edge kinds to the RISCV edge kinds enum (along the same lines as the x86-64 relaxable edges, and we should take this opportunity to rename the RISCV edges to bring them in line with the other architectures). When the ELF/RISCV LinkGraphBuilder sees an `R_RISCV_RELAX` relocation it should choose the relaxable variant for the corresponding edge.

This will also move the detection of this kind of error to a different location so I will implement your suggestion there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149526