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

[Hubert Tong via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6813
+                                                   MVT::i32, LocInfo));
+        } else if (State.isVarArg())
+          report_fatal_error("Handling of placing parameters on the stack is "
----------------
I can't fathom why there would be objections over using braces here. I believe the lack of braces seriously hurts readability in this case.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6817
+      return false;
+    }
+    MVT RegVT = IsPPC64 ? MVT::i64 : MVT::i32;
----------------
A newline would help make the early exit more obvious.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6978
   const unsigned LinkageSize = Subtarget.getFrameLowering()->getLinkageSize();
-  const unsigned PtrByteSize = Subtarget.isPPC64() ? 8 : 4;
+  bool IsPPC64 = Subtarget.isPPC64();
+  unsigned PtrByteSize = IsPPC64 ? 8 : 4;
----------------
Add `const` to be consistent with `LinkageSize`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6979
+  bool IsPPC64 = Subtarget.isPPC64();
+  unsigned PtrByteSize = IsPPC64 ? 8 : 4;
   CCInfo.AllocateStack(LinkageSize, PtrByteSize);
----------------
`const` here as well.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6999
 
-  for (CCValAssign &VA : ArgLocs) {
+  for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
+    CCValAssign &VA = ArgLocs[i];
----------------
Do the current naming conventions indicate that these should be `I` and `E`?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7000
+  for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
+    CCValAssign &VA = ArgLocs[i];
     SDValue Arg = OutVals[VA.getValNo()];
----------------
I suggest to increment here (`ArgLocs[i++]`) instead of in the loop-continuation. I find increment-right-after-latching to be more symmetric for this code.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7022
+      assert(Arg.getValueType() == MVT::f64 && isVarArg && !IsPPC64 &&
+             "Unexpected custom register for argument");
+      CCValAssign &GPR1 = VA;
----------------
Period at the end of the sentence.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7030
+      // The second GPR may not be present if there were no GPRs remaining.
+      if (i + 1 == e)
+        continue;
----------------
If changing the increment, then `i == e` here.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7032
+        continue;
+      CCValAssign &GPR2 = ArgLocs[++i];
+      if (!GPR2.isRegLoc() || GPR2.getValNo() != GPR1.getValNo())
----------------
If changing the increment, then `ArgLocs[i++]` here.


================
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");
----------------
What sort of situation leads to this?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7035
+        continue;
+      assert(GPR2.needsCustom() && "A second custom GPR is expected");
+      RegsToPass.push_back(std::make_pair(
----------------
Period at the end of the sentence.


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

https://reviews.llvm.org/D71013