[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/PPCISelLowering.cpp:6301
+                   CCState &S) {
+  AIXCCState &State = static_cast<AIXCCState &>(S);
   const PPCSubtarget &Subtarget = static_cast<const PPCSubtarget &>(
----------------
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.


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


================
Comment at: llvm/test/CodeGen/PowerPC/aix32-vector-vararg-callee-split.ll:32
+  store i8* %argp.next, i8** %arg_list, align 4
+  %4 = inttoptr i32 %3 to <4 x i32>*
+  %5 = load <4 x i32>, <4 x i32>* %4, align 16
----------------
it's subtle to see how this involves vector CC.  I gather this is the IR we would see for a va_arg call with a vector int type.  Maybe worth a comment?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97485