[PATCH] D73209: [AIX] Implement caller arguments passed in stack memory

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm marked 7 inline comments as done.
cebowleratibm added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6886-6891
+        // If there are insufficient GPRs, the PSA needs to be initialized.
+        // Initialization occurs even if an FPR was initialized for
+        // compatibility with the AIX XL compiler. The full memory for the
+        // argument will be initialized even if a prior word is saved in GPR.
+        State.addLoc(CCValAssign::getMem(ValNo, ValVT, Offset, LocVT, LocInfo));
+        break;
----------------
ZarkoCA wrote:
> Can we add a test for this? Specifically checking if the full memory for the arg is initialized. 
call_test_stackarg_float2 covers this scenario.  There are STFS and STFD for the trailing float and double args.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7091-7099
+    if (VA.isMemLoc()) {
+      SDValue PtrOff =
+          DAG.getConstant(VA.getLocMemOffset(), dl, StackPtr.getValueType());
+      PtrOff = DAG.getNode(ISD::ADD, dl, PtrVT, StackPtr, PtrOff);
+      MemOpChains.push_back(
+          DAG.getStore(Chain, dl, Arg, PtrOff, MachinePointerInfo()));
+
----------------
ZarkoCA wrote:
> I would prefer to have the MemLoc code after the RegLoc. To me it's more intuitive that way since we use up registers first and then stack.  It's purely a personal preference though and have no issue with keeping it as is. 
I think this is a personal preference but I'll reorder the logic.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7147-7151
+      if (I != E) {
+        // If only 1 GPR was available, there will only be one custom GPR and the argument will also pass in memory.
+        CCValAssign &PeekArg = ArgLocs[I];
+        if (PeekArg.isRegLoc() && PeekArg.getValNo() == PeekArg.getValNo()) {          
+          assert(PeekArg.needsCustom() && "A second custom GPR is expected.");
----------------
ZarkoCA wrote:
> Can we have a testcase for this? 
That's fair.  test_stackarg_float2 covers has a double arg split across that last available GPR.  I can add a case for the scenario where the double arg will fit in the last 2 available GPRs.


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

https://reviews.llvm.org/D73209