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

Thu Jul 8 09:41:20 PDT 2021

jrtc27 added a comment.

As a general point I don't like that this is dubbed a "riscv64" JITLink implementation. Almost all the code is the same between RV32 and RV64, the only difference is whether you have ELFCLASS32 or ELFCLASS64. Renaming ELFJITLinker_riscv64 to ELFJITLinker_riscv, plus renaming ELFLinkGraphBuilder_riscv64 to ELFLinkGraphBuilder_riscv and templating it on the ELFType, should make it "just work" for RV32. At least name the files and public interfaces riscv not riscv64 so adding RV32 support in future won't create avoidable churn.

================
Comment at: llvm/include/llvm/ExecutionEngine/JITLink/ELF_riscv64.h:28
+  R_RISCV_HI20,
+  R_RISCV_LO12_I,
+  R_RISCV_CALL,
----------------
I-type but not S-type is a bit odd

================
Comment at: llvm/lib/ExecutionEngine/JITLink/CMakeLists.txt:20
   ELF_x86_64.cpp
+  ELF_riscv64.cpp

----------------
Keep sorted

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF.cpp:70
     return createLinkGraphFromELFObject_x86_64(ObjectBuffer);
+  case ELF::EM_RISCV:
+    return createLinkGraphFromELFObject_riscv64(ObjectBuffer);
----------------
Sort

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF.cpp:85
     return;
+  case Triple::riscv64:
+    link_ELF_riscv64(std::move(G), std::move(Ctx));
----------------
Sort

================
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
+};
+
----------------
lhames wrote:
> Is this needed? You should be able to use the `isDwarfSection` method in the base class.
Can this be moved somewhere so it's shared with x86?

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp:40-42
+  assert(
+      E.getKind() == R_RISCV_PCREL_LO12_I &&
+      "Not a R_RISCV_PCREL_LO12_I relocation that want find the Hi relocation");
----------------
Dodgy grammar; this is more natural, and also shorter (though you'll need to change the message to `R_RISCV_PCREL_LO12_[IS]` once you support R_RISCV_PCREL_LO12_S).

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp:45-52
+  auto Bound = std::lower_bound(B.edges().begin(), B.edges().end(), E,
+                                [](const Edge &lhs, const Edge &rhs) {
+                                  return lhs.getOffset() < rhs.getOffset();
+                                });
+
+  for (auto it = Bound; it != B.edges().begin(); --it)
+    if (it->getKind() == R_RISCV_PCREL_HI20)
----------------
This is strange. You find the first edge that's >= the edge, then walk backwards. That is wrong for two reasons:

1. You only want to find an R_RISCV_PCREL_HI20 at precisely the right location, not keep walking back until you find one, so should use std::equal_range and iterate over the returned range to look at the relocations _only_ at that specific location.
2. This is totally the wrong way to do R_RISCV_PCREL_LO12_[IS]; it's _indirect_, so you need to find the relocation by looking at `E,getTarget().getAddress() + E.getAddend()`, otherwise you'll incorrectly handle things like:

```
1: auipc a0, %pcrel_hi(foo)
2: auipc a1, %pcrel_hi(bar)
   addi a0, a0, %pcrel_lo(1b)
   addi a1, a1, %pcrel_lo(2b)
```

```
1: auipc a0, %pcrel_hi(foo)
   ld t0, %pcrel_lo(1b)(a0)
2: auipc a1, %pcrel_hi(bar)
   ld t1, %pcrel_lo(2b)(a1)
   add t0, t0, t1
   sd t0, %pcrel_lo(1b)(a0)
```
and
```
   j 2f
1: addi a0, a0, %pcrel_lo(2f)
   j 3f
2: auipc a0, a0, %pcrel_hi(foo)
   j 1b
3:
```

all of which are legal, if silly in the case of that last one, though currently LLVM happens to not generate those kinds of things (GCC will generate things like that with `-mexplicit-relocs` - see https://godbolt.org/z/jPn3M5bM8 - though that's not a default option, and is discouraged, because it's overly aggressive and will do unsound optimisations in certain cases).

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp:88
+      *(little32_t *)FixupPtr =
+          (RawInstr & 0xFFFFF) | (static_cast<uint32_t>(Lo & 0xFFF) << 20);
+      break;
----------------
As discussed in the other review, `Lo & 0xFFF` is just `Value & 0xFFF`.

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp:99
+      *(little32_t *)(FixupPtr + 4) =
+          RawInstrJalr | (static_cast<uint32_t>(Lo) << 20);
+      break;
----------------
Lo needs masking like LO12_I, but also this is then just `Value & 0xFFF`.

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp:116
+      *(little32_t *)FixupPtr =
+          (RawInstr & 0xFFFFF) | (static_cast<uint32_t>(Lo & 0xFFF) << 20);
+      break;
----------------
Ditto

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp:146
+
+    return make_error<JITLinkError>("Unsupported x86-64 relocation:" +
+                                    formatv("{0:d}", Type));
----------------
Copy-paste error

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp:152-158
+
+    // TODO a partern is forming of iterate some sections but only give me
+    // ones I am interested, I should abstract that concept some where
+    for (auto &SecRef : Sections) {
+      if (SecRef.sh_type != ELF::SHT_RELA && SecRef.sh_type != ELF::SHT_REL)
+        continue;
+      // TODO can the elf obj file do this for me?
----------------
These comments are copied from x86_64 but I don't think they make sense

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp:186
+
+      auto JITSection = G->findSectionByName(*UpdateSectionName);
+      if (!JITSection)
----------------
Please follow these clang-tidy suggestions, marking pointers as pointers with auto is the LLVM style

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp:195
+      if (!Relocations)
+        Relocations.takeError();
+
----------------
Missing `return` at the front

================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_riscv64.cpp:222
+        }
+        uint64_t Addend = Rela.r_addend;
+        JITTargetAddress FixupAddress =
----------------
Addends are signed.

================
Comment at: llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s:13
+        .globl  main
+        .p2align  4, 0x90 
+        .type   main, at function
----------------
Why is everything 16-byte aligned? And why are you padding with x86 NOPs?

================
Comment at: llvm/test/ExecutionEngine/JITLink/RISCV/ELF_pc_indirect.s:27
+        auipc a0, %pcrel_hi(named_data)
+        ld  a0, %pcrel_lo(test_pcrel32)(a0)
+
----------------
Make this lw from a .4byte then the same test could be used for RV32 too

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105429