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

[Zarko Todorovski via Phabricator via llvm-commits]

ZarkoCA planned changes to this revision.
ZarkoCA marked 15 inline comments as done.
ZarkoCA added inline comments.


================
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);
----------------
cebowleratibm wrote:
> 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. 
I think the name is used in all PPC targets because of the call to `FuncInfo->setMinReservedArea`, I agree with the comment though. 

I changed it to CallerReservedArea.  I think if I do that, then we don't even need the comment.  


================
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
----------------
cebowleratibm wrote:
> 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.
I agree, it wasn't meant to be included here initially.  I added it when trying to figure out some of the offsets I was seeing.  I will keep it in but let the follow up patch which includes this land first before I do any commits of this one. 


================
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");
----------------
cebowleratibm wrote:
> Do we need to add a truncate node when ValueSize is less than LocSize?
> 
> Suggest "ValSize".
>From what I've looked into, no other PPC targets add a truncate node in their respective LowerFormalArguments() functions.  I did find it being done on x86 for i1s, however, and poked around to see why the difference is there.  

As far as I understand, it looks like we don't need to do on PPC since PPCTargetLowering::LowerLOAD() will do that when we set the load node in LowerFormalArguments.  

The relevant code snippet from PPCTargetLowering::LowerLOAD() is below: 

```
SDValue NewLD =
      DAG.getExtLoad(ISD::EXTLOAD, dl, getPointerTy(DAG.getDataLayout()), Chain,
                     BasePtr, MVT::i8, MMO);
  SDValue Result = DAG.getNode(ISD::TRUNCATE, dl, MVT::i1, NewLD);
```

I'm fairly sure that's the reason why we don't see truncation to loads from memory in the various PPC LowerFormalArguments() since it's done in LowerLoad for certain types.  


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1301
+
+; 32BIT-DAG:   liveins:
+; 32BIT-DAG:   - { reg: '$r3', virtual-reg: '' }
----------------
cebowleratibm wrote:
> 32BIT-LABEL ?
Sorry, missed those in the initial update. 


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

https://reviews.llvm.org/D74225