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

[Zarko Todorovski via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6957
   if (Subtarget.hasQPX())
     report_fatal_error("QPX support is not supported on AIX.");
 
----------------
cebowleratibm wrote:
> I think it would be useful to add a !byVal assertion on the args just to make it clear the logic you've implemented is not intended to handle this case.
Good idea, added. 


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7008
 
-    assert(VA.isRegLoc() && "Unexpected argument location.");
-
-    EVT ValVT = VA.getValVT();
-    MVT LocVT = VA.getLocVT();
-    MVT::SimpleValueType SVT = ValVT.getSimpleVT().SimpleTy;
-    unsigned VReg =
-        MF.addLiveIn(VA.getLocReg(), getRegClassForSVT(SVT, IsPPC64));
-    SDValue ArgValue = DAG.getCopyFromReg(Chain, dl, VReg, LocVT);
-    if (ValVT.isScalarInteger() &&
-        (ValVT.getSizeInBits() < LocVT.getSizeInBits())) {
-      ISD::ArgFlagsTy Flags = Ins[VA.getValNo()].Flags;
+      // Get the extended size of the argument type in stack
+      const unsigned ArgSize = VA.getLocVT().getStoreSize();
----------------
cebowleratibm wrote:
> Since there doesn't appear to be any necessary handling to sign or zero extend on this code path, should there be an assertion?
I added `assert(ObjSize <= ArgSize);` is this what you had in mind? Or is a different assert specifically related to sign or zero extension needed?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1271
+
+define i32 @test_ints_stack(i32 %i1, i32 %i2, i32 %i3, i32 %i4, i32 %i5, i32 %i6, i32 %i7, i32 %i8, i64 %ll9, i16 signext %s10, i8 zeroext %c11, i32 %ui12, i32 %si13, i64 %ll14, i8 zeroext %uc15, i32 %i16) {
+entry:
----------------
cebowleratibm wrote:
> The convention we've used for cc tests so far is to do expected output for both callee and caller.  I'd like to see the caller IR and expected output as well.
Added both caller and callee for all tests. 


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


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1346
+; 64BIT-NEXT:    bb.0.entry:
+; 64BIT-NEXT:      liveins: $x3, $x4, $x5, $x6, $x7, $x8, $x9, $x10
+
----------------
cebowleratibm wrote:
> I would like to see the expected output for the assembly added.  We should validate that the instructions emitted locate the arguments in the correct memory location.  We should also strive for consistency with the other tests, which have expected assembly.
Added assembly expected output. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-abi.ll:1401
+
+
----------------
cebowleratibm wrote:
> Additional test requests:
> -1 call that passes 1, 2, 4, 8 byte ints and 4, 8 byte floats in memory (mixed with all sizes represented.)
> -burn the first 8 GPR on float args, then confirm integer args passed in memory.
>   ex: foo(double, double, double, double, char, short, int)
Thanks, that's a really good testcase. I added it as `mix_callee` below along with the respective caller testcase. 


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

https://reviews.llvm.org/D74225