Skip to content

[Intrinsics] Make patchpoint.i64 generic on its return type #85911

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 26, 2024
+404 −30
Merged

[Intrinsics] Make patchpoint.i64 generic on its return type #85911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a8751cc
[Intrinsics] Make `patchpoint.i64` generic on its return type
Il-Capitano Mar 20, 2024
33aea74
Address review feedback and add more tests
Il-Capitano Mar 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions llvm/include/llvm/IR/Intrinsics.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 ---------------===//
Expand Down
25 changes: 17 additions & 8 deletions llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,17 +752,25 @@ 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();
Value *Callee = I->getOperand(PatchPointOpers::TargetPos)->stripPointerCasts();

// Check if we can lower the return type when using anyregcc.
MVT ValueType;
if (IsAnyRegCC && HasDef) {
ValueType = TLI.getSimpleValueType(DL, I->getType(), /*AllowUnknown=*/true);
if (ValueType == MVT::Other)
return false;
}

// Get the real number of arguments participating in the call <numArgs>
assert(isa<ConstantInt>(I->getOperand(PatchPointOpers::NArgPos)) &&
"Expected a constant integer.");
Expand Down Expand Up @@ -790,7 +798,8 @@ 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(ValueType.isValid());
CLI.ResultReg = createResultReg(TLI.getRegClassFor(ValueType));
CLI.NumResultRegs = 1;
Ops.push_back(MachineOperand::CreateReg(CLI.ResultReg, /*isDef=*/true));
}
Expand Down Expand Up @@ -1464,7 +1473,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:
Expand Down
16 changes: 8 additions & 8 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3330,7 +3330,7 @@ void SelectionDAGBuilder::visitInvoke(const InvokeInst &I) {
EHPadMBB->setMachineBlockAddressTaken();
break;
case Intrinsic::experimental_patchpoint_void:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need a specific void intrinsic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I open another PR with that change?

case Intrinsic::experimental_patchpoint_i64:
case Intrinsic::experimental_patchpoint:
visitPatchpoint(I, EHPadBB);
break;
case Intrinsic::experimental_gc_statepoint:
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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;
Expand Down
9 changes: 8 additions & 1 deletion llvm/lib/IR/Verifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -5661,6 +5661,13 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
}
break;
}
case Intrinsic::experimental_patchpoint: {
if (Call.getCallingConv() == CallingConv::AnyReg) {
Check(Call.getType()->isSingleValueType(),
"patchpoint: invalid return type used with anyregcc", Call);
}
break;
}
case Intrinsic::eh_exceptioncode:
case Intrinsic::eh_exceptionpointer: {
Check(isa<CatchPadInst>(Call.getArgOperand(0)),
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/PowerPC/PPCTargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/SystemZ/SystemZTargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Target/X86/X86TargetTransformInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Transforms/Scalar/PlaceSafepoints.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading