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

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm accepted this revision.
cebowleratibm added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6301
+                   CCState &S) {
+  AIXCCState &State = static_cast<AIXCCState &>(S);
   const PPCSubtarget &Subtarget = static_cast<const PPCSubtarget &>(
----------------
sfertile wrote:
> cebowleratibm wrote:
> > sfertile wrote:
> > > 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)",`
> > You had to declare AIXCCState::AnalyzeFormalArguments and AIXCCState::AnalyzeCallOperands in this patch anyways.  It was my observation that you could declare them with AIXCCAssignFn instead to enforce the type safety and avoid the static_cast.  I can accept the static cast but I think the suggestion is safer for a small effort.
> The definitions I add in AIXCCState forwards the assign function on to the base class implementation though. If I change the type of the assign function we have a type mismatch between the assign function type AIXCCState uses and  the type of assign function the base class expects. Thats simple to solve, I can duplicate the code from the base class inline in AIXCCState. I am happy with either approach but wanted to make sure you understood getting the type safety in the assign function signature come with the cost of having to duplicate that code. 
I am ok with either approach that you want to take.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97485