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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

Perhaps revert this patch since it may be good to fix formatting nits in the initial RISC-V JIT patch.



================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/riscv.h:28
+  ///
+  /// Fixup expression:
+  ///   Fixup <= Target + Addend : uint32
----------------
I know this copied from the existing x86_64.h but the documentation style is quite verbose. The blank line and `Fixup expression:` are redundant.

It is better to use the regular ELF relocation format where symbols like `S`, `A` are used to describe operands.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:60
+
+static uint32_t extractBits(uint64_t Num, unsigned High, unsigned Low) {
+  return (Num & ((1ULL << (High + 1)) - 1)) >> Low;
----------------
lld/ELF/RISCV.cpp is not a good example. High before Low is weird.

I may use low&size to represent bits [low, low+size).


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv.cpp:134
+      uint32_t RawInstr = *(little32_t *)FixupPtr;
+
+      *(little32_t *)FixupPtr = (RawInstr & 0x1FFF07F) | Imm31_25 | Imm11_7;
----------------
previous relocations don't add a blank line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105429