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

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked 2 inline comments as done and an inline comment as not done.
sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6722
+      // R10.
+      HandleCustomVecRegLoc();
+      HandleCustomVecRegLoc();
----------------
cebowleratibm wrote:
> Suggestion: assert that VA.getValNo() matches across the set of custom mem/reg locs that we expect to see.
Good idea, added.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6728
+      if (I != End &&  ArgLocs[I].isRegLoc() && ArgLocs[I].needsCustom()) {
+        HandleCustomVecRegLoc();
+        HandleCustomVecRegLoc();
----------------
ZarkoCA wrote:
> maybe adding an assert for !IsPPC64 would be useful?
Good idea, added.


================
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:
> 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97485