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

[Chris Bowler via Phabricator via llvm-commits]

cebowleratibm added a comment.

This concludes my initial review.  Mostly tidy up comments.  The logic seems correct so semantically I think it's ok (for the incremental support that it adds.)  I'll see if I can break it on the next round.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6957
   if (Subtarget.hasQPX())
     report_fatal_error("QPX support is not supported on AIX.");
 
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6959
 
+ // Potential tail calls could cause overwriting of argument stack slots.
+  const bool IsImmutable = !(getTargetMachine().Options.GuaranteedTailCallOpt &&
----------------
whitespace.  formatting.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6960
+ // Potential tail calls could cause overwriting of argument stack slots.
+  const bool IsImmutable = !(getTargetMachine().Options.GuaranteedTailCallOpt &&
+                             (CallConv == CallingConv::Fast));
----------------
I think it's better if this def is placed closer to its use.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6979
+    MVT LocVT = VA.getLocVT();
+    SDValue ArgValue;
+
----------------
I'd rather see this pushed into the respective conditional blocks.  It's not used outside so it's cleaner to contain it locally.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6982
+    if (!VA.isRegLoc() && !VA.isMemLoc())
+      report_fatal_error("Unexpected location for function call argument.");
+
----------------
I think this should be an assertion.  CC_AIX should never push a loc which is neither reg nor mem.  You could switch this up a bit:

if (VA.isRegLoc()) { ...}
assume(VA.isMemLoc)
// no need for "if (VA.isMemLoc())"


================
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();
----------------
Since there doesn't appear to be any necessary handling to sign or zero extend on this code path, should there be an assertion?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7023
+    }
     }
 
----------------
formatting.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74225