[PATCH] D129848: [RISCV] Add Stackmap/Statepoint/Patchpoint support with targets

[Sacha Coppey via Phabricator via llvm-commits]

Zeavee added inline comments.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp:262
+        ((Loc.getPointer() != nullptr) &&
+         (strcmp(Loc.getPointer(), "stackmap") == 0)))
       return MCELFStreamer::emitValueImpl(Value, Size, Loc);
----------------
jrtc27 wrote:
> This seems extremely dubious
I understand, and it is. I tried to look at where exactly the OffsetExpr is reduced to 0 in the fixups, but I did not find it. I will search more.


================
Comment at: llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp:143
+
+      RISCVMatInt::InstSeq Seq =
+          RISCVMatInt::generateInstSeq(CallTarget, STI->getFeatureBits());
----------------
jrtc27 wrote:
> This seems like a lot of duplication of what already exists for expanding PseudoLI
I will check if emitting a PseudoLI works correctly.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:998
 
+  unsigned NumBytes = 0;
+
----------------
jrtc27 wrote:
> Why initialise to 0? And why have this large scope when it's only for one case?
I will put it in a smaller scope.


================
Comment at: llvm/test/CodeGen/RISCV/rv64-patchpoint.ll:43
+
+
+  %resolveCall2 = inttoptr i64 244837814094590 to i8*
----------------
jrtc27 wrote:
> Why the blank lines?
They seem to have been added by update_llc_test_checks.py, as I do not have them in previous versions.


================
Comment at: llvm/test/CodeGen/RISCV/rv64-patchpoint.ll:102-113
+  %tmp80 = add i64 %tmp79, -16
+  %tmp81 = inttoptr i64 %tmp80 to i64*
+  %tmp82 = load i64, i64* %tmp81, align 8
+  tail call void (i64, i32, ...) @llvm.experimental.stackmap(i64 14, i32 8, i64 %arg, i64 %tmp2, i64 %tmp10, i64 %tmp82)
+  tail call void (i64, i32, i8*, i32, ...) @llvm.experimental.patchpoint.void(i64 15, i32 32, i8* null, i32 3, i64 %arg, i64 %tmp10, i64 %tmp82)
+  %tmp83 = load i64, i64* %tmp33, align 8
+  %tmp84 = add i64 %tmp83, -24
----------------
jrtc27 wrote:
> What's the actual point of this test? It seems like a bit of a mess of random arithmetic, pointer casting and memory operations, with a return of 10 for unknown reasons. Are the instructions themselves interesting? If not, make them simpler.
To be honest, I used tests from AArch64, because I thought they would better than test I would create myself. I do not think the instructions themselves are interesting, so I will modify it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129848