[PATCH] D117318: [WIP][RISCV][GlobalISel] Add lowerReturn for calling conv

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.
Herald added subscribers: alextsao1999, eopXD.


================
Comment at: llvm/lib/CodeGen/TargetRegisterInfo.cpp:237-239
+    if ((!Ty.isValid() ||
+         (RC->isAllocatable() && isTypeLegalForClass(*RC, Ty))) &&
+        RC->contains(reg) && (!BestRC || BestRC->hasSubClass(RC)))
----------------
This needs its own patch and justification


================
Comment at: llvm/lib/Target/RISCV/RISCVCallLowering.cpp:80-84
+  splitToValueTypes(OrigRetInfo, SplitRetInfos, SplitEVTs, MF,
+                    [&](ArrayRef<Register> Regs, int SplitIdx) {
+                      MIRBuilder.buildUnmerge(Regs, VRegs[SplitIdx]);
+                    });
+
----------------
This looks like you're copying the broken form of splitToValueTypes from x86. I think trying to handle all of the argument marshalling in simple looking callbacks like this is unworkable. If you call the generic splitToValueTypes in CallLowering, it should try to handle everything for you


================
Comment at: llvm/lib/Target/RISCV/RISCVCallLowering.cpp:132-159
+void RISCVCallLowering::setISDArgsForCallingConv(const Function &F,
+                                                 const ArgInfo &OrigArg,
+                                                 SmallVectorImpl<EVT> &SplitVTs,
+                                                 SmallVectorImpl<T> &ISDArgs,
+                                                 bool isRet) const {
+  const DataLayout &DL = F.getParent()->getDataLayout();
+  LLVMContext &Ctx = F.getContext();
----------------
This is reinventing generic infrastructure


================
Comment at: llvm/lib/Target/RISCV/RISCVCallLowering.cpp:161
+
+void RISCVCallLowering::splitToValueTypes(const ArgInfo &OrigArg,
+                                          SmallVectorImpl<ArgInfo> &SplitArgs,
----------------
Ditto, this is a less complete copy of the generic infrastructure


================
Comment at: llvm/lib/Target/RISCV/RISCVCallLowering.cpp:203
+template <typename T>
+void RISCVCallLowering::updateArgLocInfo(
+    SmallVectorImpl<CCValAssign> &ArgLocs,
----------------
Ditto


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D117318/new/

https://reviews.llvm.org/D117318