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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv64.h:1
+//===--- ELF_riscv64.h - JIT link functions for ELF/x86-64 ---*- C++ -*-===//
+//
----------------
Should be ELF_riscv.h


================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv64.h:1
+//===--- ELF_riscv64.h - JIT link functions for ELF/x86-64 ---*- C++ -*-===//
+//
----------------
jrtc27 wrote:
> Should be ELF_riscv.h
Also copy-paste error with that x86-64


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


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:266
+                            const object::ELFFile<ELFT> &Obj)
+      : ELFLinkGraphBuilder<ELFT>(Obj, Triple("riscv64-unknown-linux"),
+                                  FileName, getELFRISCVRelocationKindName) {}
----------------
This still hard-codes Linux, as well as that it's 64-bit now the class is just ELFLinkGraphBuilder_riscv.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:282-283
+  auto &ELFObjFile = cast<object::ELFObjectFile<object::ELF64LE>>(**ELFObj);
+  assert(ELFObjFile.getArch() == Triple::riscv64 &&
+         "RISCV 32 is not supportted in JITLink");
+  return ELFLinkGraphBuilder_riscv<object::ELF64LE>((*ELFObj)->getFileName(),
----------------
1. Asserting it's 64-bit _after_ you've cast it is unlikely to end well
2. Just add the handful of lines to have an ELF32LE case?


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:289-292
+void link_ELF_riscv64(std::unique_ptr<LinkGraph> G,
+                      std::unique_ptr<JITLinkContext> Ctx) {
+  PassConfiguration Config;
+  const Triple &TT = G->getTargetTriple();
----------------
This can just be link_ELF_riscv, nothing here cares about RV32 vs RV64


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105429