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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added a comment.

In D74225#1894003 <https://reviews.llvm.org/D74225#1894003>, @cebowleratibm wrote:

> I think we need some changes to ensure frame objects are created with their proper size and not word size.


We have 2 choices for the smaller then save slot sized objects:

1. Follow XL's behavior and perform a load of the entire save slot, and create frame objects of this size to match. If we chose this we would have to build and insert 'AssertSExt' and 'AssertZExt` nodes and truncates appropriately. The truncates should get cleaned up down stream due to the assert-extended nodes.
2. Perform the offset adjustment and load using the load instruction with the correct extending type, and have frame objects created at the correct offset matching the size of the load.

It seems we are mixing these 2 right now? but I have to play with some smaller tests to make sure my understanding is correct.

Having the caller and callee semantics in the same test is helpful, but it leads to having really large tests.  Can we split the tests into 2. 1 for everything fits in regs and 1 for when we have to go to the stack?



================
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
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7014
+    // memory from their register image.
+    if ((LocSize > ValSize) && IsPPC64)
+      CurArgOffset += LocSize - ValSize;
----------------
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 :)


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

https://reviews.llvm.org/D74225