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

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm added a comment.

All minor comments except for the concern on whether or not a truncation node is needed when ValSize < LocSize.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7010
+      int CurArgOffset = VA.getLocMemOffset();
+      // Objects in AIX are right justified.
+      if (ArgSize < ObjSize)
----------------
cebowleratibm wrote:
> suggest:
> 
> // AIX objects are right justified because they are word written to stack memory from their register image.
> 
updated suggestion:

// Objects are right-justified because AIX is big-endian.

I tend to avoid the use of the term word because in Power ISA it means 4 bytes.  To others it means PtrByteSize.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7029
   // Area that is at least reserved in the caller of this function.
-  unsigned MinReservedArea = CCInfo.getNextStackOffset();
+  unsigned MinReservedArea =
+      std::max(CCInfo.getNextStackOffset(), LinkageSize + MinParameterSaveArea);
----------------
MinReservedArea is now a misnomer.  This is effectively the LSA + PSA size, whatever the PSA size was evaluated to.  Min is always 56 in 32-bit and 112 in 64-bit. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6844
     const MVT RegVT = IsPPC64 ? MVT::i64 : MVT::i32;
-    if (unsigned Reg = State.AllocateReg(IsPPC64 ? GPR_64 : GPR_32)) {
-      // Promote integers if needed.
-      if (ValVT.getSizeInBits() < RegVT.getSizeInBits())
+    if (ValVT.getSizeInBits() < RegVT.getSizeInBits())
         LocInfo = ArgFlags.isSExt() ? CCValAssign::LocInfo::SExt
----------------
I agree with this change though I plan that it will be committed on another change to correct the caller stackarg patch I authored previously.  I think it's best to do this as an independent change with new test coverage for what was missed.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6848
+    if (unsigned Reg = State.AllocateReg(IsPPC64 ? GPR_64 : GPR_32)) {
+      // Promote integers if needed.
       State.addLoc(CCValAssign::getReg(ValNo, ValVT, Reg, RegVT, LocInfo));
----------------
this comment is now in the wrong place.  It belongs with line 6844.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7009
+    const unsigned ValueSize = ValVT.getStoreSize();
+    assert((ValueSize <= LocSize) &&
+           "Object size is larger than size of MemLoc");
----------------
Do we need to add a truncate node when ValueSize is less than LocSize?

Suggest "ValSize".


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1327
+; 64BIT:       liveins:
+; 64BIT-NEXT:  - { reg: '$x3', virtual-reg: '' }
+; 64BIT-NEXT:  - { reg: '$x4', virtual-reg: '' }
----------------
cebowleratibm wrote:
> DAG.
64BIT-LABEL probably better.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1301
+
+; 32BIT-DAG:   liveins:
+; 32BIT-DAG:   - { reg: '$r3', virtual-reg: '' }
----------------
32BIT-LABEL ?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1337
+
+; 64BIT-DAG:   fixedStack:
+; 64BIT-DAG:   - { id: 7, type: default, offset: 112, size: 8 
----------------
64BIT-LABEL


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1565
+
+; CHECK-DAG:   liveins:
+; CHECK-DAG:   - { reg: '$f1', virtual-reg: '' }
----------------
CHECK-LABEL


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1580
+
+; CHECK:       fixedStack:
+; 32BIT-DAG:   - { id: 2, type: default, offset: 128, size: 4
----------------
CHECK-LABEL


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1589
+
+; CHECK:       body:             |
+; CHECK-DAG:    bb.0.entry:
----------------
CHECK-LABEL


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1817
+
+; 32BIT-DAG:   liveins:
+; 32BIT-DAG:   - { reg: '$f1', virtual-reg: '' }
----------------
LABEL


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1823
+
+; 32BIT-DAG:   fixedStack:
+; 32BIT-DAG:   - { id: 6, type: default, offset: 56, size: 4
----------------
LABEL


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1832
+
+; 32BIT-DAG:   body:             |
+; 32BIT-DAG:     bb.0.entry:
----------------
LABEL


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1846
+
+; 64BIT:       fixedStack:
+; 64BIT-DAG:   - { id: 1, type: default, offset: 116, size: 8
----------------
LABEL


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

https://reviews.llvm.org/D74225