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

Thu Jul 8 02:53:18 PDT 2021

lhames accepted this revision.
lhames added a comment.
This revision is now accepted and ready to land.

Hi Stephen,

LGTM. This is great stuff -- Thank you for working on this!

================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv64.h:23-33
+namespace ELF_riscv64_Edges {
+enum ELFRISCVRelocationKind : Edge::Kind {
+  R_RISCV_32 = Edge::FirstRelocation,
+  R_RISCV_64,
+  R_RISCV_HI20,
+  R_RISCV_LO12_I,
+  R_RISCV_CALL,
----------------
Could you please move these edge kinds into their own file (e.g. 'RISCV.h') following the convention established in `llvm/include/llvm/ExecutionEngine/JITLink/x86_64.h`?

I don't mind if the initial edge set very closely mirrors the ELF relocation set, but the idea is to try to capture the generic fixups that the architecture requires. This simplifies the implementation of new linker backends for the architecture, and allows Plugins to remain as format-agnostic as possible.

Sadly ELF_x86_64 hasn't been moved to the generic kinds in x86_64.h yet, so it's not an ideal example here. The MachO_x86_64 backend is probably a better example.

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp:24-33
+const char *const DwarfSectionNames[] = {
+#define HANDLE_DWARF_SECTION(ENUM_NAME, ELF_NAME, CMDLINE_NAME, OPTION)        \
+  ELF_NAME,
+#include "llvm/BinaryFormat/Dwarf.def"
+#undef HANDLE_DWARF_SECTION
+};
+
----------------
Is this needed? You should be able to use the `isDwarfSection` method in the base class.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105429