[PATCH] D71013: [AIX] Allow vararg calls when all arguments reside in registers.

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked an inline comment as done.
sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6806
-    for (unsigned i = 0; i < StoreSize; i += PtrByteSize)
-      State.AllocateReg(IsPPC64 ? GPR_64 : GPR_32);
     return false;
----------------
hubert.reinterpretcast wrote:
> cebowleratibm wrote:
> > hubert.reinterpretcast wrote:
> > > cebowleratibm wrote:
> > > > sfertile wrote:
> > > > > We should likely be creating these as shadow regs in the non-vararg case, but I failed to see that in our previous review, sorry. I don't think we need to update that for this patch, but we should before the calling convention work is finished.
> > > > A shadow reg is allocated in the same way as any other reg.  What makes it a shadow reg is if there's no associated RegLoc for the register.  This falls out naturally because we only create the RegLoc for vararg calls where we require it be initialized.  The allocated but uninitialized GPRs are already recognized as shadow regs.
> > > Newline before this to increase the association of this comment line with the following block.
> > All 4 comments apply to the block of code that follows.  I'm not sure where you want the newline or why it improves readability.
> All four comments apply to the larger block of code that follows. The last comment line is about the smaller block of code that follows, and I was asking for a newline before that last comment line. I might also go for putting it inside the `if` block.
Thanks for clearing that up. I mistakenly though the CCState object had to specifically be told a register is a shadow reg, for example through `AllocateReg (unsigned Reg, unsigned ShadowReg)`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7014
 
-    if (VA.isRegLoc())
-      RegsToPass.push_back(std::make_pair(VA.getLocReg(), Arg));
+    if (VA.isRegLoc()) {
+      if (!VA.needsCustom()) {
----------------
cebowleratibm wrote:
> sfertile wrote:
> > Suggestion: I'd prefer we lead with the 2 common cases, then have the special case handling at the end. 
> > 
> > ```
> > if (VA.isRegLoc() && !VA.needsCustom())) {
> >   RegsToPass.push_back(std::make_pair(VA.getLocReg(), Arg));
> > } else if(VA.isMemLoc()) {
> >   /// Impl
> > } else (VA.isRegLoc()) {
> >   // Custom handling here.
> > }
> > ```
> > 
> > I don't fell strongly about this though and think the current form is fine too.
> >   
> > 
> I think it's a good suggestion to move the MemLoc fatal error.  I'll move it to the top of the loop rather than here.
> 
> I prefer to use the continue to keep the nesting down rather than use an else.
Sure, that sounds good to me.


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

https://reviews.llvm.org/D71013