[PATCH] D76380: [PowerPC][AIX] Implement by-val caller arguments in multiple registers

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7151
+    const CCValAssign &PeekVA = ArgLocs[I];
+    unsigned ValNo = PeekVA.getValNo();
+    SDValue Arg = OutVals[ValNo];
----------------
If all we need PeekVA for is its ValNo lets do `unsigned ValNo = ArgLocs[I].getValNo()`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7166
+          // Adjust the load offset by the number of bytes read so far.
+          SDNodeFlags AddFlags;
+          AddFlags.setNoUnsignedWrap(true);
----------------
We do this same operation twice in this function, and the same pattern appears several times elsewhere in the file. I suggest putting it in a well named helper function.  Only bother using for the 2 uses in this function for this patch, then we can convert some of the other uses in NFC patches.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:7167
+          SDNodeFlags AddFlags;
+          AddFlags.setNoUnsignedWrap(true);
+          LoadAddr =
----------------
If we are setting the no-wrap flags maybe we should set both.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-cc-byval.ll:208
+
+%struct.S5 = type { [5 x i8] }
+
----------------
I don't see a lot of utility in testing each of 1 through 8 bytes with nearly exactly the same test. The are each so similar that we end up with a long verbose test with not a lot of practical coverage. Instead how about we add 

1) one of 5,6,7,8 bytes but with other arguments before/after the by val.
2) A test with a couple byvals mixed with other arguments.
3) One test with a 'large' by val like  { [32 x i8] }.

Also use a struct type that isn't just containing an array. Ex if you choose 8 bytes as the size for the test in case 1 use:  { i16, i16, i16, i16} as the struct type.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76380