Skip to content

[RISCV] Share ArgGPRs array between SelectionDAG and GISel. #74152

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
Dec 4, 2023
Merged

[RISCV] Share ArgGPRs array between SelectionDAG and GISel. #74152

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
2964c7a
[RISCV] Share ArgGPRs array between SelectionDAG and GISel.
topperc Dec 1, 2023
8a5bb67
fixup! clang-format
topperc Dec 4, 2023
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
6 changes: 1 addition & 5 deletions llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -423,10 +423,6 @@ bool RISCVCallLowering::lowerReturn(MachineIRBuilder &MIRBuilder,
return true;
}

static const MCPhysReg ArgGPRs[] = {RISCV::X10, RISCV::X11, RISCV::X12,
RISCV::X13, RISCV::X14, RISCV::X15,
RISCV::X16, RISCV::X17};

/// 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(
Expand All @@ -435,7 +431,7 @@ void RISCVCallLowering::saveVarArgRegisters(
MachineFunction &MF = MIRBuilder.getMF();
const RISCVSubtarget &Subtarget = MF.getSubtarget<RISCVSubtarget>();
unsigned XLenInBytes = Subtarget.getXLen() / 8;
ArrayRef<MCPhysReg> ArgRegs(ArgGPRs);
ArrayRef<MCPhysReg> ArgRegs = RISCV::getArgGPRs();
unsigned Idx = CCInfo.getFirstUnallocated(ArgRegs);

// Offset of the first variable argument from stack pointer, and size of
Expand Down
17 changes: 12 additions & 5 deletions llvm/lib/Target/RISCV/RISCVISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16844,10 +16844,6 @@ void RISCVTargetLowering::AdjustInstrPostInstrSelection(MachineInstr &MI,
// register-size fields in the same situations they would be for fixed
// arguments.

static const MCPhysReg ArgGPRs[] = {
RISCV::X10, RISCV::X11, RISCV::X12, RISCV::X13,
RISCV::X14, RISCV::X15, RISCV::X16, RISCV::X17
};
static const MCPhysReg ArgFPR16s[] = {
RISCV::F10_H, RISCV::F11_H, RISCV::F12_H, RISCV::F13_H,
RISCV::F14_H, RISCV::F15_H, RISCV::F16_H, RISCV::F17_H
Expand All @@ -16872,13 +16868,22 @@ static const MCPhysReg ArgVRM4s[] = {RISCV::V8M4, RISCV::V12M4, RISCV::V16M4,
RISCV::V20M4};
static const MCPhysReg ArgVRM8s[] = {RISCV::V8M8, RISCV::V16M8};

ArrayRef<MCPhysReg> RISCV::getArgGPRs() {
static const MCPhysReg ArgGPRs[] = {RISCV::X10, RISCV::X11, RISCV::X12,
RISCV::X13, RISCV::X14, RISCV::X15,
RISCV::X16, RISCV::X17};

return ArrayRef(ArgGPRs);
}

// Pass a 2*XLEN argument that has been split into two XLEN values through
// registers or the stack as necessary.
static bool CC_RISCVAssign2XLen(unsigned XLen, CCState &State, CCValAssign VA1,
ISD::ArgFlagsTy ArgFlags1, unsigned ValNo2,
MVT ValVT2, MVT LocVT2,
ISD::ArgFlagsTy ArgFlags2) {
unsigned XLenInBytes = XLen / 8;
ArrayRef<MCPhysReg> ArgGPRs = RISCV::getArgGPRs();
if (Register Reg = State.AllocateReg(ArgGPRs)) {
// At least one half can be passed via register.
State.addLoc(CCValAssign::getReg(VA1.getValNo(), VA1.getValVT(), Reg,
Expand Down Expand Up @@ -16999,6 +17004,8 @@ bool RISCV::CC_RISCV(const DataLayout &DL, RISCVABI::ABI ABI, unsigned ValNo,
LocInfo = CCValAssign::BCvt;
}

ArrayRef<MCPhysReg> ArgGPRs = RISCV::getArgGPRs();

// If this is a variadic argument, the RISC-V calling convention requires
// that it is assigned an 'even' or 'aligned' register if it has 8-byte
// alignment (RV32) or 16-byte alignment (RV64). An aligned register should
Expand Down Expand Up @@ -17684,7 +17691,7 @@ SDValue RISCVTargetLowering::LowerFormalArguments(
MF.getInfo<RISCVMachineFunctionInfo>()->setIsVectorCall();

if (IsVarArg) {
ArrayRef<MCPhysReg> ArgRegs = ArrayRef(ArgGPRs);
ArrayRef<MCPhysReg> ArgRegs = RISCV::getArgGPRs();
unsigned Idx = CCInfo.getFirstUnallocated(ArgRegs);
const TargetRegisterClass *RC = &RISCV::GPRRegClass;
MachineFrameInfo &MFI = MF.getFrameInfo();
Expand Down
3 changes: 3 additions & 0 deletions llvm/lib/Target/RISCV/RISCVISelLowering.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,9 @@ bool CC_RISCV_FastCC(const DataLayout &DL, RISCVABI::ABI ABI, unsigned ValNo,
bool CC_RISCV_GHC(unsigned ValNo, MVT ValVT, MVT LocVT,
CCValAssign::LocInfo LocInfo, ISD::ArgFlagsTy ArgFlags,
CCState &State);

ArrayRef<MCPhysReg> getArgGPRs();
Copy link
Contributor

Choose a reason for hiding this comment

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

What about putting this function in RISCVRegisterInfo.h/cpp, no need to be a class method but a function in RISCV namespace.

Copy link
Contributor

Choose a reason for hiding this comment

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

And if we want to determine register list via ABI, we may need ABI parameter?

Copy link
Collaborator Author

@topperc topperc Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What about putting this function in RISCVRegisterInfo.h/cpp, no need to be a class method but a function in RISCV namespace.

It's not a class method in this patch.

Shouldn't it stay with other calling convention functions that are the primary users? And we should keep it with other register lists that are already in RISCVISelLowering.cpp

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

And if we want to determine register list via ABI, we may need ABI parameter?

Agreed. We need the EABI patch to land.

Copy link
Contributor

@wangpc-pp wangpc-pp Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

What about putting this function in RISCVRegisterInfo.h/cpp, no need to be a class method but a function in RISCV namespace.

It's not a class method in this patch.

I meant if we gonna to move this function to RISCVRegisterInfo.h/cpp, it can be just a function in RISCV namespace.

Shouldn't it stay with other calling convention functions that are the primary users? And we should keep it with other register lists that are already in RISCVISelLowering.cpp

There are some other register lists that can be reused in both SelectionDAG ISel and GISel, I tend to move these lists to a more common place like RISCVRegisterInfo.h/cpp, which represents register informations I think.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GISel doesn't need the other lists like ArgFPR16s, , ArgFPR32s, ArgFPR64s, ArgVRs, etc. If we move ArgGPRs I think we should move those to, but I don't see any good reason to do that since GISel doesn't need them. We already exported CC_RISCV, CC_RISCV_FastCC, CC_RISCV_GHC functions from SelectionDAG so GISel could use them. Those are the primary users of these arrays.

Or alternatively, maybe I could add something like this that we could share with SelectionDAG and GISel?

unsigned RISCV::getVarArgsRegistersToSave(CCState &State) {
  unsigned Idx = State.getFirstUnallocated(ArgRegs);
  return (ArgRegs.size() - Idx);
}

Copy link
Contributor

@wangpc-pp wangpc-pp Dec 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

GISel doesn't need the other lists like ArgFPR16s, , ArgFPR32s, ArgFPR64s, ArgVRs, etc. If we move ArgGPRs I think we should move those to, but I don't see any good reason to do that since GISel doesn't need them. We already exported CC_RISCV, CC_RISCV_FastCC, CC_RISCV_GHC functions from SelectionDAG so GISel could use them. Those are the primary users of these arrays.

I didn't follow the process of GISel porting, is it because we haven't supported FP CC so we don't need these lists? Anyway, I won't block this PR if you think it's better to not move. :-)

Or alternatively, maybe I could add something like this that we could share with SelectionDAG and GISel?

unsigned RISCV::getVarArgsRegistersToSave(CCState &State) {
  unsigned Idx = State.getFirstUnallocated(ArgRegs);
  return (ArgRegs.size() - Idx);
}

This could be done. And, we can still see a lot of duplications in SelectionDAG and GISel, can these be shared too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

GISel doesn't need the other lists like ArgFPR16s, , ArgFPR32s, ArgFPR64s, ArgVRs, etc. If we move ArgGPRs I think we should move those to, but I don't see any good reason to do that since GISel doesn't need them. We already exported CC_RISCV, CC_RISCV_FastCC, CC_RISCV_GHC functions from SelectionDAG so GISel could use them. Those are the primary users of these arrays.

I didn't follow the process of GISel porting, is it because we haven't supported FP CC so we don't need these lists? Anyway, I won't block this PR if you think it's better to not move. :-)

The FP and vector register lists are only used by the CC_RISCV* functions. Those are called by both SelectionDAG and GISel. GISel only needs direct access to the GPR list due to the VarArg handling.

Or alternatively, maybe I could add something like this that we could share with SelectionDAG and GISel?

unsigned RISCV::getVarArgsRegistersToSave(CCState &State) {

unsigned Idx = State.getFirstUnallocated(ArgRegs);

return (ArgRegs.size() - Idx);

}

This could be done. And, we can still see a lot of duplications in SelectionDAG and GISel, can these be shared too?

Never mind, this doesn't work because we need the list for the save loop in the VarArg code.


} // end namespace RISCV

namespace RISCVVIntrinsicsTable {
Expand Down