[Intrinsics] Make patchpoint.i64 generic on its return type
#85911
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write If you have received no comments on your PR for a week, you can request a review If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
backend:AArch64
backend:PowerPC
backend:SystemZ
backend:X86
llvm:SelectionDAG
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-backend-aarch64 Author: None (Il-Capitano) ChangesCurrently patchpoints can only have two result types, NOTE: The test case currently fails with the same issue described in #85828. Full diff: https://github.com/llvm/llvm-project/pull/85911.diff 10 Files Affected:
diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index 091f9b38107989..d0ef9c25f39ea8 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -1590,11 +1590,11 @@ def int_experimental_patchpoint_void : Intrinsic<[],
llvm_ptr_ty, llvm_i32_ty,
llvm_vararg_ty],
[Throws]>;
-def int_experimental_patchpoint_i64 : Intrinsic<[llvm_i64_ty],
- [llvm_i64_ty, llvm_i32_ty,
- llvm_ptr_ty, llvm_i32_ty,
- llvm_vararg_ty],
- [Throws]>;
+def int_experimental_patchpoint : Intrinsic<[llvm_any_ty],
+ [llvm_i64_ty, llvm_i32_ty,
+ llvm_ptr_ty, llvm_i32_ty,
+ llvm_vararg_ty],
+ [Throws]>;
//===------------------------ Garbage Collection Intrinsics ---------------===//
diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
index 8b834862fb4d89..fa147a681c3754 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -752,12 +752,12 @@ FastISel::CallLoweringInfo &FastISel::CallLoweringInfo::setCallee(
}
bool FastISel::selectPatchpoint(const CallInst *I) {
- // void|i64 @llvm.experimental.patchpoint.void|i64(i64 <id>,
- // i32 <numBytes>,
- // i8* <target>,
- // i32 <numArgs>,
- // [Args...],
- // [live variables...])
+ // <ty> @llvm.experimental.patchpoint.<ty>(i64 <id>,
+ // i32 <numBytes>,
+ // i8* <target>,
+ // i32 <numArgs>,
+ // [Args...],
+ // [live variables...])
CallingConv::ID CC = I->getCallingConv();
bool IsAnyRegCC = CC == CallingConv::AnyReg;
bool HasDef = !I->getType()->isVoidTy();
@@ -790,7 +790,9 @@ bool FastISel::selectPatchpoint(const CallInst *I) {
// Add an explicit result reg if we use the anyreg calling convention.
if (IsAnyRegCC && HasDef) {
assert(CLI.NumResultRegs == 0 && "Unexpected result register.");
- CLI.ResultReg = createResultReg(TLI.getRegClassFor(MVT::i64));
+ assert(I->getType()->isSingleValueType());
+ MVT ValueType = TLI.getSimpleValueType(DL, I->getType());
+ CLI.ResultReg = createResultReg(TLI.getRegClassFor(ValueType));
CLI.NumResultRegs = 1;
Ops.push_back(MachineOperand::CreateReg(CLI.ResultReg, /*isDef=*/true));
}
@@ -1464,7 +1466,7 @@ bool FastISel::selectIntrinsicCall(const IntrinsicInst *II) {
case Intrinsic::experimental_stackmap:
return selectStackmap(II);
case Intrinsic::experimental_patchpoint_void:
- case Intrinsic::experimental_patchpoint_i64:
+ case Intrinsic::experimental_patchpoint:
return selectPatchpoint(II);
case Intrinsic::xray_customevent:
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index dd19ee16d1d656..4ba968db74f50e 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -3330,7 +3330,7 @@ void SelectionDAGBuilder::visitInvoke(const InvokeInst &I) {
EHPadMBB->setMachineBlockAddressTaken();
break;
case Intrinsic::experimental_patchpoint_void:
- case Intrinsic::experimental_patchpoint_i64:
+ case Intrinsic::experimental_patchpoint:
visitPatchpoint(I, EHPadBB);
break;
case Intrinsic::experimental_gc_statepoint:
@@ -7443,7 +7443,7 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
visitStackmap(I);
return;
case Intrinsic::experimental_patchpoint_void:
- case Intrinsic::experimental_patchpoint_i64:
+ case Intrinsic::experimental_patchpoint:
visitPatchpoint(I);
return;
case Intrinsic::experimental_gc_statepoint:
@@ -10255,12 +10255,12 @@ void SelectionDAGBuilder::visitStackmap(const CallInst &CI) {
/// Lower llvm.experimental.patchpoint directly to its target opcode.
void SelectionDAGBuilder::visitPatchpoint(const CallBase &CB,
const BasicBlock *EHPadBB) {
- // void|i64 @llvm.experimental.patchpoint.void|i64(i64 <id>,
- // i32 <numBytes>,
- // i8* <target>,
- // i32 <numArgs>,
- // [Args...],
- // [live variables...])
+ // <ty> @llvm.experimental.patchpoint.<ty>(i64 <id>,
+ // i32 <numBytes>,
+ // i8* <target>,
+ // i32 <numArgs>,
+ // [Args...],
+ // [live variables...])
CallingConv::ID CC = CB.getCallingConv();
bool IsAnyRegCC = CC == CallingConv::AnyReg;
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 1e16e864846241..fc16c57ba32667 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -5016,7 +5016,7 @@ void Verifier::visitInstruction(Instruction &I) {
F->getIntrinsicID() == Intrinsic::coro_await_suspend_handle ||
F->getIntrinsicID() ==
Intrinsic::experimental_patchpoint_void ||
- F->getIntrinsicID() == Intrinsic::experimental_patchpoint_i64 ||
+ F->getIntrinsicID() == Intrinsic::experimental_patchpoint ||
F->getIntrinsicID() == Intrinsic::experimental_gc_statepoint ||
F->getIntrinsicID() == Intrinsic::wasm_rethrow ||
IsAttachedCallOperand(F, CBI, i),
@@ -5661,6 +5661,12 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
}
break;
}
+ case Intrinsic::experimental_patchpoint: {
+ // Check that result type can fit in a single register.
+ Check(Call.getType()->isSingleValueType(),
+ "patchpoint result type is not a valid type for a register", Call);
+ break;
+ }
case Intrinsic::eh_exceptioncode:
case Intrinsic::eh_exceptionpointer: {
Check(isa<CatchPadInst>(Call.getArgOperand(0)),
diff --git a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
index 7a86c5c6088120..ee7137b92445bb 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
+++ b/llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
@@ -479,7 +479,7 @@ AArch64TTIImpl::getIntImmCostIntrin(Intrinsic::ID IID, unsigned Idx,
return TTI::TCC_Free;
break;
case Intrinsic::experimental_patchpoint_void:
- case Intrinsic::experimental_patchpoint_i64:
+ case Intrinsic::experimental_patchpoint:
if ((Idx < 4) || (Imm.getBitWidth() <= 64 && isInt<64>(Imm.getSExtValue())))
return TTI::TCC_Free;
break;
diff --git a/llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp b/llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp
index 7adf1adcc64768..57e1019adb7410 100644
--- a/llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp
+++ b/llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp
@@ -219,7 +219,7 @@ InstructionCost PPCTTIImpl::getIntImmCostIntrin(Intrinsic::ID IID, unsigned Idx,
return TTI::TCC_Free;
break;
case Intrinsic::experimental_patchpoint_void:
- case Intrinsic::experimental_patchpoint_i64:
+ case Intrinsic::experimental_patchpoint:
if ((Idx < 4) || (Imm.getBitWidth() <= 64 && isInt<64>(Imm.getSExtValue())))
return TTI::TCC_Free;
break;
diff --git a/llvm/lib/Target/SystemZ/SystemZTargetTransformInfo.cpp b/llvm/lib/Target/SystemZ/SystemZTargetTransformInfo.cpp
index e4adb7be564952..5bdbaf47064d6c 100644
--- a/llvm/lib/Target/SystemZ/SystemZTargetTransformInfo.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZTargetTransformInfo.cpp
@@ -265,7 +265,7 @@ SystemZTTIImpl::getIntImmCostIntrin(Intrinsic::ID IID, unsigned Idx,
return TTI::TCC_Free;
break;
case Intrinsic::experimental_patchpoint_void:
- case Intrinsic::experimental_patchpoint_i64:
+ case Intrinsic::experimental_patchpoint:
if ((Idx < 4) || (Imm.getBitWidth() <= 64 && isInt<64>(Imm.getSExtValue())))
return TTI::TCC_Free;
break;
diff --git a/llvm/lib/Target/X86/X86TargetTransformInfo.cpp b/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
index d336ab9d309c4e..a9e1eec68251cd 100644
--- a/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
+++ b/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
@@ -5651,7 +5651,7 @@ InstructionCost X86TTIImpl::getIntImmCostIntrin(Intrinsic::ID IID, unsigned Idx,
return TTI::TCC_Free;
break;
case Intrinsic::experimental_patchpoint_void:
- case Intrinsic::experimental_patchpoint_i64:
+ case Intrinsic::experimental_patchpoint:
if ((Idx < 4) || (Imm.getBitWidth() <= 64 && Imm.isSignedIntN(64)))
return TTI::TCC_Free;
break;
diff --git a/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp b/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp
index 436a85f62df681..f5c9aaa4f20bc7 100644
--- a/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp
+++ b/llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp
@@ -517,7 +517,7 @@ static bool doesNotRequireEntrySafepointBefore(CallBase *Call) {
switch (II->getIntrinsicID()) {
case Intrinsic::experimental_gc_statepoint:
case Intrinsic::experimental_patchpoint_void:
- case Intrinsic::experimental_patchpoint_i64:
+ case Intrinsic::experimental_patchpoint:
// The can wrap an actual call which may grow the stack by an unbounded
// amount or run forever.
return false;
diff --git a/llvm/test/CodeGen/AArch64/arm64-patchpoint.ll b/llvm/test/CodeGen/AArch64/arm64-patchpoint.ll
index c58f4b10290974..10a7cdaa5a7f64 100644
--- a/llvm/test/CodeGen/AArch64/arm64-patchpoint.ll
+++ b/llvm/test/CodeGen/AArch64/arm64-patchpoint.ll
@@ -79,6 +79,61 @@ entry:
ret void
}
+; Test register allocation for an i32 result value of patchpoint.
+define i32 @generic_patchpoint_i32() {
+entry:
+; CHECK-LABEL: generic_patchpoint_i32:
+; CHECK: Ltmp
+; CHECK-NEXT: nop
+; The return value is already in w0.
+; CHECK-NEXT: ldp
+; CHECK-NEXT: ret
+ %result = tail call i32 (i64, i32, ptr, i32, ...) @llvm.experimental.patchpoint.i32(i64 5, i32 4, ptr null, i32 0)
+ ret i32 %result
+}
+
+; Test register allocation for an i64 result value of patchpoint.
+define i64 @generic_patchpoint_i64() {
+entry:
+; CHECK-LABEL: generic_patchpoint_i64:
+; CHECK: Ltmp
+; CHECK-NEXT: nop
+; The return value is already in x0.
+; CHECK-NEXT: ldp
+; CHECK-NEXT: ret
+ %result = tail call i64 (i64, i32, ptr, i32, ...) @llvm.experimental.patchpoint.i64(i64 5, i32 4, ptr null, i32 0)
+ ret i64 %result
+}
+
+; Test register allocation for a float result value of patchpoint.
+define float @generic_patchpoint_f32() {
+entry:
+; CHECK-LABEL: generic_patchpoint_f32:
+; CHECK: Ltmp
+; CHECK-NEXT: nop
+; The return value is already in s0.
+; CHECK-NEXT: ldp
+; CHECK-NEXT: ret
+ %result = tail call float (i64, i32, ptr, i32, ...) @llvm.experimental.patchpoint.f32(i64 5, i32 4, ptr null, i32 0)
+ ret float %result
+}
+
+; Test register allocation for a double result value of patchpoint.
+define double @generic_patchpoint_f64() {
+entry:
+; CHECK-LABEL: generic_patchpoint_f64:
+; CHECK: Ltmp
+; CHECK-NEXT: nop
+; The return value is already in d0.
+; CHECK-NEXT: ldp
+; CHECK-NEXT: ret
+ %result = tail call double (i64, i32, ptr, i32, ...) @llvm.experimental.patchpoint.f64(i64 5, i32 4, ptr null, i32 0)
+ ret double %result
+}
+
declare void @llvm.experimental.stackmap(i64, i32, ...)
declare void @llvm.experimental.patchpoint.void(i64, i32, ptr, i32, ...)
+declare i32 @llvm.experimental.patchpoint.i32(i64, i32, ptr, i32, ...)
declare i64 @llvm.experimental.patchpoint.i64(i64, i32, ptr, i32, ...)
+declare float @llvm.experimental.patchpoint.f32(i64, i32, ptr, i32, ...)
+declare double @llvm.experimental.patchpoint.f64(i64, i32, ptr, i32, ...)
|
This allows patchpoints to allocate different registers for their result values, e.g. FP registers.
Il-Capitano
force-pushed
the
generic-patchpoint
branch
from
515adbb
to
a8751cc
Compare
Patchpoints lower to function calls or I constrained the return type because of two concerns:
Thinking about it a bit more, the constraint is only needed in case Let me know if this approach would be reasonable. |
* FastISel now fails for an invalid return type, instead of asserting. * The type constraing in the verifier is only applied for `anyregcc`. * Added tests for pointer, half and vector return types. * Added tests using `anyregcc`.
| @@ -3330,7 +3330,7 @@ void SelectionDAGBuilder::visitInvoke(const InvokeInst &I) { | |||
| EHPadMBB->setMachineBlockAddressTaken(); | |||
| break; | |||
| case Intrinsic::experimental_patchpoint_void: | |||
There was a problem hiding this comment.
Do we still need a specific void intrinsic?
There was a problem hiding this comment.
I tried removing it, and it seems like it changes the mangled name to @llvm.experimental.patchpoint.isVoid. I'm not sure if this would be desireable.
There was a problem hiding this comment.
The mangling for void is the ugly isVoid, but I don't see that as a great reason to leave around an intrinsic that only differs by adding a concrete return type
There was a problem hiding this comment.
Should I open another PR with that change?
|
@Il-Capitano Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested Please check whether problems have been caused by your change specifically, as How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
Currently patchpoints can only have two result types,
voidandi64. This limits the result to general purpose registers.This patch makes
patchpoint.i64an overloadable intrinsic, allowing result values that can fit in a single register (e.g. integers, pointers, floats).