Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RISCV] Implement Statepoint and Patchpoint lowering to call instructions #77337

Merged
merged 6 commits into from
Apr 11, 2024

Conversation

Zeavee
Copy link
Contributor

@Zeavee Zeavee commented Jan 8, 2024

This patch adds stackmap support for RISC-V with call targets.

Based on patch from https://reviews.llvm.org/D129848.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 8, 2024

@llvm/pr-subscribers-backend-risc-v

Author: Sacha Coppey (Zeavee)

Changes

This patch adds stackmap support for RISC-V with call targets.

Based on patch from https://reviews.llvm.org/D129848.


Full diff: https://github.com/llvm/llvm-project/pull/77337.diff

7 Files Affected:

  • (modified) llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp (+4-27)
  • (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp (+41)
  • (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.h (+7)
  • (modified) llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp (+57)
  • (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+11)
  • (modified) llvm/lib/Target/RISCV/RISCVInstrInfo.cpp (+7-2)
  • (modified) llvm/test/CodeGen/RISCV/rv64-patchpoint.ll (+45-1)
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index d616aaeddf4114..494a21b49e7e38 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -2999,34 +2999,11 @@ void RISCVAsmParser::emitToStreamer(MCStreamer &S, const MCInst &Inst) {
 
 void RISCVAsmParser::emitLoadImm(MCRegister DestReg, int64_t Value,
                                  MCStreamer &Out) {
-  RISCVMatInt::InstSeq Seq = RISCVMatInt::generateInstSeq(Value, getSTI());
-
-  MCRegister SrcReg = RISCV::X0;
-  for (const RISCVMatInt::Inst &Inst : Seq) {
-    switch (Inst.getOpndKind()) {
-    case RISCVMatInt::Imm:
-      emitToStreamer(Out,
-                     MCInstBuilder(Inst.getOpcode()).addReg(DestReg).addImm(Inst.getImm()));
-      break;
-    case RISCVMatInt::RegX0:
-      emitToStreamer(
-          Out, MCInstBuilder(Inst.getOpcode()).addReg(DestReg).addReg(SrcReg).addReg(
-                   RISCV::X0));
-      break;
-    case RISCVMatInt::RegReg:
-      emitToStreamer(
-          Out, MCInstBuilder(Inst.getOpcode()).addReg(DestReg).addReg(SrcReg).addReg(
-                   SrcReg));
-      break;
-    case RISCVMatInt::RegImm:
-      emitToStreamer(
-          Out, MCInstBuilder(Inst.getOpcode()).addReg(DestReg).addReg(SrcReg).addImm(
-                   Inst.getImm()));
-      break;
-    }
+  SmallVector<MCInst, 8> Seq =
+      RISCVMatInt::generateMCInstSeq(Value, getSTI(), DestReg);
 
-    // Only the first instruction has X0 as its source.
-    SrcReg = DestReg;
+  for (MCInst &Inst : Seq) {
+    emitToStreamer(Out, Inst);
   }
 }
 
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp
index 4358a5b878e631..d873d03d4b11c1 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.cpp
@@ -9,6 +9,7 @@
 #include "RISCVMatInt.h"
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
 #include "llvm/ADT/APInt.h"
+#include "llvm/MC/MCInstBuilder.h"
 #include "llvm/Support/MathExtras.h"
 using namespace llvm;
 
@@ -469,6 +470,46 @@ InstSeq generateTwoRegInstSeq(int64_t Val, const MCSubtargetInfo &STI,
   return RISCVMatInt::InstSeq();
 }
 
+SmallVector<MCInst, 8>
+generateMCInstSeq(int64_t Val, const MCSubtargetInfo &STI, MCRegister DestReg) {
+  RISCVMatInt::InstSeq Seq = RISCVMatInt::generateInstSeq(Val, STI);
+
+  SmallVector<MCInst, 8> instructions;
+
+  MCRegister SrcReg = RISCV::X0;
+  for (RISCVMatInt::Inst &Inst : Seq) {
+    switch (Inst.getOpndKind()) {
+    case RISCVMatInt::Imm:
+      instructions.push_back(MCInstBuilder(Inst.getOpcode())
+                                 .addReg(DestReg)
+                                 .addImm(Inst.getImm()));
+      break;
+    case RISCVMatInt::RegX0:
+      instructions.push_back(MCInstBuilder(Inst.getOpcode())
+                                 .addReg(DestReg)
+                                 .addReg(SrcReg)
+                                 .addReg(RISCV::X0));
+      break;
+    case RISCVMatInt::RegReg:
+      instructions.push_back(MCInstBuilder(Inst.getOpcode())
+                                 .addReg(DestReg)
+                                 .addReg(SrcReg)
+                                 .addReg(SrcReg));
+      break;
+    case RISCVMatInt::RegImm:
+      instructions.push_back(MCInstBuilder(Inst.getOpcode())
+                                 .addReg(DestReg)
+                                 .addReg(SrcReg)
+                                 .addImm(Inst.getImm()));
+      break;
+    }
+
+    // Only the first instruction has X0 as its source.
+    SrcReg = DestReg;
+  }
+  return instructions;
+}
+
 int getIntMatCost(const APInt &Val, unsigned Size, const MCSubtargetInfo &STI,
                   bool CompressionCost) {
   bool IsRV64 = STI.hasFeature(RISCV::Feature64Bit);
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.h
index 780f685463f300..edd4cdeba8a25a 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.h
@@ -10,7 +10,10 @@
 #define LLVM_LIB_TARGET_RISCV_MCTARGETDESC_MATINT_H
 
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/TargetParser/SubtargetFeature.h"
 #include <cstdint>
 
 namespace llvm {
@@ -56,6 +59,10 @@ InstSeq generateInstSeq(int64_t Val, const MCSubtargetInfo &STI);
 InstSeq generateTwoRegInstSeq(int64_t Val, const MCSubtargetInfo &STI,
                               unsigned &ShiftAmt, unsigned &AddOpc);
 
+// Helper to generate the generateInstSeq instruction sequence using MCInsts
+SmallVector<MCInst, 8>
+generateMCInstSeq(int64_t Val, const MCSubtargetInfo &STI, MCRegister DestReg);
+
 // Helper to estimate the number of instructions required to materialise the
 // given immediate value into a register. This estimate does not account for
 // `Val` possibly fitting into an immediate, and so may over-estimate.
diff --git a/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp b/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
index f2bd5118fc0717..c8975a9cdc4110 100644
--- a/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
+++ b/llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp
@@ -14,6 +14,7 @@
 #include "MCTargetDesc/RISCVBaseInfo.h"
 #include "MCTargetDesc/RISCVInstPrinter.h"
 #include "MCTargetDesc/RISCVMCExpr.h"
+#include "MCTargetDesc/RISCVMatInt.h"
 #include "MCTargetDesc/RISCVTargetStreamer.h"
 #include "RISCV.h"
 #include "RISCVMachineFunctionInfo.h"
@@ -152,8 +153,35 @@ void RISCVAsmPrinter::LowerPATCHPOINT(MCStreamer &OutStreamer, StackMaps &SM,
 
   PatchPointOpers Opers(&MI);
 
+  const MachineOperand &CalleeMO = Opers.getCallTarget();
   unsigned EncodedBytes = 0;
 
+  if (CalleeMO.isImm()) {
+    uint64_t CallTarget = CalleeMO.getImm();
+    if (CallTarget) {
+      assert((CallTarget & 0xFFFF'FFFF'FFFF) == CallTarget &&
+             "High 16 bits of call target should be zero.");
+      // Materialize the jump address:
+      SmallVector<MCInst, 8> Seq =
+          RISCVMatInt::generateMCInstSeq(CallTarget, *STI, RISCV::X1);
+      for (MCInst &Inst : Seq) {
+        EmitToStreamer(OutStreamer, Inst);
+      }
+      EncodedBytes += Seq.size() * 4;
+      EmitToStreamer(OutStreamer, MCInstBuilder(RISCV::JALR)
+                                      .addReg(RISCV::X1)
+                                      .addReg(RISCV::X1)
+                                      .addImm(0));
+      EncodedBytes += 4;
+    }
+  } else if (CalleeMO.isGlobal()) {
+    MCOperand CallTargetMCOp;
+    lowerOperand(CalleeMO, CallTargetMCOp);
+    EmitToStreamer(OutStreamer,
+                   MCInstBuilder(RISCV::PseudoCALL).addOperand(CallTargetMCOp));
+    EncodedBytes += 8;
+  }
+
   // Emit padding.
   unsigned NumBytes = Opers.getNumPatchBytes();
   assert(NumBytes >= EncodedBytes &&
@@ -172,6 +200,35 @@ void RISCVAsmPrinter::LowerSTATEPOINT(MCStreamer &OutStreamer, StackMaps &SM,
     assert(PatchBytes % NOPBytes == 0 &&
            "Invalid number of NOP bytes requested!");
     emitNops(PatchBytes / NOPBytes);
+  } else {
+    // Lower call target and choose correct opcode
+    const MachineOperand &CallTarget = SOpers.getCallTarget();
+    MCOperand CallTargetMCOp;
+    switch (CallTarget.getType()) {
+    case MachineOperand::MO_GlobalAddress:
+    case MachineOperand::MO_ExternalSymbol:
+      lowerOperand(CallTarget, CallTargetMCOp);
+      EmitToStreamer(
+          OutStreamer,
+          MCInstBuilder(RISCV::PseudoCALL).addOperand(CallTargetMCOp));
+      break;
+    case MachineOperand::MO_Immediate:
+      CallTargetMCOp = MCOperand::createImm(CallTarget.getImm());
+      EmitToStreamer(OutStreamer, MCInstBuilder(RISCV::JAL)
+                                      .addReg(RISCV::X1)
+                                      .addOperand(CallTargetMCOp));
+      break;
+    case MachineOperand::MO_Register:
+      CallTargetMCOp = MCOperand::createReg(CallTarget.getReg());
+      EmitToStreamer(OutStreamer, MCInstBuilder(RISCV::JALR)
+                                      .addReg(RISCV::X1)
+                                      .addOperand(CallTargetMCOp)
+                                      .addImm(0));
+      break;
+    default:
+      llvm_unreachable("Unsupported operand type in statepoint call target");
+      break;
+    }
   }
 
   auto &Ctx = OutStreamer.getContext();
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 79c16cf4c4c361..9efa15af2d4535 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -16969,6 +16969,17 @@ RISCVTargetLowering::EmitInstrWithCustomInserter(MachineInstr &MI,
   case RISCV::PseudoFROUND_D_IN32X:
     return emitFROUND(MI, BB, Subtarget);
   case TargetOpcode::STATEPOINT:
+    // STATEPOINT is a pseudo instruction which has no implicit defs/uses
+    // while jal call instruction (where statepoint will be lowered at the end)
+    // has implicit def. This def is early-clobber as it will be set at
+    // the moment of the call and earlier than any use is read.
+    // Add this implicit dead def here as a workaround.
+    MI.addOperand(*MI.getMF(),
+                  MachineOperand::CreateReg(
+                      RISCV::X1, /*isDef*/ true,
+                      /*isImp*/ true, /*isKill*/ false, /*isDead*/ true,
+                      /*isUndef*/ false, /*isEarlyClobber*/ true));
+    [[fallthrough]];
   case TargetOpcode::STACKMAP:
   case TargetOpcode::PATCHPOINT:
     if (!Subtarget.is64Bit())
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 7f6a045a7d042f..37cd378cb0eaed 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -1524,9 +1524,14 @@ unsigned RISCVInstrInfo::getInstSizeInBytes(const MachineInstr &MI) const {
   case TargetOpcode::PATCHPOINT:
     // The size of the patchpoint intrinsic is the number of bytes requested
     return PatchPointOpers(&MI).getNumPatchBytes();
-  case TargetOpcode::STATEPOINT:
+  case TargetOpcode::STATEPOINT: {
     // The size of the statepoint intrinsic is the number of bytes requested
-    return StatepointOpers(&MI).getNumPatchBytes();
+    unsigned NumBytes = StatepointOpers(&MI).getNumPatchBytes();
+    // A statepoint is at least a PseudoCALL
+    if (NumBytes < 8)
+      NumBytes = 8;
+    return NumBytes;
+  }
   default:
     return get(Opcode).getSize();
   }
diff --git a/llvm/test/CodeGen/RISCV/rv64-patchpoint.ll b/llvm/test/CodeGen/RISCV/rv64-patchpoint.ll
index d2a3bccfef7bb0..10f80162af0cc1 100644
--- a/llvm/test/CodeGen/RISCV/rv64-patchpoint.ll
+++ b/llvm/test/CodeGen/RISCV/rv64-patchpoint.ll
@@ -1,12 +1,56 @@
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=riscv64 -debug-entry-values -enable-misched=0 < %s | FileCheck %s
 
+; Trivial patchpoint codegen
+;
+define i64 @trivial_patchpoint_codegen(i64 %p1, i64 %p2, i64 %p3, i64 %p4) {
+; CHECK-LABEL: trivial_patchpoint_codegen:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    addi sp, sp, -16
+; CHECK-NEXT:    .cfi_def_cfa_offset 16
+; CHECK-NEXT:    sd s0, 8(sp) # 8-byte Folded Spill
+; CHECK-NEXT:    sd s1, 0(sp) # 8-byte Folded Spill
+; CHECK-NEXT:    .cfi_offset s0, -8
+; CHECK-NEXT:    .cfi_offset s1, -16
+; CHECK-NEXT:    mv s0, a0
+; CHECK-NEXT:  .Ltmp0:
+; CHECK-NEXT:    lui ra, 3563
+; CHECK-NEXT:    addiw ra, ra, -577
+; CHECK-NEXT:    slli ra, ra, 12
+; CHECK-NEXT:    addi ra, ra, -259
+; CHECK-NEXT:    slli ra, ra, 12
+; CHECK-NEXT:    addi ra, ra, -1282
+; CHECK-NEXT:    jalr ra
+; CHECK-NEXT:    mv s1, a0
+; CHECK-NEXT:    mv a0, s0
+; CHECK-NEXT:    mv a1, s1
+; CHECK-NEXT:  .Ltmp1:
+; CHECK-NEXT:    lui ra, 3563
+; CHECK-NEXT:    addiw ra, ra, -577
+; CHECK-NEXT:    slli ra, ra, 12
+; CHECK-NEXT:    addi ra, ra, -259
+; CHECK-NEXT:    slli ra, ra, 12
+; CHECK-NEXT:    addi ra, ra, -1281
+; CHECK-NEXT:    jalr ra
+; CHECK-NEXT:    mv a0, s1
+; CHECK-NEXT:    ld s0, 8(sp) # 8-byte Folded Reload
+; CHECK-NEXT:    ld s1, 0(sp) # 8-byte Folded Reload
+; CHECK-NEXT:    addi sp, sp, 16
+; CHECK-NEXT:    ret
+entry:
+  %resolveCall2 = inttoptr i64 244837814094590 to i8*
+  %result = tail call i64 (i64, i32, i8*, i32, ...) @llvm.experimental.patchpoint.i64(i64 2, i32 28, i8* %resolveCall2, i32 4, i64 %p1, i64 %p2, i64 %p3, i64 %p4)
+  %resolveCall3 = inttoptr i64 244837814094591 to i8*
+  tail call void (i64, i32, i8*, i32, ...) @llvm.experimental.patchpoint.void(i64 3, i32 28, i8* %resolveCall3, i32 2, i64 %p1, i64 %result)
+  ret i64 %result
+}
+
 ; Test small patchpoints that don't emit calls.
 define void @small_patchpoint_codegen(i64 %p1, i64 %p2, i64 %p3, i64 %p4) {
 ; CHECK-LABEL: small_patchpoint_codegen:
 ; CHECK:       # %bb.0: # %entry
 ; CHECK-NEXT:    .cfi_def_cfa_offset 0
-; CHECK-NEXT:  .Ltmp0:
+; CHECK-NEXT:  .Ltmp2:
 ; CHECK-NEXT:    nop
 ; CHECK-NEXT:    nop
 ; CHECK-NEXT:    nop

@Zeavee
Copy link
Contributor Author

Zeavee commented Jan 22, 2024

@preames @asb Sorry for the ping, I don't have the rights to add reviewers, can you please have a look at the PR? It is the same as the previous patch (see description).

@Zeavee
Copy link
Contributor Author

Zeavee commented Feb 12, 2024

Ping

@Zeavee
Copy link
Contributor Author

Zeavee commented Mar 6, 2024

Ping

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with these intrinsics, just some style suggestions.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.h Outdated Show resolved Hide resolved
llvm/lib/Target/RISCV/MCTargetDesc/RISCVMatInt.h Outdated Show resolved Hide resolved
Copy link

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link

✅ With the latest revision this PR passed the Python code formatter.

@Zeavee Zeavee changed the title [RISCV] Add Stackmap/Statepoint/Patchpoint support with targets [RISCV] Implement Statepoint and Patchpoint lowering to call instructions Apr 2, 2024
Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have more comments. Is this patch already used in real project like GraalVM?

@Zeavee
Copy link
Contributor Author

Zeavee commented Apr 4, 2024

I don't have more comments. Is this patch already used in real project like GraalVM?

Yes, it has been used in GraalVM for the past year and there has not been issues with this patch.

for (MCInst &Inst : Seq) {
MCInst CInst;
bool compressed = RISCVRVC::compress(CInst, Inst, *STI);
EmitToStreamer(OutStreamer, Inst);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
EmitToStreamer(OutStreamer, Inst);
EmitToStreamer(OutStreamer, Compressed ? CInst : Inst));

Copy link
Contributor Author

@Zeavee Zeavee Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I used Inst instead of CInst was that EmitToStreamer increments RISCVNumInstrsCompressed if the instruction is compressed. In this case, should I use CInst and increment RISCVNumInstrsCompressed here if needed?

Copy link
Contributor

@wangpc-pp wangpc-pp Apr 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created a PR (#88120) so that we don't have to call RISCVRVC::compress twice.
And after that, you can write:

bool Compressed = EmitToStreamer(OutStreamer, Inst);
EncodedBytes += Compressed ? 2 : 4;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Committed, please rebase. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thank you!

RISCVMatInt::generateMCInstSeq(CallTarget, *STI, RISCV::X1, Seq);
for (MCInst &Inst : Seq) {
MCInst CInst;
bool compressed = RISCVRVC::compress(CInst, Inst, *STI);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compressed -> Compressed

.addReg(RISCV::X1)
.addImm(0);
bool compressed = RISCVRVC::compress(CInst, Inst, *STI);
EmitToStreamer(OutStreamer, Inst);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

.addReg(RISCV::X1)
.addReg(RISCV::X1)
.addImm(0);
bool compressed = RISCVRVC::compress(CInst, Inst, *STI);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto.

wangpc-pp added a commit that referenced this pull request Apr 10, 2024
This is helpful to reduce calls of `RISCVRVC::compress` in #77337.

Reviewers: asb, lukel97, topperc

Reviewed By: topperc

Pull Request: #88120
@Zeavee Zeavee force-pushed the risc-v-stackmaps-with-target branch from e21c4d3 to bbd31c8 Compare April 10, 2024 08:48
Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wangpc-pp wangpc-pp merged commit 53003e3 into llvm:main Apr 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants