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

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm marked 14 inline comments as done.
cebowleratibm 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) {
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7022
+      assert(Arg.getValueType() == MVT::f64 && isVarArg && !IsPPC64 &&
+             "Unexpected custom register for argument");
+      CCValAssign &GPR1 = VA;
----------------
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 ;)



================
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");
----------------
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!");



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

https://reviews.llvm.org/D71013