[RISCV] Refactory the existing CC_RISCV32 function to conform to the CCAssignFn type

[xiangzhai]

Summary

Hi LLVM developers,

As Alex suggested, I refactory the existing CC_RISCV32 function to conform to the CCAssignFn type for D41653 Pass a real CCAssignFn to the ValueHandler, please review my patch, thanks a lot!

Regards,
Leslie Zhai

[asb]

Thanks for the patch Leslie. My main concern with this approach is it will fail to work when VarArg support is added (D40805). Conforming to the vararg ABI requires 1) knowing which arguments are fixed vs non-fixed (I currently just have an extra parameter for this in CC_RISCV, Mips handles this with MipsCCState), 2) knowing the original type of an argument. Both issues could be addressed by introducing a RISCVCCState (where I'd probably keep a Datalayout and a SubtargetInfo reference), but looking at GlobalISel that wouldn't actually help without further changes - CallLowering::handleAssignments creates a CCState directly, meaning you'd need to make further changes to support a custom CCState (as used in Hexagon, SystemZ, Mips and other backends).

I think some more GlobalISel design work needs to be done here really. It looks like splitting a smaller assignArgs function out of handleAssignments and making it virtual would help, but still wouldn't provide complete drop-in compatibility with custom CCState subclasses.

[xiangzhai]

Hi Alex,

Thanks for your review!

My main concern with this approach is it will fail to work when VarArg support is added (D40805).

I will apply the patch of D40805 and read carefully about it.

meaning you'd need to make further changes to support a custom CCState (as used in Hexagon, SystemZ, Mips and other backends)

I will learn from other Targets to support a custom RISCVCCState.

I think some more GlobalISel design work needs to be done here really.

My sincere thanks will goto the developers who taught me a lot :) Thanks for your leading!

Regards,
Leslie Zhai

[xiangzhai]

Hi LLVM developers,

Thanks for your code view, I can rebase my patch based on D40805 .

1. Consider about IsFixed and OrigTy, but assert as firstly:

assert(IsFixed && "IsFixed support not yet implemented");
assert(OrigTy && "OrigTy support not yet implemented");

The implementation of RISCVCCState will be in the next patch, please give me some suggestion about custom RISCVCCState inherited from CCState, I think, at least, it needs to cover IsFixed and OrigTy.

2. Remove parameter DL for CC_RISCV, because it is able to use State.getMachineFunction().getDataLayout() through parameter State.

Please review my patch again, thanks a lot!

Regards,
Leslie Zhai

[asb]

Hi Leslie. Obviously we need to resolve the CCState issue before merging this, as it would regress codegen in its current state. Good observation that we can get the DataLayout via CCState. Given that, why not get rid of the various wrapper functions and just determine XLen and XLenVT from the DataLayout?

[xiangzhai]

Hi Alex,

Thanks for your leading!

why not get rid of the various wrapper functions and just determine XLen and XLenVT from the DataLayout?

Good idea!

Obviously we need to resolve the CCState issue before merging this.

Sorry for my Chinese English :) is there some patch related to custom <Target>CCState, or someone began to implement RISCVCCState? if not, I will try to implement it.

Regards,
Leslie Zhai

lib/Target/RISCV/RISCVISelLowering.h

 // selection DAG.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_RISCV_RISCVISELLOWERING_H
 #define LLVM_LIB_TARGET_RISCV_RISCVISELLOWERING_H
 
 #include "RISCV.h"
