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

Tue Sep 7 10:34:13 PDT 2021

jrtc27 added inline comments.

================
Comment at: llvm/include/llvm/BinaryFormat/ELFRelocs/RISCV.def:61
 ELF_RELOC(R_RISCV_IRELATIVE,         58)
+/* Overlay extension - awaiting real numbers as part of RISC-V psABI */
+ELF_RELOC(R_RISCV_OVL_HI20,          220)
----------------
Obviously can't be merged like this

================
Comment at: llvm/include/llvm/MC/MCExpr.h:356-357
+    // A SymbolRef VK is needed in order to parse e.g. `.word foo at overlay_plt`
+    VK_RISCV_OVLPLT,         // overlay_plt
+    VK_RISCV_OVL,            // overlay
+
----------------
Why do you need both? Functions get PLTs, objects get data pointers, it should already be implied by the type.

If this is going to be a generic concept then this shouldn't be RISC-V specific.

================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1969

+  if (Option == "warnreservedreg") {
+    getTargetStreamer().emitDirectiveOptionWarnReservedReg();
----------------
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.

================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:170
   // multiple "bitmask" flags.
-  MO_DIRECT_FLAG_MASK = 15
+  MO_DIRECT_FLAG_MASK = 31
 };
----------------
This change doesn't cover any of your additions but does cover ones that are already there, so makes no sense

================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp:125
+          MCSymbolRefExpr::VK_RISCV_OVLPLT)
+      return ELF::R_RISCV_OVLPLT32;
+    if (Expr->getKind() == MCExpr::SymbolRef &&
----------------
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.

================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:157
+unsigned
+RISCVMCCodeEmitter::expandFunctionCallOverlay(const MCInst &MI, raw_ostream &OS,
+                                              SmallVectorImpl<MCFixup> &Fixups,
----------------
Does this need to be expanded so late? It's not like CALL where there's a single relocation for the entire sequence. Can it be expanded earlier like things like LLA?

================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:176
+      // Extract the call symbol that should be targetted by this call.
+      assert(MI.getOperand(0).isExpr() && "Something has gone wrong?");
+      const RISCVMCExpr *RCallExpr = cast<RISCVMCExpr>(MI.getOperand(0).getExpr());
----------------
craig.topper wrote:
> This assert isn't really needed. The getExpr() call on the next line will assert.
Not helpful

================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:2818-2826
+  // Overlay calls are special. They are unaffected by -fpic or the code
+  // model, so they're handled first here.
+  if (SpecialMOLo || SpecialMOHi) {
+    assert(SpecialMOLo && SpecialMOHi && "MOFlags only on one half?");
+    SDValue AddrLo = getTargetNode(N, DL, Ty, DAG, SpecialMOLo);
+    SDValue AddrHi = getTargetNode(N, DL, Ty, DAG, SpecialMOHi);
+    SDValue LoadHi = SDValue(DAG.getMachineNode(RISCV::LUI, DL, Ty, AddrHi), 0);
----------------
This probably warrants just being a separate function

================
Comment at: llvm/test/CodeGen/RISCV/ovlcall-fncall.ll:1
+; RUN: llc -mtriple riscv32-unknown-elf -filetype=obj < %s\
+; RUN: | llvm-objdump -d -r -M no-aliases -M numeric - | FileCheck %s
----------------
Tests need to match the style of existing ones. That includes update_llc_test_checks.py.

================
Comment at: llvm/test/CodeGen/RISCV/ovlcall-fncall.ll:49
+  %0 = load i32 (...)*, i32 (...)** @indirect_foo, align 4
+  %callee.knr.cast = bitcast i32 (...)* %0 to i32 ()*
+  %call = call i32 %callee.knr.cast()
----------------
This smells a lot like you generated this from C with dodgy code (`void foo()` and `void foo(void)` are not the same in C).

================
Comment at: llvm/test/CodeGen/RISCV/ovlcall-fncall.ll:124
+
+attributes #0 = { noinline nounwind optnone "target-features"="+reserve-x28,+reserve-x29,+reserve-x30,+reserve-x31,-relax" }
+attributes #1 = { noinline nounwind optnone "target-features"="+reserve-x28,+reserve-x29,+reserve-x30,+reserve-x31,-relax" "overlay-call" }
----------------
optnone makes no sense for these kinds of tests.

Target features belong on the llc call, not per-function (this does matter for the output, as it governs what ends up in the ELF attributes).

================
Comment at: llvm/test/MC/RISCV/ovlcall-pseudos.s:10
+# CHECK-NEXT: jalr  ra, 0(t6)
+ovlcall a_symbol at overlay
+
----------------
This seems redundant, ovlcall should imply overlay

================
Comment at: llvm/test/MC/RISCV/ovlcall-pseudos.s:17
+# CHECK-NEXT: jalr  ra, 0(t6)
+ovlcall b_symbol at resident
+
----------------
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.

================
Comment at: llvm/test/Transforms/Inline/RISCV/riscv-overlaycall.ll:19
+
+attributes #0 = { "overlay-call" }
----------------
Inline it

================
Comment at: llvm/test/Transforms/Inline/RISCV/riscv-overlaycallee.ll:19
+
+attributes #0 = { "overlay-call" }
----------------
Inline it

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109371