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

[Jonas Hahnfeld via Phabricator via llvm-commits]

Hahnfeld 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,
----------------
Please keep this sorted based on the enum values (see comment at the top)


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


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:589-592
+  } else if (Config.HasRVC && Config.IsRV32 && isInt<12>(Displace) && RD == 1) {
+    NewEdgeKind = R_RISCV_RVC_JUMP;
+    Aux.Writes.push_back(0x2001); // c.jal
+    Remove = 6;
----------------
I'm probably missing something here: Why is this only possible in RV32?


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:823-826
+    case ELF::R_RISCV_RELAX:
+      return EdgeKind_riscv::R_RISCV_RELAX;
+    case ELF::R_RISCV_ALIGN:
+      return EdgeKind_riscv::R_RISCV_ALIGN;
----------------
same here, please keep this sorted


================
Comment at: llvm/lib/ExecutionEngine/JITLink/riscv.cpp:81-84
+  case R_RISCV_RELAX:
+    return "R_RISCV_RELAX";
+  case R_RISCV_ALIGN:
+    return "R_RISCV_ALIGN";
----------------
same


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149526