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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6806
+    // Successfully reserved GPRs are only initialized for vararg calls.
+    // Custom handling is required to split an f64 into 2 GPRs.
+    if (!IsPPC64 && LocVT.SimpleTy == MVT::f64) {
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7022
+      assert(Arg.getValueType() == MVT::f64 && isVarArg && !IsPPC64 &&
+             "Unexpected custom register for argument");
+      CCValAssign &GPR1 = VA;
----------------
cebowleratibm wrote:
> hubert.reinterpretcast wrote:
> > Period at the end of the sentence.
> For the assertion comment?  I do not see this precedent in the LLVM source but I will add an exclamation because that seems to be the thing to do ;)
> 
There is not a consistency in precedent in the LLVM source on this, but it's an easier rule to follow to always use punctuation (except to check precedent carefully where user messages are involved).


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7034
+      if (!GPR2.isRegLoc() || GPR2.getValNo() != GPR1.getValNo())
+        continue;
+      assert(GPR2.needsCustom() && "A second custom GPR is expected");
----------------
cebowleratibm wrote:
> hubert.reinterpretcast wrote:
> > What sort of situation leads to this?
> The matching ValNo should be an assertion.  I should have written:
> 
>        // If no GPRs were left then there may be no more ArgLocs remaining.
>       if (I + 1 == e)
>         continue;
>       CCValAssign &GPR2 = ArgLocs[++I];
>       // If no GPRs were left then there may be non-reg ArgLocs remaining.
>       if (!GPR2.isRegLoc()))
>         continue;
>       assert(GPR2.getValNo() == GPR1.getValNo() && GPR2.needsCustom() &&
>              "A second custom GPR is expected!");
> 
If we observed one custom reg and we are dealing with the var arg case. My understanding is that, if we were unable to allocate a second GPR for the vararg case, the loop would have hit `report_fatal_error`.


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

https://reviews.llvm.org/D71013