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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCCCState.h:41
+
+class AIXCCState : public CCState {
+private:
----------------
cebowleratibm wrote:
> 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.
IIUC PPCCCState is exclusively used by the 32-bit ELF target. When we add support for fp128 on AIX we might need the same functionality for 32-bit AIX at which point we would probably inherit from it instead of the base class. I can rename PPCCCState to ELF32CCState (I need to double check it is indeed only used by the 32-bit target first)


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6301
+                   CCState &S) {
+  AIXCCState &State = static_cast<AIXCCState &>(S);
   const PPCSubtarget &Subtarget = static_cast<const PPCSubtarget &>(
----------------
cebowleratibm wrote:
> 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.
Sure, but then I would have to duplicate `CCState::AnalyzeFormalArguments` and `CCState::AnalyzeCallOperands` functions in AIXCCState since the types for AIXCCAssignFn differs from the type of CCAssignFn. Is this acceptable? I picked the static cast in the assign function since we do something similar in the table-gen assign functions already, eg:
```CCIf<"!static_cast<PPCCCState *>(&State)->WasOriginalArgPPCF128(ValNo)",`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97485