[PATCH] D74225: [AIX] Implement formal arguments passed in stack memory

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm added a comment.

Code semantics look ok.  If you can post a revision to address the minor concerns I need to have one more pass-through of the test updates in detail.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7001
+      }
+      InVals.push_back(ArgValue);
+    } else {
----------------
suggest continue here, then get rid of the else { }


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7005
+      // Get the extended size of the argument type in stack.
+      const unsigned ArgSize = VA.getLocVT().getStoreSize();
+      // Get the actual size of the argument type.
----------------
you already latched VA.getLocVT and VA.getValVT into locals you can reuse.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7007
+      // Get the actual size of the argument type.
+      const unsigned ObjSize = VA.getValVT().getStoreSize();
+      assert(ObjSize <= ArgSize);
----------------
I find the use of name "ObjSize" and "ArgSize" confusing.  Suggest sticking to terminology "ValueSize" and "LocSize".


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7010
+      int CurArgOffset = VA.getLocMemOffset();
+      // Objects in AIX are right justified.
+      if (ArgSize < ObjSize)
----------------
suggest:

// AIX objects are right justified because they are word written to stack memory from their register image.



================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1311
+; 32BIT:       fixedStack:
+; 32BIT:       - { id: 0, type: default, offset: 92, size: 4
+; 32BIT:       - { id: 1, type: default, offset: 88, size: 4
----------------
ZarkoCA wrote:
> cebowleratibm wrote:
> > I suggest 32BIT-DAG and reorder the memory locations in growing offset.  IMO it makes it easier to read and understand the test.
> Agreed, it does look better once done that way. 
suggest 32BIT-LABEL to tag the expected sections.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1350
+
+; ASM32PWR4-DAG:   add 3, 3, 4
+; ASM32PWR4-DAG:   lwz [[REG1:[0-9]+]], 60(1)
----------------
Suggest adding a LABEL check for the assembly output.  Applies multiple.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1351
+; ASM32PWR4-DAG:   add 3, 3, 4
+; ASM32PWR4-DAG:   lwz [[REG1:[0-9]+]], 60(1)
+; ASM32PWR4-DAG:   add 3, 3, 5
----------------
suggest reordering these lwz closer to the reg use for readability.


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

https://reviews.llvm.org/D74225