[PATCH] D123496: [RISCV] Add Stackmap/Statepoint/Patchpoint support without targets

[Sacha Coppey via Phabricator via llvm-commits]

Zeavee marked 6 inline comments as done.
Zeavee added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:160
+  if (CallTarget) {
+    assert((CallTarget & 0xFFFFFFFFFFFF) == CallTarget &&
+           "High 16 bits of call target should be zero.");
----------------
luismarques wrote:
> Nit: put some `'` digit separators to make it easier to read the constant. E.g. `0x0xFFFF'FFFF'FFFF`. (Or use some higher-level function from `MathExtras.h` if there is one).
Applied in next patch.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:166-193
+    EmitToStreamer(OutStreamer, MCInstBuilder(RISCV::ORI)
+                                    .addReg(RISCV::X1)
+                                    .addReg(RISCV::X0)
+                                    .addImm((CallTarget >> 36) & 0xFFF));
+    EmitToStreamer(OutStreamer, MCInstBuilder(RISCV::SLLI)
+                                    .addReg(RISCV::X1)
+                                    .addReg(RISCV::X1)
----------------
luismarques wrote:
> Can't we do better than this for materializing the address? If the number of instructions doesn't have to be fixed you can even use `generateInstSeq` from `RISCVMatInt.h` to generate a more optimal instruction sequence.
`generateInstSeq` is indeed better, thank you!


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:277
 
 void RISCVAsmPrinter::emitInstruction(const MachineInstr *MI) {
   // Do any auto-generated pseudo lowerings.
----------------
reames wrote:
> Seeing this on the AsmPrinter is suprising.  I'd have expected this code to be in RISCVMCInstLower.cpp.  Is there something I'm missing here?
It seems to be the case for most architectures. It is true however that the lowering of those instructions should be in `RISCVMCInstLower.cpp` as it would be more consistent. Should I move it there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123496