[PATCH] D145687: [BOLT] Add minimal RISC-V 64-bit support

[Job Noorman via Phabricator via llvm-commits]

jobnoorman marked 4 inline comments as done.
jobnoorman added inline comments.


================
Comment at: bolt/include/bolt/Core/BinaryContext.h:730
 
+  bool isRISCV() const { return TheTriple->getArch() == llvm::Triple::riscv64; }
+
----------------
craig.topper wrote:
> Should this be isRISCV64()?
This would technically be more correct at the moment. However, I don't think anything in this patch is actually RV64 specific. The only reason we cannot currently handle RV32 is that BOLT rejects ELF32 files. Once that's fixed, I believe this patch could also handle RV32. For this reason, I would propose to keep the current naming. Would you agree?


================
Comment at: bolt/lib/Core/Relocation.cpp:505
+    return extractUImmRISCV(Contents);
+  case ELF::R_RISCV_PCREL_LO12_I:
+    return extractIImmRISCV(Contents);
----------------
craig.topper wrote:
> What about PCREL_LO12_S?
I will leave the implementation of PCREL_LO12_S to a future patch.


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:1461
+  // Skip the first special entry since no relocation points to it.
+  uint64_t InstrOffset = 32;
+
----------------
yota9 wrote:
> Is it first PLT entry skipped? I don't think it is good idea to set constant size here in that case. Maybe it is possible to make more universal like it is done in aarch64? there are no PLT entry size assumptions too as I remember
It's not really the first entry that's skipped, but a special entry at the beginning of the PLT section. The [riscv psabi](https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#program-linkage-table) defines the layout of the PLT section:
- First 32 bytes: a special entry that calls `_dl_runtime_resolve` to resolve GOT entries. Initially, all GOT entries point here.
- Then, 16 bytes per "normal" PLT entry.

So since the sizes of these entries are standardized, I think it's OK to keep the as constants here. Would you agree?


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:2624
   if (!ReferencedSection && !ForceRelocation) {
+    // Check for ABS symbols. Used e.g., for __global_pointer$ on RISC-V
+    auto RelSymbol = Rel.getSymbol();
----------------
yota9 wrote:
> We would need to check this code. Please provide more info about example of such symbols, sections it is contained/referenced in and segments.
> Is there any chance that the problem might be solved by this commit https://github.com/llvm/llvm-project/commit/0776fc32b15dc76c6b43c41fc4471552833265de ? 
The symbol looks like this:

```
   Num:    Value          Size Type    Bind   Vis      Ndx Name
    86: 0000000000002800     0 NOTYPE  GLOBAL DEFAULT  ABS __global_pointer$
```

It is used in RISC-V to initialize the `gp` register using a pair of `R_RISCV_PCREL_HI20`/`R_RISCV_PCREL_LO12` relocations.

Since `ABS` symbols don't reference/belong to any section, I believe that commit doesn't solve the problem?


================
Comment at: bolt/lib/Rewrite/RewriteInstance.cpp:3255
     Function.postProcessCFG();
+    BC->MIB->postProcessFunction(Function);
 
----------------
yota9 wrote:
> Probably better to move it to arch-specific pass like it is currently done in ADRRelaxationPass or VeneerElimination for example
I moved this to a separate pass but I left the implementation in a `MCPlusBuilder` method. I did this because trying to implement the logic directly in a pass is quite difficult because passes don't have access to a target's MC interface. I would have needed to add methods like `isCallAuipc`/`isJalr` to `MCPlusBuilder` which feels wrong since it's supposed to be a generic interface.

Would it be a good idea to allow targets to register their own passes (e.g., `MCPlusBuilder::registerPasses()`) so that they can be implemented in a target-specific subfolder and have access to their MC interface? This might allow us to remove some target-specific methods from `MCPlusBuilder`. If you agree with this idea, I could give it a shot.


================
Comment at: bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp:39
+
+  bool isNoop(const MCInst &Inst) const override {
+    return Inst.getOpcode() == RISCV::ADDI &&
----------------
craig.topper wrote:
> Does this need to support c.nop?
It does, added!


================
Comment at: bolt/lib/Target/RISCV/RISCVMCPlusBuilder.cpp:253
+    case ELF::R_RISCV_PCREL_LO12_I:
+    case ELF::R_RISCV_PCREL_LO12_S:
+      return RISCVMCExpr::create(Expr, RISCVMCExpr::VK_RISCV_PCREL_LO, Ctx);
----------------
craig.topper wrote:
> This references R_RISCV_PCREL_LO12_S but nothing else in this patch does.
Removed.


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

https://reviews.llvm.org/D145687