[RISCV][GISel] Add FP calling convention support #69138
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
topperc
requested review from
asb,
aemerson,
arsenm,
luismarques,
mshockwave,
michaelmaitland and
nitinjohnraj
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-llvm-globalisel Author: Craig Topper (topperc) ChangesThis does not include passing values on the stack. Test cases copied from SelectionDAG and converted to check MIR output. Test cases that require stack were removed. This is stacked on #69118 and #69129 Patch is 46.70 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/69138.diff 8 Files Affected:
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
index a362a709329d5df..b0d692795730df5 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
@@ -77,6 +77,44 @@ struct RISCVOutgoingValueHandler : public CallLowering::OutgoingValueHandler {
MIRBuilder.buildCopy(PhysReg, ExtReg);
MIB.addUse(PhysReg, RegState::Implicit);
}
+
+ unsigned assignCustomValue(CallLowering::ArgInfo &Arg,
+ ArrayRef<CCValAssign> VAs,
+ std::function<void()> *Thunk) override {
+ assert(Arg.Regs.size() == 1 && "Can't handle multple regs yet");
+
+ CCValAssign VA = VAs[0];
+ assert(VA.needsCustom() && "Value doesn't need custom handling");
+
+ // Custom lowering for other types, such as f16, is currently not supported
+ if (VA.getValVT() != MVT::f64)
+ return 0;
+
+ CCValAssign NextVA = VAs[1];
+ assert(NextVA.needsCustom() && "Value doesn't need custom handling");
+ assert(NextVA.getValVT() == MVT::f64 && "Unsupported type");
+
+ assert(VA.getValNo() == NextVA.getValNo() &&
+ "Values belong to different arguments");
+
+ assert(VA.isRegLoc() && "Value should be in reg");
+ assert(NextVA.isRegLoc() && "Value should be in reg");
+
+ Register NewRegs[] = {MRI.createGenericVirtualRegister(LLT::scalar(32)),
+ MRI.createGenericVirtualRegister(LLT::scalar(32))};
+ MIRBuilder.buildUnmerge(NewRegs, Arg.Regs[0]);
+
+ if (Thunk) {
+ *Thunk = [=]() {
+ assignValueToReg(NewRegs[0], VA.getLocReg(), VA);
+ assignValueToReg(NewRegs[1], NextVA.getLocReg(), NextVA);
+ };
+ return 1;
+ }
+ assignValueToReg(NewRegs[0], VA.getLocReg(), VA);
+ assignValueToReg(NewRegs[1], NextVA.getLocReg(), NextVA);
+ return 1;
+ }
};
struct RISCVIncomingValueAssigner : public CallLowering::IncomingValueAssigner {
@@ -127,9 +165,50 @@ struct RISCVIncomingValueHandler : public CallLowering::IncomingValueHandler {
void assignValueToReg(Register ValVReg, Register PhysReg,
CCValAssign VA) override {
- // Copy argument received in physical register to desired VReg.
+ markPhysRegUsed(PhysReg);
+ IncomingValueHandler::assignValueToReg(ValVReg, PhysReg, VA);
+ }
+
+ unsigned assignCustomValue(CallLowering::ArgInfo &Arg,
+ ArrayRef<CCValAssign> VAs,
+ std::function<void()> *Thunk = nullptr) override {
+ const CCValAssign &VALo = VAs[0];
+ const CCValAssign &VAHi = VAs[1];
+
+ assert(VAHi.needsCustom() && "Value doesn't need custom handling");
+
+ assert(VALo.getLocVT() == MVT::i32 && VAHi.getLocVT() == MVT::i32 &&
+ VALo.getValVT() == MVT::f64 && VAHi.getValVT() == MVT::f64 &&
+ "unexpected custom value");
+
+ Register NewRegs[] = {MRI.createGenericVirtualRegister(LLT::scalar(32)),
+ MRI.createGenericVirtualRegister(LLT::scalar(32))};
+
+ assignValueToReg(NewRegs[0], VALo.getLocReg(), VALo);
+ assignValueToReg(NewRegs[1], VAHi.getLocReg(), VAHi);
+
+ MIRBuilder.buildMergeLikeInstr(Arg.Regs[0], NewRegs);
+
+ return 1;
+ }
+
+ /// How the physical register gets marked varies between formal
+ /// parameters (it's a basic-block live-in), and a call instruction
+ /// (it's an implicit-def of the BL).
+ virtual void markPhysRegUsed(MCRegister PhysReg) = 0;
+};
+
+struct RISCVFormalArgHandler : public RISCVIncomingValueHandler {
+ RISCVFormalArgHandler(MachineIRBuilder &B, MachineRegisterInfo &MRI)
+ : RISCVIncomingValueHandler(B, MRI) {}
+
+ void markPhysRegUsed(MCRegister PhysReg) override {
+ MIRBuilder.getMRI()->addLiveIn(PhysReg);
+ MIRBuilder.getMBB().addLiveIn(PhysReg);
+ }
+
+ virtual void markPhysRegUsed(unsigned PhysReg) {
MIRBuilder.getMBB().addLiveIn(PhysReg);
- MIRBuilder.buildCopy(ValVReg, PhysReg);
}
};
@@ -138,14 +217,11 @@ struct RISCVCallReturnHandler : public RISCVIncomingValueHandler {
MachineInstrBuilder &MIB)
: RISCVIncomingValueHandler(B, MRI), MIB(MIB) {}
- MachineInstrBuilder MIB;
-
- void assignValueToReg(Register ValVReg, Register PhysReg,
- CCValAssign VA) override {
- // Copy argument received in physical register to desired VReg.
+ void markPhysRegUsed(MCRegister PhysReg) override {
MIB.addDef(PhysReg, RegState::Implicit);
- MIRBuilder.buildCopy(ValVReg, PhysReg);
}
+
+ MachineInstrBuilder MIB;
};
} // namespace
@@ -153,6 +229,28 @@ struct RISCVCallReturnHandler : public RISCVIncomingValueHandler {
RISCVCallLowering::RISCVCallLowering(const RISCVTargetLowering &TLI)
: CallLowering(&TLI) {}
+static bool isSupportedArgumentType(Type *T) {
+ if (T->isIntegerTy())
+ return true;
+ if (T->isPointerTy())
+ return true;
+ if (T->isFloatingPointTy())
+ return true;
+ return false;
+}
+
+static bool isSupportedReturnType(Type *T) {
+ if (T->isIntegerTy())
+ return true;
+ if (T->isPointerTy())
+ return true;
+ if (T->isFloatingPointTy())
+ return true;
+ if (T->isAggregateType())
+ return true;
+ return false;
+}
+
bool RISCVCallLowering::lowerReturnVal(MachineIRBuilder &MIRBuilder,
const Value *Val,
ArrayRef<Register> VRegs,
@@ -160,8 +258,7 @@ bool RISCVCallLowering::lowerReturnVal(MachineIRBuilder &MIRBuilder,
if (!Val)
return true;
- // TODO: Only integer, pointer and aggregate types are supported now.
- if (!Val->getType()->isIntOrPtrTy() && !Val->getType()->isAggregateType())
+ if (!isSupportedReturnType(Val->getType()))
return false;
MachineFunction &MF = MIRBuilder.getMF();
@@ -210,11 +307,8 @@ bool RISCVCallLowering::lowerFormalArguments(MachineIRBuilder &MIRBuilder,
// TODO: Support all argument types.
for (auto &Arg : F.args()) {
- if (Arg.getType()->isIntegerTy())
- continue;
- if (Arg.getType()->isPointerTy())
- continue;
- return false;
+ if (!isSupportedArgumentType(Arg.getType()))
+ return false;
}
MachineFunction &MF = MIRBuilder.getMF();
@@ -239,7 +333,7 @@ bool RISCVCallLowering::lowerFormalArguments(MachineIRBuilder &MIRBuilder,
RISCVIncomingValueAssigner Assigner(
CC == CallingConv::Fast ? RISCV::CC_RISCV_FastCC : RISCV::CC_RISCV,
/*IsRet=*/false);
- RISCVIncomingValueHandler Handler(MIRBuilder, MF.getRegInfo());
+ RISCVFormalArgHandler Handler(MIRBuilder, MF.getRegInfo());
return determineAndHandleAssignments(Handler, Assigner, SplitArgInfos,
MIRBuilder, CC, F.isVarArg());
@@ -253,14 +347,9 @@ bool RISCVCallLowering::lowerCall(MachineIRBuilder &MIRBuilder,
CallingConv::ID CC = F.getCallingConv();
// TODO: Support all argument types.
- for (auto &AInfo : Info.OrigArgs) {
- if (AInfo.Ty->isIntegerTy())
- continue;
- if (AInfo.Ty->isPointerTy())
- continue;
- if (AInfo.Ty->isFloatingPointTy())
- continue;
- return false;
+ for (auto &Arg : Info.OrigArgs) {
+ if (!isSupportedArgumentType(Arg.Ty))
+ return false;
}
SmallVector<ArgInfo, 32> SplitArgInfos;
@@ -294,13 +383,9 @@ bool RISCVCallLowering::lowerCall(MachineIRBuilder &MIRBuilder,
MIRBuilder.insertInstr(Call);
- if (Info.OrigRet.Ty->isVoidTy())
+ if (Info.OrigRet.Ty->isVoidTy() || !isSupportedReturnType(Info.OrigRet.Ty))
return true;
- // TODO: Only integer, pointer and aggregate types are supported now.
- if (!Info.OrigRet.Ty->isIntOrPtrTy() && !Info.OrigRet.Ty->isAggregateType())
- return false;
-
SmallVector<ArgInfo, 4> SplitRetInfos;
splitToValueTypes(Info.OrigRet, SplitRetInfos, DL, CC);
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index d7552317fd8bc69..50e48c86b2a5077 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -16452,16 +16452,23 @@ bool RISCV::CC_RISCV(const DataLayout &DL, RISCVABI::ABI ABI, unsigned ValNo,
// stack. LowerCall/LowerFormalArguments/LowerReturn must recognise these
// cases.
Register Reg = State.AllocateReg(ArgGPRs);
- LocVT = MVT::i32;
if (!Reg) {
unsigned StackOffset = State.AllocateStack(8, Align(8));
State.addLoc(
CCValAssign::getMem(ValNo, ValVT, StackOffset, LocVT, LocInfo));
return false;
}
- if (!State.AllocateReg(ArgGPRs))
- State.AllocateStack(4, Align(4));
- State.addLoc(CCValAssign::getReg(ValNo, ValVT, Reg, LocVT, LocInfo));
+ LocVT = MVT::i32;
+ State.addLoc(CCValAssign::getCustomReg(ValNo, ValVT, Reg, LocVT, LocInfo));
+ Register HiReg = State.AllocateReg(ArgGPRs);
+ if (HiReg) {
+ State.addLoc(
+ CCValAssign::getCustomReg(ValNo, ValVT, HiReg, LocVT, LocInfo));
+ } else {
+ unsigned StackOffset = State.AllocateStack(4, Align(4));
+ State.addLoc(
+ CCValAssign::getCustomMem(ValNo, ValVT, StackOffset, LocVT, LocInfo));
+ }
return false;
}
@@ -16770,38 +16777,32 @@ static SDValue unpackFromMemLoc(SelectionDAG &DAG, SDValue Chain,
}
static SDValue unpackF64OnRV32DSoftABI(SelectionDAG &DAG, SDValue Chain,
- const CCValAssign &VA, const SDLoc &DL) {
+ const CCValAssign &VA,
+ const CCValAssign &HiVA,
+ const SDLoc &DL) {
assert(VA.getLocVT() == MVT::i32 && VA.getValVT() == MVT::f64 &&
"Unexpected VA");
MachineFunction &MF = DAG.getMachineFunction();
MachineFrameInfo &MFI = MF.getFrameInfo();
MachineRegisterInfo &RegInfo = MF.getRegInfo();
- if (VA.isMemLoc()) {
- // f64 is passed on the stack.
- int FI =
- MFI.CreateFixedObject(8, VA.getLocMemOffset(), /*IsImmutable=*/true);
- SDValue FIN = DAG.getFrameIndex(FI, MVT::i32);
- return DAG.getLoad(MVT::f64, DL, Chain, FIN,
- MachinePointerInfo::getFixedStack(MF, FI));
- }
-
assert(VA.isRegLoc() && "Expected register VA assignment");
Register LoVReg = RegInfo.createVirtualRegister(&RISCV::GPRRegClass);
RegInfo.addLiveIn(VA.getLocReg(), LoVReg);
SDValue Lo = DAG.getCopyFromReg(Chain, DL, LoVReg, MVT::i32);
SDValue Hi;
- if (VA.getLocReg() == RISCV::X17) {
+ if (HiVA.isMemLoc()) {
// Second half of f64 is passed on the stack.
- int FI = MFI.CreateFixedObject(4, 0, /*IsImmutable=*/true);
+ int FI = MFI.CreateFixedObject(4, HiVA.getLocMemOffset(),
+ /*IsImmutable=*/true);
SDValue FIN = DAG.getFrameIndex(FI, MVT::i32);
Hi = DAG.getLoad(MVT::i32, DL, Chain, FIN,
MachinePointerInfo::getFixedStack(MF, FI));
} else {
// Second half of f64 is passed in another GPR.
Register HiVReg = RegInfo.createVirtualRegister(&RISCV::GPRRegClass);
- RegInfo.addLiveIn(VA.getLocReg() + 1, HiVReg);
+ RegInfo.addLiveIn(HiVA.getLocReg(), HiVReg);
Hi = DAG.getCopyFromReg(Chain, DL, HiVReg, MVT::i32);
}
return DAG.getNode(RISCVISD::BuildPairF64, DL, MVT::f64, Lo, Hi);
@@ -17044,15 +17045,16 @@ SDValue RISCVTargetLowering::LowerFormalArguments(
CallConv == CallingConv::Fast ? RISCV::CC_RISCV_FastCC
: RISCV::CC_RISCV);
- for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
+ for (unsigned i = 0, e = ArgLocs.size(), InsIdx = 0; i != e; ++i, ++InsIdx) {
CCValAssign &VA = ArgLocs[i];
SDValue ArgValue;
// Passing f64 on RV32D with a soft float ABI must be handled as a special
// case.
- if (VA.getLocVT() == MVT::i32 && VA.getValVT() == MVT::f64)
- ArgValue = unpackF64OnRV32DSoftABI(DAG, Chain, VA, DL);
- else if (VA.isRegLoc())
- ArgValue = unpackFromRegLoc(DAG, Chain, VA, DL, Ins[i], *this);
+ if (VA.getLocVT() == MVT::i32 && VA.getValVT() == MVT::f64) {
+ assert(VA.needsCustom());
+ ArgValue = unpackF64OnRV32DSoftABI(DAG, Chain, VA, ArgLocs[++i], DL);
+ } else if (VA.isRegLoc())
+ ArgValue = unpackFromRegLoc(DAG, Chain, VA, DL, Ins[InsIdx], *this);
else
ArgValue = unpackFromMemLoc(DAG, Chain, VA, DL);
@@ -17064,12 +17066,12 @@ SDValue RISCVTargetLowering::LowerFormalArguments(
// stores are relative to that.
InVals.push_back(DAG.getLoad(VA.getValVT(), DL, Chain, ArgValue,
MachinePointerInfo()));
- unsigned ArgIndex = Ins[i].OrigArgIndex;
- unsigned ArgPartOffset = Ins[i].PartOffset;
+ unsigned ArgIndex = Ins[InsIdx].OrigArgIndex;
+ unsigned ArgPartOffset = Ins[InsIdx].PartOffset;
assert(VA.getValVT().isVector() || ArgPartOffset == 0);
- while (i + 1 != e && Ins[i + 1].OrigArgIndex == ArgIndex) {
+ while (i + 1 != e && Ins[InsIdx + 1].OrigArgIndex == ArgIndex) {
CCValAssign &PartVA = ArgLocs[i + 1];
- unsigned PartOffset = Ins[i + 1].PartOffset - ArgPartOffset;
+ unsigned PartOffset = Ins[InsIdx + 1].PartOffset - ArgPartOffset;
SDValue Offset = DAG.getIntPtrConstant(PartOffset, DL);
if (PartVA.getValVT().isScalableVector())
Offset = DAG.getNode(ISD::VSCALE, DL, XLenVT, Offset);
@@ -17077,6 +17079,7 @@ SDValue RISCVTargetLowering::LowerFormalArguments(
InVals.push_back(DAG.getLoad(PartVA.getValVT(), DL, Chain, Address,
MachinePointerInfo()));
++i;
+ ++InsIdx;
}
continue;
}
@@ -17292,15 +17295,18 @@ SDValue RISCVTargetLowering::LowerCall(CallLoweringInfo &CLI,
SmallVector<std::pair<Register, SDValue>, 8> RegsToPass;
SmallVector<SDValue, 8> MemOpChains;
SDValue StackPtr;
- for (unsigned i = 0, j = 0, e = ArgLocs.size(); i != e; ++i) {
+ for (unsigned i = 0, j = 0, e = ArgLocs.size(), OutIdx = 0; i != e;
+ ++i, ++OutIdx) {
CCValAssign &VA = ArgLocs[i];
- SDValue ArgValue = OutVals[i];
- ISD::ArgFlagsTy Flags = Outs[i].Flags;
+ SDValue ArgValue = OutVals[OutIdx];
+ ISD::ArgFlagsTy Flags = Outs[OutIdx].Flags;
// Handle passing f64 on RV32D with a soft float ABI as a special case.
bool IsF64OnRV32DSoftABI =
VA.getLocVT() == MVT::i32 && VA.getValVT() == MVT::f64;
- if (IsF64OnRV32DSoftABI && VA.isRegLoc()) {
+ if (IsF64OnRV32DSoftABI) {
+ assert(VA.isRegLoc() && "Expected register VA assignment");
+ assert(VA.needsCustom());
SDValue SplitF64 = DAG.getNode(
RISCVISD::SplitF64, DL, DAG.getVTList(MVT::i32, MVT::i32), ArgValue);
SDValue Lo = SplitF64.getValue(0);
@@ -17309,18 +17315,22 @@ SDValue RISCVTargetLowering::LowerCall(CallLoweringInfo &CLI,
Register RegLo = VA.getLocReg();
RegsToPass.push_back(std::make_pair(RegLo, Lo));
- if (RegLo == RISCV::X17) {
+ // Get the CCValAssign for the Hi part.
+ CCValAssign &HiVA = ArgLocs[++i];
+
+ if (HiVA.isMemLoc()) {
// Second half of f64 is passed on the stack.
- // Work out the address of the stack slot.
if (!StackPtr.getNode())
StackPtr = DAG.getCopyFromReg(Chain, DL, RISCV::X2, PtrVT);
+ SDValue Address =
+ DAG.getNode(ISD::ADD, DL, PtrVT, StackPtr,
+ DAG.getIntPtrConstant(HiVA.getLocMemOffset(), DL));
// Emit the store.
MemOpChains.push_back(
- DAG.getStore(Chain, DL, Hi, StackPtr, MachinePointerInfo()));
+ DAG.getStore(Chain, DL, Hi, Address, MachinePointerInfo()));
} else {
// Second half of f64 is passed in another GPR.
- assert(RegLo < RISCV::X31 && "Invalid register pair");
- Register RegHigh = RegLo + 1;
+ Register RegHigh = HiVA.getLocReg();
RegsToPass.push_back(std::make_pair(RegHigh, Hi));
}
continue;
@@ -17334,7 +17344,7 @@ SDValue RISCVTargetLowering::LowerCall(CallLoweringInfo &CLI,
if (VA.getLocInfo() == CCValAssign::Indirect) {
// Store the argument in a stack slot and pass its address.
Align StackAlign =
- std::max(getPrefTypeAlign(Outs[i].ArgVT, DAG),
+ std::max(getPrefTypeAlign(Outs[OutIdx].ArgVT, DAG),
getPrefTypeAlign(ArgValue.getValueType(), DAG));
TypeSize StoredSize = ArgValue.getValueType().getStoreSize();
// If the original argument was split (e.g. i128), we need
@@ -17342,16 +17352,16 @@ SDValue RISCVTargetLowering::LowerCall(CallLoweringInfo &CLI,
// Vectors may be partly split to registers and partly to the stack, in
// which case the base address is partly offset and subsequent stores are
// relative to that.
- unsigned ArgIndex = Outs[i].OrigArgIndex;
- unsigned ArgPartOffset = Outs[i].PartOffset;
+ unsigned ArgIndex = Outs[OutIdx].OrigArgIndex;
+ unsigned ArgPartOffset = Outs[OutIdx].PartOffset;
assert(VA.getValVT().isVector() || ArgPartOffset == 0);
// Calculate the total size to store. We don't have access to what we're
// actually storing other than performing the loop and collecting the
// info.
SmallVector<std::pair<SDValue, SDValue>> Parts;
- while (i + 1 != e && Outs[i + 1].OrigArgIndex == ArgIndex) {
+ while (i + 1 != e && Outs[OutIdx + 1].OrigArgIndex == ArgIndex) {
SDValue PartValue = OutVals[i + 1];
- unsigned PartOffset = Outs[i + 1].PartOffset - ArgPartOffset;
+ unsigned PartOffset = Outs[OutIdx + 1].PartOffset - ArgPartOffset;
SDValue Offset = DAG.getIntPtrConstant(PartOffset, DL);
EVT PartVT = PartValue.getValueType();
if (PartVT.isScalableVector())
@@ -17360,6 +17370,7 @@ SDValue RISCVTargetLowering::LowerCall(CallLoweringInfo &CLI,
StackAlign = std::max(StackAlign, getPrefTypeAlign(PartVT, DAG));
Parts.push_back(std::make_pair(PartValue, Offset));
++i;
+ ++OutIdx;
}
SDValue SpillSlot = DAG.CreateStackTemporary(StoredSize, StackAlign);
int FI = cast<FrameIndexSDNode>(SpillSlot)->getIndex();
@@ -17501,7 +17512,8 @@ SDValue RISCVTargetLowering::LowerCall(CallLoweringInfo &CLI,
analyzeInputArgs(MF, RetCCInfo, Ins, /*IsRet=*/true, RISCV::CC_RISCV);
// Copy all of the result registers out of their specified physreg.
- for (auto &VA : RVLocs) {
+ for (unsigned i = 0, e = RVLocs.size(); i != e; ++i) {
+ auto &VA = RVLocs[i];
// Copy the value out
SDValue RetValue =
DAG.getCopyFromReg(Chain, DL, VA.getLocReg(), VA.getLocVT(), Glue);
@@ -17510,9 +17522,9 @@ SDValue RISCVTargetLowering::LowerCall(CallLoweringInfo &CLI,
Glue = RetValue.getValue(2);
if (VA.getLocVT() == MVT::i32 && VA.getValVT() == MVT::f64) {
- assert(VA.getLocReg() == ArgGPRs[0] && "Unexpected reg assignment");
- SDValue RetValue2 =
- DAG.getCopyFromReg(Chain, DL, ArgGPRs[1], MVT::i32, Glue);
+ assert(VA.needsCustom());
+ SDValue RetValue2 = DAG.getCopyFromReg(Chain, DL, RVLocs[++i].getLocReg(),
+ MVT::i32, Glue);
Chain = RetValue2.getValue(1);
Glue = RetValue2.getValue(2);
RetValue = DAG.getNode(RISCVISD::BuildPairF64, DL, MVT::f64, RetValue,
@@ -17575,21 +17587,21 @@ RISCVTargetLowering::LowerReturn(SDValue Chain, CallingConv::ID CallConv,
SmallVector<SDValue, 4> RetOps(1, Chain);
// Copy the result values into the output registers.
- for (unsigned i = 0, e = RVLocs.size(); i < e; ++i) {
- SDValue Val = OutVals[i];
+ for (unsigned i = 0, e = RVLocs.size(), OutIdx = 0; i < e; ++i, ++OutIdx) {
+ SDValue Val = OutVals[OutIdx];
CCValAssign &VA = RVLocs[i];
assert(VA.isRegLoc() && "Can only return in registers!");
if (VA.getLocVT() == MVT::i32 && VA.getValVT() == MVT::f64) {
// Handle returning f64 on RV32D with a soft float ABI.
assert(VA.isRegLoc() && "Expected return via registers");
+ assert(VA.needsCustom());
SDValue SplitF64 = DAG.getNode(RISCVISD::SplitF64, DL,
DAG.getVTList(MVT::i32, MVT::i32), Val);
SDValue Lo = SplitF64.getValue(0);
SDValue Hi = SplitF64.getValue(1);
Register RegLo = VA.ge...
[truncated]
|
topperc
commented
Oct 16, 2023
| @@ -294,13 +377,9 @@ bool RISCVCallLowering::lowerCall(MachineIRBuilder &MIRBuilder, | |||
|
|
|||
| MIRBuilder.insertInstr(Call); | |||
|
|
|||
| if (Info.OrigRet.Ty->isVoidTy()) | |||
| if (Info.OrigRet.Ty->isVoidTy() || !isSupportedReturnType(Info.OrigRet.Ty)) | |||
There was a problem hiding this comment.
This is mistake. I shouldn't have merged these ifs
topperc
force-pushed
the
pr/gisel-fp-call
branch
2 times, most recently
from
805ce98 to
274b53a
Compare
preames
reviewed
Oct 20, 2023
| unsigned assignCustomValue(CallLowering::ArgInfo &Arg, | ||
| ArrayRef<CCValAssign> VAs, | ||
| std::function<void()> *Thunk) override { | ||
| const CCValAssign &VALo = VAs[0]; |
There was a problem hiding this comment.
Add an assert on VAs.size() being 2.
There was a problem hiding this comment.
The size isn't ==2. It is >=2. The caller has no idea how many entries this handler needs. It's continuously calling slice on an ArrayRef of all VAs to drop the ones that have been consumed. I'll add the assert for >=2
| } | ||
|
|
||
| unsigned assignCustomValue(CallLowering::ArgInfo &Arg, | ||
| ArrayRef<CCValAssign> VAs, | ||
| std::function<void()> *Thunk = nullptr) override { |
There was a problem hiding this comment.
The interface here is weird. The thunk is used to delay outgoing register assignments. The caller always passes Thunk, but I don't think it really wants to use Thunk for incoming assignments.
In the non-custom code in the caller we have this
if (Handler.isIncomingArgumentHandler())
Handler.assignValueToReg(ArgReg, VA.getLocReg(), VA);
else {
DelayedOutgoingRegAssignments.emplace_back([=, &Handler]() {
Handler.assignValueToReg(ArgReg, VA.getLocReg(), VA);
});
This is the code where thunk is passed and used.
if (VA.needsCustom()) {
std::function<void()> Thunk;
unsigned NumArgRegs = Handler.assignCustomValue(
Args[i], ArrayRef(ArgLocs).slice(j), &Thunk);
if (Thunk)
DelayedOutgoingRegAssignments.emplace_back(Thunk);
if (!NumArgRegs)
return false;
j += NumArgRegs;
continue;
}
So it seems to me that we shouldn't be using Thunk for incoming to match how the non-custom code sets up DelayedOutgoingRegAssignments.
| unsigned assignCustomValue(CallLowering::ArgInfo &Arg, | ||
| ArrayRef<CCValAssign> VAs, | ||
| std::function<void()> *Thunk = nullptr) override { | ||
| const CCValAssign &VALo = VAs[0]; |
| }; | ||
|
|
||
| } // namespace | ||
|
|
||
| RISCVCallLowering::RISCVCallLowering(const RISCVTargetLowering &TLI) | ||
| : CallLowering(&TLI) {} | ||
|
|
||
| static bool isSupportedArgumentType(Type *T) { |
| // Copy argument received in physical register to desired VReg. | ||
| MIRBuilder.getMBB().addLiveIn(PhysReg); | ||
| MIRBuilder.buildCopy(ValVReg, PhysReg); | ||
| markPhysRegUsed(PhysReg); |
There was a problem hiding this comment.
Correct me if I'm wrong, but it looks like this plus a couple lines below in terms of changing markPhysRegUsed definitions is an NFC-esq refactoring? Can it be landed separately?
There was a problem hiding this comment.
It is in the PR as a separate commit. I'll commit it separately and not squash it.
topperc
force-pushed
the
pr/gisel-fp-call
branch
2 times, most recently
from
e70c480 to
940719d
Compare
topperc
changed the title
topperc
added a commit
to topperc/llvm-project
that referenced
this pull request
Oct 21, 2023
Previously they were passed by non-const reference. No in tree target modifies the values. This makes it possible to call assignValueToAddress from assignCustomValue without a const_cast. For example in this pass llvm#69138. Another option would be to make assignCustomValue take a MutableArrayRef<CCValAssign>.
4vtomat
reviewed
Oct 22, 2023
| } | ||
|
|
||
| if (Thunk) { | ||
| *Thunk = [=]() { |
There was a problem hiding this comment.
One minor comment is that since Thunk is the same as the logic after the if condition, can we fold them and replace with the same lambda call?
for example,
auto assignFunc = [=]() {
assignValueToReg(NewRegs[0], VALo.getLocReg(), VALo);
if (VAHi.isRegLoc())
assignValueToReg(NewRegs[1], VAHi.getLocReg(), VAHi);
}
if (Thunk) {
*Thunk = assignFunc;
return 1;
}
assignFunc();
return 1;
something like that~
|
Do we also want to add test cases for using |
Like |
Oh I missed the file since it's folded lol~ |
topperc
force-pushed
the
pr/gisel-fp-call
branch
from
940719d to
c504d92
Compare
preames
approved these changes
Oct 25, 2023
| @@ -94,11 +94,60 @@ struct RISCVOutgoingValueHandler : public CallLowering::OutgoingValueHandler { | |||
|
|
|||
| void assignValueToReg(Register ValVReg, Register PhysReg, | |||
| CCValAssign VA) override { | |||
| // If we're passing an f32 value into an i64, anyextend before copying. | |||
| if (VA.getLocVT() == MVT::i64 && VA.getValVT() == MVT::f32) | |||
There was a problem hiding this comment.
It looks like maybe this bit of code could be handled by setting LocInfo to CCValAssign::AExt, and letting it fallthrough instead? Not sure about what else that means though, so take with bucket of salt.
There was a problem hiding this comment.
I think so too, but it would require moving some code in SelectionDAG if we fix the original VA. Or we need to modify the VA and pass the modified version on.
…gisters. This does not include passing values on the stack. Test cases copied from SelectionDAG and converted to check MIR output. Test cases that require stack were removed.
This builds on the register support to add stack support.
topperc
force-pushed
the
pr/gisel-fp-call
branch
from
c504d92 to
332df07
Compare
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Oct 26, 2023
This includes support for using GPRs, FPRs, and stack.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This includes support for using GPRs, FPRs, and stack. Commits are split between registers and memory.