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

[Zarko Todorovski via Phabricator via llvm-commits]

ZarkoCA marked 2 inline comments as done.
ZarkoCA added inline comments.


================
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
----------------
sfertile wrote:
> ZarkoCA wrote:
> > 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. 
> I don't see the LocInfo being used on the Memloc path for lower formal args, so this shouldn't semantically affect this patch.Is it included because it affects the lit tests in an observable way? If it doesn't pull it out, if it does then you can pull this patch into your branch and use just the diff of your changes for this review.
It's not needed for the lower formal args but this patch includes caller tests which are affected.  


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7014
+    // memory from their register image.
+    if ((LocSize > ValSize) && IsPPC64)
+      CurArgOffset += LocSize - ValSize;
----------------
sfertile wrote:
> Why don't we have to perform the adjustment for ppc32? IIUC ValSize is promoted to i32 for i16/i8, but what about i1s? They shouldn't be promoted by the generic selection dag code because they are a legal register type on PowerPC. At the very least we need a lit test that includes `i1 zeroext` passed in a stack slot. Sorry if there is one, I scanned through and don't see one but the test changes are 1000 lines long :)
Sorry, @sfertile I missed your comments and you're right.  The i1 zeroext is a case which we'll need to do exactly that. I will remove the IsPPC64 and a test for it. 


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

https://reviews.llvm.org/D74225