[PATCH] D105429: [JITLink][RISCV] Initial Support RISCV64 in JITLink

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF.cpp:82-83
   switch (G->getTargetTriple().getArch()) {
+  case Triple::riscv64:
+  case Triple::riscv32:
+    link_ELF_riscv(std::move(G), std::move(Ctx));
----------------
Sort


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:57
+  return make_error<JITLinkError>(
+      "No HI20 relocation type be found for LO12 relocation type");
+}
----------------
Should say PCREL in there otherwise it can be confused with the absolute HI20/LO12 relocations that are more normal (ie don't have this indirection)


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:90
+      int64_t Value = E.getTarget().getAddress() + E.getAddend();
+      int32_t Lo = Value - ((Value + 0x800) & 0xFFFFF000);
+      uint32_t RawInstr = *(little32_t *)FixupPtr;
----------------
This is still using the old inefficient "backwards" way of doing it, just mask Value with 0xFFF and be done with it.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:99
+      int32_t Hi = (Value + 0x800) & 0xFFFFF000;
+      int32_t Lo = Value - Hi;
+      uint32_t RawInstrAuipc = *(little32_t *)FixupPtr;
----------------
Ditto


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:118-119
+        return RelHI20.takeError();
+      int64_t Value = (**RelHI20).getTarget().getAddress() +
+                      (**RelHI20).getAddend() - E.getTarget().getAddress();
+      int64_t Lo = Value & 0xFFF;
----------------
makes more sense to me, though `Expected<const Edge &>` would remove one level of indirection for callers so you can just do `RelHI20->getTarget()` etc.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105429