+#include "llvm/CodeGen/CallingConvLower.h"
 #include "llvm/CodeGen/SelectionDAG.h"
 #include "llvm/CodeGen/TargetLowering.h"
 
 namespace llvm {
 class RISCVSubtarget;
 namespace RISCVISD {
 enum NodeType : unsigned {
   FIRST_NUMBER = ISD::BUILTIN_OP_END,
public:
   std::pair<unsigned, const TargetRegisterClass *>
   getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
                                StringRef Constraint, MVT VT) const override;
 
   MachineBasicBlock *
   EmitInstrWithCustomInserter(MachineInstr &MI,
                               MachineBasicBlock *BB) const override;
 
+  CCAssignFn *CCAssignFnForCall(const DataLayout &DL, bool IsFixed = false,
+                                Type *OrigTy = nullptr) const;
+  CCAssignFn *CCAssignFnForReturn(const DataLayout &DL, bool IsFixed = false,
+                                  Type *OrigTy = nullptr) const;
+
 private:
   void analyzeInputArgs(MachineFunction &MF, CCState &CCInfo,
                         const SmallVectorImpl<ISD::InputArg> &Ins,
                         bool IsRet) const;
   void analyzeOutputArgs(MachineFunction &MF, CCState &CCInfo,
                          const SmallVectorImpl<ISD::OutputArg> &Outs,
                          bool IsRet, CallLoweringInfo *CLI) const;
   // Lower incoming arguments, copy physregs into vregs

lib/Target/RISCV/RISCVISelLowering.cpp

if (unsigned Reg = State.AllocateReg(ArgGPRs)) {
     State.addLoc(CCValAssign::getMem(
         ValNo2, ValVT2, State.AllocateStack(XLenInBytes, XLenInBytes), LocVT2,
         CCValAssign::Full));
   }
 
   return false;
 }
 
-// Implements the RISC-V calling convention. Returns true upon failure.
-static bool CC_RISCV(const DataLayout &DL, unsigned ValNo, MVT ValVT, MVT LocVT,
-                     CCValAssign::LocInfo LocInfo, ISD::ArgFlagsTy ArgFlags,
-                     CCState &State, bool IsFixed, bool IsRet, Type *OrigTy) {
-  unsigned XLen = DL.getLargestLegalIntTypeSizeInBits();
-  assert(XLen == 32 || XLen == 64);
-  MVT XLenVT = XLen == 32 ? MVT::i32 : MVT::i64;
-  assert(ValVT == XLenVT && "Unexpected ValVT");
-  assert(LocVT == XLenVT && "Unexpected LocVT");
-
-  // Any return value split in to more than two values can't be returned
-  // directly.
-  if (IsRet && ValNo > 1)
-    return true;
+static bool CC_RISCVFn(unsigned XLen, MVT XLenVT, unsigned ValNo, MVT ValVT,
+                       MVT LocVT, CCValAssign::LocInfo LocInfo,
+                       ISD::ArgFlagsTy ArgFlags, CCState &State,
+                       bool IsFixed = false, Type *OrigTy = nullptr) {
+  const DataLayout &DL = State.getMachineFunction().getDataLayout();
 
   // 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
   // be used regardless of whether the original argument was split during
   // legalisation or not. The argument will not be passed by registers if the
   // original type is larger than 2*XLEN, so the register alignment rule does
   // not apply.
if (Reg) {
     State.addLoc(CCValAssign::getReg(ValNo, ValVT, Reg, LocVT, LocInfo));
   } else {
     State.addLoc(
         CCValAssign::getMem(ValNo, ValVT, StackOffset, LocVT, LocInfo));
   }
   return false;
 }
 
+static bool CC_RISCV32Fn(unsigned ValNo, MVT ValVT, MVT LocVT,
+                         CCValAssign::LocInfo LocInfo, ISD::ArgFlagsTy ArgFlags,
+                         CCState &State) {
+  unsigned XLen = 32;
+  MVT XLenVT = MVT::i32;
+
+  return CC_RISCVFn(XLen, XLenVT, ValNo, ValVT, LocVT, LocInfo, ArgFlags,
+                    State);
+}
+
+static bool CC_RISCV64Fn(unsigned ValNo, MVT ValVT, MVT LocVT,
+                         CCValAssign::LocInfo LocInfo, ISD::ArgFlagsTy ArgFlags,
+                         CCState &State) {
+  unsigned XLen = 64;
+  MVT XLenVT = MVT::i64;
+
+  return CC_RISCVFn(XLen, XLenVT, ValNo, ValVT, LocVT, LocInfo, ArgFlags,
+                    State);
+}
+
+static bool RetCC_RISCV32Fn(unsigned ValNo, MVT ValVT, MVT LocVT,
+                          CCValAssign::LocInfo LocInfo,
+                          ISD::ArgFlagsTy ArgFlags, CCState &State) {
+  unsigned XLen = 32;
+  MVT XLenVT = MVT::i32;
+
+  if (ValNo > 1)
+    return true;
+
+  return CC_RISCVFn(XLen, XLenVT, ValNo, ValVT, LocVT, LocInfo, ArgFlags,
+                    State);
+}
+
+static bool RetCC_RISCV64Fn(unsigned ValNo, MVT ValVT, MVT LocVT,
+                          CCValAssign::LocInfo LocInfo,
+                          ISD::ArgFlagsTy ArgFlags, CCState &State) {
+  unsigned XLen = 64;
+  MVT XLenVT = MVT::i64;
+
+  if (ValNo > 1)
+    return true;
+
+  return CC_RISCVFn(XLen, XLenVT, ValNo, ValVT, LocVT, LocInfo, ArgFlags,
+                    State);
+}
+
+CCAssignFn *RISCVTargetLowering::CCAssignFnForCall(const DataLayout &DL,
+                                                   bool IsFixed,
+                                                   Type *OrigTy) const {
+  assert(IsFixed && "IsFixed support not yet implemented");
+  assert(OrigTy && "OrigTy support not yet implemented");
+  if (DL.getLargestLegalIntTypeSizeInBits() == 32)
+    return CC_RISCV32Fn;
+
+  return CC_RISCV64Fn;
+}
+
+CCAssignFn *RISCVTargetLowering::CCAssignFnForReturn(const DataLayout &DL,
+                                                     bool IsFixed,
+                                                     Type *OrigTy) const {
+  assert(IsFixed && "IsFixed support not yet implemented");
+  assert(OrigTy && "OrigTy support not yet implemented");
+  if (DL.getLargestLegalIntTypeSizeInBits() == 32)
+    return RetCC_RISCV32Fn;
+
+  return RetCC_RISCV64Fn;
+}
+
+// Implements the RISC-V calling convention. Returns true upon failure.
+static bool CC_RISCV(unsigned ValNo, MVT ValVT, MVT LocVT,
+                     CCValAssign::LocInfo LocInfo, ISD::ArgFlagsTy ArgFlags,
+                     CCState &State, bool IsFixed, bool IsRet, Type *OrigTy) {
+  const DataLayout &DL = State.getMachineFunction().getDataLayout();
+  unsigned XLen = DL.getLargestLegalIntTypeSizeInBits();
+  assert(XLen == 32 || XLen == 64);
+  MVT XLenVT = XLen == 32 ? MVT::i32 : MVT::i64;
+  assert(ValVT == XLenVT && "Unexpected ValVT");
+  assert(LocVT == XLenVT && "Unexpected LocVT");
+
+  // Any return value split in to more than two values can't be returned
+  // directly.
+  if (IsRet && ValNo > 1)
+    return true;
+
+  return CC_RISCVFn(XLen, XLenVT, ValNo, ValVT, LocVT, LocInfo, ArgFlags, State,
+                    IsFixed, OrigTy);
+}
+
 void RISCVTargetLowering::analyzeInputArgs(
     MachineFunction &MF, CCState &CCInfo,
     const SmallVectorImpl<ISD::InputArg> &Ins, bool IsRet) const {
   unsigned NumArgs = Ins.size();
   FunctionType *FType = MF.getFunction().getFunctionType();
 
   for (unsigned i = 0; i != NumArgs; ++i) {
     MVT ArgVT = Ins[i].VT;
     ISD::ArgFlagsTy ArgFlags = Ins[i].Flags;
 
     Type *ArgTy = nullptr;
     if (IsRet)
       ArgTy = FType->getReturnType();
     else if (Ins[i].isOrigArg())
       ArgTy = FType->getParamType(Ins[i].getOrigArgIndex());
 
-    if (CC_RISCV(MF.getDataLayout(), i, ArgVT, ArgVT, CCValAssign::Full,
-                 ArgFlags, CCInfo, /*IsRet=*/true, IsRet, ArgTy)) {
+    if (CC_RISCV(i, ArgVT, ArgVT, CCValAssign::Full, ArgFlags, CCInfo,
+                 /*IsFixed=*/true, IsRet, ArgTy)) {
       DEBUG(dbgs() << "InputArg #" << i << " has unhandled type "
                    << EVT(ArgVT).getEVTString() << '\n');
       llvm_unreachable(nullptr);
     }
   }
 }
 
 void RISCVTargetLowering::analyzeOutputArgs(
     MachineFunction &MF, CCState &CCInfo,
     const SmallVectorImpl<ISD::OutputArg> &Outs, bool IsRet,
     CallLoweringInfo *CLI) const {
   unsigned NumArgs = Outs.size();
 
   for (unsigned i = 0; i != NumArgs; i++) {
     MVT ArgVT = Outs[i].VT;
     ISD::ArgFlagsTy ArgFlags = Outs[i].Flags;
     Type *OrigTy = CLI ? CLI->getArgs()[Outs[i].OrigArgIndex].Ty : nullptr;
 
-    if (CC_RISCV(MF.getDataLayout(), i, ArgVT, ArgVT, CCValAssign::Full,
-                 ArgFlags, CCInfo, Outs[i].IsFixed, IsRet, OrigTy)) {
+    if (CC_RISCV(i, ArgVT, ArgVT, CCValAssign::Full, ArgFlags, CCInfo,
+                 Outs[i].IsFixed, IsRet, OrigTy)) {
       DEBUG(dbgs() << "OutputArg #" << i << " has unhandled type "
                    << EVT(ArgVT).getEVTString() << "\n");
       llvm_unreachable(nullptr);
     }
   }
 }
 
 // The caller is responsible for loading the full value if the argument is

 bool RISCVTargetLowering::CanLowerReturn(
     CallingConv::ID CallConv, MachineFunction &MF, bool IsVarArg,
     const SmallVectorImpl<ISD::OutputArg> &Outs, LLVMContext &Context) const {
   SmallVector<CCValAssign, 16> RVLocs;
   CCState CCInfo(CallConv, IsVarArg, MF, RVLocs, Context);
   for (unsigned i = 0, e = Outs.size(); i != e; ++i) {
     MVT VT = Outs[i].VT;
     ISD::ArgFlagsTy ArgFlags = Outs[i].Flags;
-    if (CC_RISCV(MF.getDataLayout(), i, VT, VT, CCValAssign::Full, ArgFlags,
-                 CCInfo, /*IsFixed=*/true, /*IsRet=*/true, nullptr))
+    if (CC_RISCV(i, VT, VT, CCValAssign::Full, ArgFlags, CCInfo,
+                 /*IsFixed=*/true, /*IsRet=*/true, nullptr))
       return false;
   }
   return true;
 }
 
 SDValue
 RISCVTargetLowering::LowerReturn(SDValue Chain, CallingConv::ID CallConv,
                                  bool IsVarArg,