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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Thank you for looking at this Amy. I'm sorry it too me so long to get to it.
I've addressed some of your comments and added responses to others.



================
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
----------------
amyk wrote:
> 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.
It's just a style preference for me to add the switch statement to the whole thing. The code for sizes `1, 2, 4` has not changed and I've just replaced the:
```
            EVT ObjType = (ObjSize == 1 ? MVT::i8 :
                           (ObjSize == 2 ? MVT::i16 : MVT::i32));
```
with the switch statement.
I like this better because I have to check the size anyway to get the EVT so there is no point in splitting this part off into its own code. At least that is how I feel from a style perspective.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4506
+            break;
+          case 3: {
+            // i24 = i16 + i8
----------------
amyk wrote:
> 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?
I think using Arg in this case is correct. We do want to make sure that we adjust the address in the Big Endian situation. I want to be able to treat the 3/5/6/7 case in a similar way to the way that we create the single load situations.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4519
+            break;
+          case 5:
+            // i40 = i32 + i8
----------------
amyk wrote:
> 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.
Actually, I'm going to remove the `{ }` from all of the cases. I needed them when I was doing development but at this point they are not required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108795