[RISCV] Share ArgGPRs array between SelectionDAG and GISel.

[topperc]

This will allow us to isolate the EABI from D70401 to this new function.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

This will allow us to isolate the EABI from D70401 to this new function.

---

Full diff: https://github.com/llvm/llvm-project/pull/74152.diff

3 Files Affected:

• (modified) llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp (+1-5)
• (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+13-5)
• (modified) llvm/lib/Target/RISCV/RISCVISelLowering.h (+3)

diff --git a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
index bd602635dd5f704..5e8f22bc7735f3b 100644
--- a/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
+++ b/llvm/lib/Target/RISCV/GISel/RISCVCallLowering.cpp
@@ -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(
@@ -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
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 241bc96766f29b2..606d612ac993158 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -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
@@ -16872,6 +16868,15 @@ 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,
@@ -16879,6 +16884,7 @@ static bool CC_RISCVAssign2XLen(unsigned XLen, CCState &State, CCValAssign VA1,
                                 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,
@@ -16999,6 +17005,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
@@ -17684,7 +17692,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();
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.h b/llvm/lib/Target/RISCV/RISCVISelLowering.h
index 486efeb8339ab0b..ae798cc47bf8333 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.h
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.h
@@ -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();
+
 } // end namespace RISCV
 
 namespace RISCVVIntrinsicsTable {

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[wangpc-pp]

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

[wangpc-pp]

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

[topperc]

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

[topperc]

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

Agreed. We need the EABI patch to land.

[wangpc-pp]

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.

[topperc]

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);
}

[wangpc-pp]

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?

[topperc]

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.

[wangpc-pp]

LGTM.

[michaelmaitland]

LGTM