[PATCH] D97485: PowerPC][AIX] Handle variadic vector formal arguments.

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCCCState.h:41
+
+class AIXCCState : public CCState {
+private:
----------------
Should this derive from PPCCCState?  If not, I think PPCCCState should be renamed something to indicate not AIX because AIX is still a PPC calling convention.


================
Comment at: llvm/lib/Target/PowerPC/PPCCCState.h:68
+
+  bool isFixed(unsigned ValNo) { return IsFixed.test(ValNo); }
+};
----------------
const


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6301
+                   CCState &S) {
+  AIXCCState &State = static_cast<AIXCCState &>(S);
   const PPCSubtarget &Subtarget = static_cast<const PPCSubtarget &>(
----------------
Can you define:


```
typedef bool AIXCCAssignFn(unsigned ValNo, MVT ValVT,
                        MVT LocVT, CCValAssign::LocInfo LocInfo,
                        ISD::ArgFlagsTy ArgFlags, AIXCCState &State);
```

and retype the AIXCCState functions accordingly?  I think this would be more typesafe to avoid the need to static cast the state.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6708
+      HandleMemLoc();
+      while (I != End && ArgLocs[I].isRegLoc() && ArgLocs[I].needsCustom()) {
+        VA = ArgLocs[I++];
----------------
By my understanding, if we have a custom memloc for a vector arg then there must be either 2 or 4 custom reg locs that follow.  2 for the 32-bit "split" mem/reg case, or 2/4 for 32/64 bit shadow regs.  I think it would be better to structure this with the explicit logic to expect the 2 or 4 custom regs that follow with assertions, rather than use a loop to make the caller/callee contract fully explicit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97485