[PATCH] D109371: [RISCV][RFC] Add LLVM support for RISC-V overlay system

[Edward Jones via Phabricator via llvm-commits]

edward-jones added a comment.

Thanks for all of the prompt feedback, I'll incorporate the suggested changes and update the patch



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1969
 
+  if (Option == "warnreservedreg") {
+    getTargetStreamer().emitDirectiveOptionWarnReservedReg();
----------------
jrtc27 wrote:
> This needs to go through riscv-asm-manual/riscv-toolchain-conventions approval.
> 
> Also, when would you ever have this on? It seems like if you're writing assembly you're likely to want to use reserved registers? CodeGen certainly assumes it can, because it knows what they're for, so this would break, say, existing GCC or Clang output. And currently the notion of reserved registers is purely CodeGen and up, not at the MC layer.
Yeah, there's approval needed for the ELF reloc numbers, and the assembler changes too. This patch will be linked from pull requests to those documentation repos.

Good question about `warnreservedreg`. The motivation here was to help with porting existing RISC-V code which might use registers reserved for the overlay engine, so this option could be selectively disabled if code was being written for the overlay engine itself.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:125
+          MCSymbolRefExpr::VK_RISCV_OVLPLT)
+      return ELF::R_RISCV_OVLPLT32;
+    if (Expr->getKind() == MCExpr::SymbolRef &&
----------------
jrtc27 wrote:
> Even if overlays don't make much sense outside of 32-bit, it would be nicer if the scheme were in theory generalisable to 64-bit, like how R_RISCV_RELATIVE, for example, is for whatever your XLEN is.
Noted, this is definitely unnecessarily restrictive.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVFixupKinds.h:108
 
+  fixup_riscv_ovl_hi20,
+  fixup_riscv_ovl_lo12_i,
----------------
I realize these need comments about their usage too


================
Comment at: llvm/test/MC/RISCV/ovlcall-pseudos.s:17
+# CHECK-NEXT: jalr  ra, 0(t6)
+ovlcall b_symbol at resident
+
----------------
jrtc27 wrote:
> All these implicit register uses really scare me. If you're writing assembly I feel you should just be writing the actual thing, to be honest. This is a lot of hidden magic, and it doesn't seem at all necessary, people are going to be mostly writing in C.
Yes, originally there was a motivation to make it simpler to write these calls, but in hindsight I don't think there's a good justification for using a pseudo instruction instead of just being explicit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109371