[PATCH] D123496: Add Stackmap support for RISC-V

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:139
+  // Emit nops.
+  for (unsigned i = 0; i < NumNOPBytes; i += 4)
+    EmitToStreamer(OutStreamer, MCInstBuilder(RISCV::ADDI)
----------------
You repeat this code a number of times.  Please pull out a helper function EmitNops(N), and maybe one for EmitNop as well.  

You might be able to reuse AP.emitNops here, but that returns a fixed number of nops, not a fixed number of nop bytes.  Looking at the x86 parallel, I think that's what you want here.  


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:156
+  PatchPointOpers Opers(&MI);
+
+  int64_t CallTarget = Opers.getCallTarget().getImm();
----------------
CallTarget can be a global symbol as well as a constant.

I'd suggest splitting this patch into support for patchpoint/statapoint without targets (i.e. the nop patchable forms), and then adding the target resolution separately.  The call resolution adds a decent amount of complexity, and is worth reviewing in detail on it's own.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:277
 
 void RISCVAsmPrinter::emitInstruction(const MachineInstr *MI) {
   // Do any auto-generated pseudo lowerings.
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123496