[PATCH] D108795: [PowerPC] Fix issue with lowering byval parameters.

[Amy Kwan via Phabricator via llvm-commits]

amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4331
 
+static SDValue StoreWithOffset(SelectionDAG &DAG, SDValue Val, SDValue Arg,
+                               const Argument *FuncArg, uint64_t Offset,
----------------
Can we add documentation to this function?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4491
-          } else {
-            // For sizes that don't fit a truncating store (3, 5, 6, 7),
-            // store the whole register as-is to the parameter save area
----------------
Might be a silly question, but since it looks like the code for `ObjSize == [1|2|4]` looks like it is still the same, does it make sense for the switch to be in the else case for 3,5,6,7? 
Or is it a preference to just transform the whole thing as a switch (like what you've done in the patch)? I'm just wondering as it kind of looks like the code in the cases are fairly repetitive for some of the cases.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4506
+            break;
+          case 3: {
+            // i24 = i16 + i8
----------------
Question: I just wanted to check whether or not using `Arg` in cases 3/5/6/7 is expected, or if it is supposed to be frame index like in the code before?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4519
+            break;
+          case 5:
+            // i40 = i32 + i8
----------------
nit: It looks like for cases 3 and 6, there are `{ }` surrounding the case statements. It may be good to put them around 5 and 7 for consistency.


================
Comment at: llvm/test/CodeGen/PowerPC/ppc64-byval-multi-store.ll:3
+; RUN: llc -verify-machineinstrs --mtriple powerpc64le-unknown-linux-gnu \
+; RUN: -mcpu=pwr8 -ppc-asm-full-reg-names < %s | FileCheck %s --check-prefix=P8LE
+; RUN: llc -verify-machineinstrs --mtriple powerpc64le-unknown-linux-gnu \
----------------
nit: Indenting (for here and other lines)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108795