[GISEL][RISCV] Legalize llvm.vacopy intrinsic #73066
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
|
@llvm/pr-subscribers-llvm-support @llvm/pr-subscribers-llvm-globalisel Author: Michael Maitland (michaelmaitland) ChangesThis change is stacked on:
This change is best reviewed by giving review on e4065d0e309b769b2659b36516394111c53d9951, or by leaving comments on the changes this PR is stacked on. The test case in this PR demonstrates that variadic code is fully functional in comparison to SDAG (the test is adapted from Patch is 230.61 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/73066.diff 28 Files Affected:
diff --git a/llvm/docs/GlobalISel/GenericOpcode.rst b/llvm/docs/GlobalISel/GenericOpcode.rst
index 6c42ddcaff1eccf..7d71fa313108693 100644
--- a/llvm/docs/GlobalISel/GenericOpcode.rst
+++ b/llvm/docs/GlobalISel/GenericOpcode.rst
@@ -898,6 +898,17 @@ G_VAARG
I found no documentation for this instruction at the time of writing.
+G_VACOPY
+^^^^^^^^
+
+In a target-dependent way, it copies the source va_list element into the
+destination va_list element. This opcode is necessary because the copy may be
+arbitrarily complex.
+
+.. code-block:: none
+
+ G_VACOPY %2(p0), %3(p0)
+
Other Operations
----------------
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/LegalizerHelper.h b/llvm/include/llvm/CodeGen/GlobalISel/LegalizerHelper.h
index 86d3cb2bedb95b6..a2a343144e82976 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/LegalizerHelper.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/LegalizerHelper.h
@@ -430,6 +430,8 @@ class LegalizerHelper {
LegalizeResult lowerVectorReduction(MachineInstr &MI);
LegalizeResult lowerMemcpyInline(MachineInstr &MI);
LegalizeResult lowerMemCpyFamily(MachineInstr &MI, unsigned MaxLen = 0);
+ LegalizeResult lowerVAArg(MachineInstr &MI);
+ LegalizeResult lowerVACopy(MachineInstr &MI);
};
/// Helper function that creates a libcall to the given \p Name using the given
diff --git a/llvm/include/llvm/Support/TargetOpcodes.def b/llvm/include/llvm/Support/TargetOpcodes.def
index 941c6d5f8cad8ce..5c3da9e65c74060 100644
--- a/llvm/include/llvm/Support/TargetOpcodes.def
+++ b/llvm/include/llvm/Support/TargetOpcodes.def
@@ -454,9 +454,13 @@ HANDLE_TARGET_OPCODE(G_FCONSTANT)
/// Generic va_start instruction. Stores to its one pointer operand.
HANDLE_TARGET_OPCODE(G_VASTART)
-/// Generic va_start instruction. Stores to its one pointer operand.
+/// Generic va_arg instruction. Stores to its one pointer operand.
HANDLE_TARGET_OPCODE(G_VAARG)
+/// Generic va_copy instruction. Copies the source element into the destination
+/// element.
+HANDLE_TARGET_OPCODE(G_VACOPY)
+
// Generic sign extend
HANDLE_TARGET_OPCODE(G_SEXT)
HANDLE_TARGET_OPCODE(G_SEXT_INREG)
diff --git a/llvm/include/llvm/Target/GenericOpcodes.td b/llvm/include/llvm/Target/GenericOpcodes.td
index 9a9c09d3c20d612..3b26ab35fa509f2 100644
--- a/llvm/include/llvm/Target/GenericOpcodes.td
+++ b/llvm/include/llvm/Target/GenericOpcodes.td
@@ -155,6 +155,13 @@ def G_VASTART : GenericInstruction {
let mayStore = true;
}
+def G_VACOPY : GenericInstruction {
+ let OutOperandList = (outs);
+ let InOperandList = (ins type0:$dest, type0:$src);
+ let hasSideEffects = true;
+ let mayStore = true;
+}
+
def G_VAARG : GenericInstruction {
let OutOperandList = (outs type0:$val);
let InOperandList = (ins type1:$list, unknown:$align);
diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index 62450e4c43ff3e6..8f898c7d500da20 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -2075,6 +2075,12 @@ bool IRTranslator::translateKnownIntrinsic(const CallInst &CI, Intrinsic::ID ID,
ListSize, Align(1)));
return true;
}
+ case Intrinsic::vacopy: {
+ Register DstList = getOrCreateVReg(*CI.getArgOperand(0));
+ Register SrcList = getOrCreateVReg(*CI.getArgOperand(1));
+ MIRBuilder.buildInstr(TargetOpcode::G_VACOPY, {}, {DstList, SrcList});
+ return true;
+ }
case Intrinsic::dbg_value: {
// This form of DBG_VALUE is target-independent.
const DbgValueInst &DI = cast<DbgValueInst>(CI);
diff --git a/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp b/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
index dd5577d47f97764..1c254883c7bc6da 100644
--- a/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp
@@ -3780,6 +3780,10 @@ LegalizerHelper::lower(MachineInstr &MI, unsigned TypeIdx, LLT LowerHintTy) {
return lowerTRUNC(MI);
GISEL_VECREDUCE_CASES_NONSEQ
return lowerVectorReduction(MI);
+ case G_VAARG:
+ return lowerVAArg(MI);
+ case G_VACOPY:
+ return lowerVACopy(MI);
}
}
@@ -7865,6 +7869,100 @@ LegalizerHelper::lowerVectorReduction(MachineInstr &MI) {
return UnableToLegalize;
}
+static Type *getTypeForLLT(LLT Ty, LLVMContext &C);
+
+LegalizerHelper::LegalizeResult LegalizerHelper::lowerVAArg(MachineInstr &MI) {
+ Observer.changingInstr(MI);
+ MachineFunction &MF = *MI.getMF();
+ const DataLayout &DL = MIRBuilder.getDataLayout();
+ LLVMContext &Ctx = MF.getFunction().getContext();
+ Register ListPtr = MI.getOperand(1).getReg();
+ LLT PtrTy = MRI.getType(ListPtr);
+
+ // LstPtr is a pointer to the head of the list. Get the address
+ // of the head of the list.
+ Align PtrAlignment = Align(DL.getABITypeAlign(getTypeForLLT(PtrTy, Ctx)));
+ MachineMemOperand *PtrLoadMMO =
+ MF.getMachineMemOperand(MachinePointerInfo::getUnknownStack(MF),
+ MachineMemOperand::MOLoad, PtrTy, PtrAlignment);
+ Register HeadOfList = MRI.createGenericVirtualRegister(PtrTy);
+ Register VAList =
+ MIRBuilder.buildLoad(HeadOfList, ListPtr, *PtrLoadMMO).getReg(0);
+
+ const MaybeAlign MA(MI.getOperand(2).getImm());
+ LLT PtrTyAsScalarTy = LLT::scalar(PtrTy.getSizeInBits());
+ if (MA && *MA > TLI.getMinStackArgumentAlignment()) {
+ Register AlignAmt =
+ MIRBuilder.buildConstant(PtrTyAsScalarTy, MA->value() - 1).getReg(0);
+ Register AddDst = MRI.createGenericVirtualRegister(PtrTy);
+ MIRBuilder.buildPtrAdd(AddDst, HeadOfList, AlignAmt);
+ Register Mask =
+ MIRBuilder.buildConstant(PtrTyAsScalarTy, -(int64_t)MA->value())
+ .getReg(0);
+ Register AndDst = MRI.createGenericVirtualRegister(PtrTy);
+ VAList = MIRBuilder.buildPtrMask(AndDst, AddDst, Mask).getReg(0);
+ }
+
+ // Increment the pointer, VAList, to the next vaarg
+ // The list should be bumped by the size of element in the current head of
+ // list.
+ Register Dst = MI.getOperand(0).getReg();
+ LLT Ty = MRI.getType(Dst);
+ Register IncAmt =
+ MIRBuilder
+ .buildConstant(PtrTyAsScalarTy,
+ DL.getTypeAllocSize(getTypeForLLT(Ty, Ctx)))
+ .getReg(0);
+ Register Succ = MRI.createGenericVirtualRegister(PtrTy);
+ MIRBuilder.buildPtrAdd(Succ, VAList, IncAmt);
+
+ // Store the increment VAList to the legalized pointer
+ MachineMemOperand *StoreMMO =
+ MF.getMachineMemOperand(MachinePointerInfo::getUnknownStack(MF),
+ MachineMemOperand::MOStore, PtrTy, PtrAlignment);
+ MIRBuilder.buildStore(Succ, ListPtr, *StoreMMO);
+ // Load the actual argument out of the pointer VAList
+ Align EltAlignment = Align(DL.getABITypeAlign(getTypeForLLT(Ty, Ctx)));
+ MachineMemOperand *EltLoadMMO =
+ MF.getMachineMemOperand(MachinePointerInfo::getUnknownStack(MF),
+ MachineMemOperand::MOLoad, Ty, EltAlignment);
+ MIRBuilder.buildLoad(Dst, VAList, *EltLoadMMO);
+
+ Observer.changedInstr(MI);
+ Observer.erasingInstr(MI);
+ MI.eraseFromParent();
+ return Legalized;
+}
+
+LegalizerHelper::LegalizeResult LegalizerHelper::lowerVACopy(MachineInstr &MI) {
+ MachineFunction &MF = *MI.getMF();
+ const DataLayout &DL = MIRBuilder.getDataLayout();
+ LLVMContext &Ctx = MF.getFunction().getContext();
+
+ Register DstLst = MI.getOperand(0).getReg();
+ LLT PtrTy = MRI.getType(DstLst);
+
+ // Load the source va_list
+ Align Alignment = Align(DL.getABITypeAlign(getTypeForLLT(PtrTy, Ctx)));
+ MachineMemOperand *LoadMMO =
+ MF.getMachineMemOperand(MachinePointerInfo::getUnknownStack(MF),
+ MachineMemOperand::MOLoad, PtrTy, Alignment);
+ Register Tmp = MRI.createGenericVirtualRegister(PtrTy);
+ Register SrcLst = MI.getOperand(1).getReg();
+ MIRBuilder.buildLoad(Tmp, SrcLst, *LoadMMO);
+
+ // Store the result in the destination va_list
+ MachineMemOperand *StoreMMO =
+ MF.getMachineMemOperand(MachinePointerInfo::getUnknownStack(MF),
+ MachineMemOperand::MOStore, PtrTy, Alignment);
+ MIRBuilder.buildStore(DstLst, Tmp, *StoreMMO);
+
+ Observer.changedInstr(MI);
+ Observer.erasingInstr(MI);
+ MI.eraseFromParent();
+ return Legalized;
+}
+
static bool shouldLowerMemFuncForSize(const MachineFunction &MF) {
// On Darwin, -Os means optimize for size without hurting performance, so
// only really optimize for size when -Oz (MinSize) is used.
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
index 1aba8a8f52e96fc..f0aa0417a03164b 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
@@ -423,18 +423,76 @@ bool RISCVCallLowering::lowerReturn(MachineIRBuilder &MIRBuilder,
return true;
}
+/// If there are varargs that were passed in a0-a7, the data in those registers
+/// must be copied to the varargs save area on the stack.
+void RISCVCallLowering::saveVarArgRegisters(
+ MachineIRBuilder &MIRBuilder, CallLowering::IncomingValueHandler &Handler,
+ IncomingValueAssigner &Assigner, CCState &CCInfo) const {
+ MachineFunction &MF = MIRBuilder.getMF();
+ const RISCVSubtarget &Subtarget = MF.getSubtarget<RISCVSubtarget>();
+ unsigned XLenInBytes = Subtarget.getXLen() / 8;
+ ArrayRef<MCPhysReg> ArgRegs({RISCV::X10, RISCV::X11, RISCV::X12, RISCV::X13,
+ RISCV::X14, RISCV::X15, RISCV::X16, RISCV::X17});
+ unsigned Idx = CCInfo.getFirstUnallocated(ArgRegs);
+
+ // Offset of the first variable argument from stack pointer, and size of
+ // the vararg save area. For now, the varargs save area is either zero or
+ // large enough to hold a0-a7.
+ int VaArgOffset, VarArgsSaveSize;
+ // If all registers are allocated, then all varargs must be passed on the
+ // stack and we don't need to save any argregs.
+ if (ArgRegs.size() == Idx) {
+ VaArgOffset = Assigner.StackSize;
+ VarArgsSaveSize = 0;
+ } else {
+ VarArgsSaveSize = XLenInBytes * (ArgRegs.size() - Idx);
+ VaArgOffset = -VarArgsSaveSize;
+ }
+
+ // Record the frame index of the first variable argument which is a value
+ // necessary to G_VASTART.
+ MachineFrameInfo &MFI = MF.getFrameInfo();
+ int FI = MFI.CreateFixedObject(XLenInBytes, VaArgOffset, true);
+ RISCVMachineFunctionInfo *RVFI = MF.getInfo<RISCVMachineFunctionInfo>();
+ RVFI->setVarArgsFrameIndex(FI);
+
+ // If saving an odd number of registers then create an extra stack slot to
+ // ensure that the frame pointer is 2*XLEN-aligned, which in turn ensures
+ // offsets to even-numbered registered remain 2*XLEN-aligned.
+ if (Idx % 2) {
+ MFI.CreateFixedObject(XLenInBytes, VaArgOffset - (int)XLenInBytes, true);
+ VarArgsSaveSize += XLenInBytes;
+ }
+ RVFI->setVarArgsSaveSize(VarArgsSaveSize);
+
+ // Copy the integer registers that may have been used for passing varargs
+ // to the vararg save area.
+ const LLT p0 = LLT::pointer(0, Subtarget.getXLen());
+ const LLT sXLen = LLT::scalar(Subtarget.getXLen());
+ const MVT XLenMVT = MVT::getIntegerVT(Subtarget.getXLen());
+ MachineRegisterInfo &MRI = MF.getRegInfo();
+ for (unsigned I = Idx; I < ArgRegs.size(); ++I, VaArgOffset += XLenInBytes) {
+ const Register VReg = MRI.createGenericVirtualRegister(sXLen);
+ Handler.assignValueToReg(
+ VReg, ArgRegs[I],
+ CCValAssign::getReg(I + MF.getFunction().getNumOperands(), XLenMVT,
+ ArgRegs[I], XLenMVT, CCValAssign::Full));
+ FI = MFI.CreateFixedObject(XLenInBytes, VaArgOffset, true);
+ auto FIN = MIRBuilder.buildFrameIndex(p0, FI);
+ auto MPO = MachinePointerInfo::getFixedStack(MF, FI);
+ MIRBuilder.buildStore(VReg, FIN, MPO, inferAlignFromPtrInfo(MF, MPO));
+ }
+}
+
bool RISCVCallLowering::lowerFormalArguments(MachineIRBuilder &MIRBuilder,
const Function &F,
ArrayRef<ArrayRef<Register>> VRegs,
FunctionLoweringInfo &FLI) const {
- // Early exit if there are no arguments.
- if (F.arg_empty())
+ // Early exit if there are no arguments. varargs are not part of F.args() but
+ // must be lowered.
+ if (F.arg_empty() && !F.isVarArg())
return true;
- // TODO: Support vararg functions.
- if (F.isVarArg())
- return false;
-
const RISCVSubtarget &Subtarget =
MIRBuilder.getMF().getSubtarget<RISCVSubtarget>();
for (auto &Arg : F.args()) {
@@ -467,8 +525,16 @@ bool RISCVCallLowering::lowerFormalArguments(MachineIRBuilder &MIRBuilder,
/*IsRet=*/false);
RISCVFormalArgHandler Handler(MIRBuilder, MF.getRegInfo());
- return determineAndHandleAssignments(Handler, Assigner, SplitArgInfos,
- MIRBuilder, CC, F.isVarArg());
+ SmallVector<CCValAssign, 16> ArgLocs;
+ CCState CCInfo(CC, F.isVarArg(), MIRBuilder.getMF(), ArgLocs, F.getContext());
+ if (!determineAssignments(Assigner, SplitArgInfos, CCInfo) ||
+ !handleAssignments(Handler, SplitArgInfos, CCInfo, ArgLocs, MIRBuilder))
+ return false;
+
+ if (F.isVarArg())
+ saveVarArgRegisters(MIRBuilder, Handler, Assigner, CCInfo);
+
+ return true;
}
bool RISCVCallLowering::lowerCall(MachineIRBuilder &MIRBuilder,
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.h b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.h
index d80a666f3489475..abe704b4a645189 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.h
+++ b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.h
@@ -42,6 +42,11 @@ class RISCVCallLowering : public CallLowering {
private:
bool lowerReturnVal(MachineIRBuilder &MIRBuilder, const Value *Val,
ArrayRef<Register> VRegs, MachineInstrBuilder &Ret) const;
+
+ void saveVarArgRegisters(MachineIRBuilder &MIRBuilder,
+ CallLowering::IncomingValueHandler &Handler,
+ IncomingValueAssigner &Assigner,
+ CCState &CCInfo) const;
};
} // end namespace llvm
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
index 3c72269d1e00c2f..1808686bd135da2 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp
@@ -48,6 +48,9 @@ class RISCVInstructionSelector : public InstructionSelector {
const TargetRegisterClass *
getRegClassForTypeOnBank(LLT Ty, const RegisterBank &RB) const;
+ bool isRegInGprb(Register Reg, MachineRegisterInfo &MRI) const;
+ bool isRegInFprb(Register Reg, MachineRegisterInfo &MRI) const;
+
// tblgen-erated 'select' implementation, used as the initial selector for
// the patterns that don't require complex C++.
bool selectImpl(MachineInstr &I, CodeGenCoverage &CoverageInfo) const;
@@ -62,6 +65,8 @@ class RISCVInstructionSelector : public InstructionSelector {
// Custom selection methods
bool selectCopy(MachineInstr &MI, MachineRegisterInfo &MRI) const;
+ bool selectImplicitDef(MachineInstr &MI, MachineIRBuilder &MIB,
+ MachineRegisterInfo &MRI) const;
bool materializeImm(Register Reg, int64_t Imm, MachineIRBuilder &MIB) const;
bool selectGlobalValue(MachineInstr &MI, MachineIRBuilder &MIB,
MachineRegisterInfo &MRI) const;
@@ -72,6 +77,10 @@ class RISCVInstructionSelector : public InstructionSelector {
MachineRegisterInfo &MRI) const;
bool selectFPCompare(MachineInstr &MI, MachineIRBuilder &MIB,
MachineRegisterInfo &MRI) const;
+ bool selectMergeValues(MachineInstr &MI, MachineIRBuilder &MIB,
+ MachineRegisterInfo &MRI) const;
+ bool selectUnmergeValues(MachineInstr &MI, MachineIRBuilder &MIB,
+ MachineRegisterInfo &MRI) const;
ComplexRendererFns selectShiftMask(MachineOperand &Root) const;
ComplexRendererFns selectAddrRegImm(MachineOperand &Root) const;
@@ -564,11 +573,55 @@ bool RISCVInstructionSelector::select(MachineInstr &MI) {
return selectSelect(MI, MIB, MRI);
case TargetOpcode::G_FCMP:
return selectFPCompare(MI, MIB, MRI);
+ case TargetOpcode::G_IMPLICIT_DEF:
+ return selectImplicitDef(MI, MIB, MRI);
+ case TargetOpcode::G_MERGE_VALUES:
+ return selectMergeValues(MI, MIB, MRI);
+ case TargetOpcode::G_UNMERGE_VALUES:
+ return selectUnmergeValues(MI, MIB, MRI);
default:
return false;
}
}
+bool RISCVInstructionSelector::selectMergeValues(
+ MachineInstr &MI, MachineIRBuilder &MIB, MachineRegisterInfo &MRI) const {
+ assert(MI.getOpcode() == TargetOpcode::G_MERGE_VALUES);
+
+ // Build a F64 Pair from operands
+ if (MI.getNumOperands() != 3)
+ return false;
+ Register Dst = MI.getOperand(0).getReg();
+ Register Lo = MI.getOperand(1).getReg();
+ Register Hi = MI.getOperand(2).getReg();
+ if (!isRegInFprb(Dst, MRI) || !(isRegInGprb(Lo, MRI) && isRegInGprb(Hi, MRI)))
+ return false;
+ MachineInstr *Result =
+ MIB.buildInstr(RISCV::BuildPairF64Pseudo, {Dst}, {Lo, Hi});
+
+ MI.eraseFromParent();
+ return constrainSelectedInstRegOperands(*Result, TII, TRI, RBI);
+}
+
+bool RISCVInstructionSelector::selectUnmergeValues(
+ MachineInstr &MI, MachineIRBuilder &MIB, MachineRegisterInfo &MRI) const {
+ assert(MI.getOpcode() == TargetOpcode::G_UNMERGE_VALUES);
+
+ // Split F64 Src into two s32 parts
+ if (MI.getNumOperands() != 3)
+ return false;
+ Register Src = MI.getOperand(2).getReg();
+ Register Lo = MI.getOperand(0).getReg();
+ Register Hi = MI.getOperand(1).getReg();
+ if (!isRegInFprb(Src, MRI) ||
+ !(isRegInGprb(Lo, MRI) && isRegInGprb(Hi, MRI)))
+ return false;
+ MachineInstr *Result = MIB.buildInstr(RISCV::SplitF64Pseudo, {Lo, Hi}, {Src});
+
+ MI.eraseFromParent();
+ return constrainSelectedInstRegOperands(*Result, TII, TRI, RBI);
+}
+
bool RISCVInstructionSelector::replacePtrWithInt(MachineOperand &Op,
MachineIRBuilder &MIB,
MachineRegisterInfo &MRI) {
@@ -652,6 +705,16 @@ const TargetRegisterClass *RISCVInstructionSelector::getRegClassForTypeOnBank(
return nullptr;
}
+bool RISCVInstructionSelector::isRegInGprb(Register Reg,
+ MachineRegisterInfo &MRI) const {
+ return RBI.getRegBank(Reg, MRI, TRI)->getID() == RISCV::GPRBRegBankID;
+}
+
+bool RISCVInstructionSelector::isRegInFprb(Register Reg,
+ MachineRegisterInfo &MRI) const {
+ return RBI.getRegBank(Reg, MRI, TRI)->getID() == RISCV::FPRBRegBankID;
+}
+
bool RISCVInstructionSelector::selectCopy(MachineInstr &MI,
MachineRegisterInfo &MRI) const {
Register DstReg = MI.getOperand(0).getReg();
@@ -677,6 +740,25 @@ bool RISCVInstructionSelector::selectCopy(MachineInstr &MI,
return true;
}
+bool RISCVInstructionSelector::selectImplicitDef(
+ MachineInstr &MI, MachineIRBuilder &MIB, MachineRegisterInfo &MRI) const {
+ assert(MI.getOpcode() == TargetOpcode::G_IMPLICIT_DEF);
+
+ const Register DstReg = MI.getOperand(0).getReg();
+ const TargetRegisterClass *DstRC = getRegClassForTypeOnBank(
+ MRI.getType(DstReg), *RBI.getRegBank(DstReg, MRI, TRI));
+
+ assert(DstRC &&
+ "Register class not available for LLT, register bank combination");
+
+ if (!RBI.constrainGenericRegister(DstReg, *DstRC, MRI)) {
+ LLVM_DEBUG(dbgs() << "Failed to constrain " << TII.getName(MI.getOpcode())
+ << " operand\n");
+ }
+ MI.setDesc(TII.get(TargetOpcode::IMPLICIT_DEF));
+ return true;
+}
+
bool RISCVInstructionSelector::materializeImm(Register DstReg, int64_t Imm,
MachineIRBuilder &MIB) const {
MachineRegisterInfo &MRI = *MIB.getMRI();
diff --git a/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp b/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
index 9eb5812e024b915..e1b4cdf486577cf 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp
@@ -11,6 +11,7 @@
//===----------------------------------------------------------------------===//
#include "RISCVL...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Can we just handle Intrinsic::vacopy in RISCVLegalizerInfo::legalizeIntrinsic like AArch64 does? How did this patch not break AArch6 testing? |
michaelmaitland
force-pushed
the
lower-variadic
branch
from
e4065d0
to
80974e9
Compare
It isn't clear to me why we have G_VASTART and G_VAARG, but would use legalizeIntrinsic for vacopy. It seems fitting that all three should get opcodes or all three should use legalizeIntrinsic. We have all three ISD nodes in SDAG. We have I've gone ahead and made the change and based on reviewer input we can decide which approach to take. |
G_VAARG exists because va_arg is an instruction in IR so it is not an intrinsic. The commit that added G_VASTART said it need to be an instrction because it carries a memory operand. |
|
I think every generic intrinsic and instruction should have a defined G_* opcode and not route through G_INTRINSIC*. G_INTRINSIC* is for target intrinsics |
topperc
reviewed
Nov 29, 2023
topperc
reviewed
Nov 29, 2023
| // Load the source va_list | ||
| Align Alignment = Align(DL.getABITypeAlign(getTypeForLLT(PtrTy, Ctx))); | ||
| MachineMemOperand *LoadMMO = | ||
| MF.getMachineMemOperand(MachinePointerInfo::getUnknownStack(MF), |
There was a problem hiding this comment.
Do we know for sure it's stack? That's what it should be in a well behaved usage, but I don't we know for sure here.
I see the SelectionDAG's VACOPY retains the Value* from IR so it can look it up. Maybe we can do the same when we add G_VACOPY.
There was a problem hiding this comment.
It doesn't look like the IR lowering processes preserves the Value * from the IR when lowering to the intrinsic. I agree we should do this when we add G_VACOPY and let it slide for now. What do you think?
There was a problem hiding this comment.
For now we need to use a default constructed MachinePointerInfo. We can fix it properly when we add G_VACOPY.
I agree. Are you okay with going through G_INTRINSIC* for this patch and then we can move all targets to G_VACOPY in a future patch? |
topperc
reviewed
Nov 30, 2023
| LLT PtrTy = MRI.getType(DstLst); | ||
|
|
||
| // Load the source va_list | ||
| Align Alignment = Align(DL.getABITypeAlign(getTypeForLLT(PtrTy, Ctx))); |
There was a problem hiding this comment.
The call to the Align constructor is unnecessary
topperc
approved these changes
Nov 30, 2023
arsenm
reviewed
Nov 30, 2023
| if (!RBI.constrainGenericRegister(DstReg, *DstRC, MRI)) { | ||
| LLVM_DEBUG(dbgs() << "Failed to constrain " << TII.getName(MI.getOpcode()) | ||
| << " operand\n"); | ||
| } |
There was a problem hiding this comment.
This should return false and hit the fallback, the mutated instruction later will fail the verifier
arsenm
reviewed
Nov 30, 2023
| @@ -86,6 +88,10 @@ RISCVLegalizerInfo::RISCVLegalizerInfo(const RISCVSubtarget &ST) { | |||
| unsigned BigTyIdx = Op == G_MERGE_VALUES ? 0 : 1; | |||
| unsigned LitTyIdx = Op == G_MERGE_VALUES ? 1 : 0; | |||
| getActionDefinitionsBuilder(Op) | |||
| .legalIf([=, &ST](const LegalityQuery &Query) -> bool { | |||
| return ST.hasStdExtD() && typeIs(LitTyIdx, s32)(Query) && | |||
There was a problem hiding this comment.
Should add the legalIf for hasStdExtD, not have the check inside the callback
arsenm
reviewed
Nov 30, 2023
| DL.getTypeAllocSize(getTypeForLLT(Ty, Ctx))) | ||
| .getReg(0); | ||
| Register Succ = MRI.createGenericVirtualRegister(PtrTy); | ||
| MIRBuilder.buildPtrAdd(Succ, VAList, IncAmt); |
There was a problem hiding this comment.
auto Succ = MIRBuilder.buildPtrAdd(PtrTy ...
| MachineMemOperand::MOLoad, Ty, EltAlignment); | ||
| MIRBuilder.buildLoad(Dst, VAList, *EltLoadMMO); | ||
|
|
||
| Observer.changedInstr(MI); |
There was a problem hiding this comment.
You didn't change it, just erased it
michaelmaitland
force-pushed
the
lower-variadic
branch
from
a564253
to
283d9a1
Compare
In the future, we can consider adding a G_VACOPY opcode instead of going through the GIntrinsic for all targets. We do the approach in this patch because that is what other targets do today.
…move incorrect observer call
michaelmaitland
changed the title
michaelmaitland
force-pushed
the
lower-variadic
branch
from
33c68b0
to
04d3975
Compare
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.
In the future, we can consider adding a G_VACOPY opcode instead of going
through the GIntrinsic for all targets. We do the approach in this patch because
that is what other targets do today.