[PATCH] D100478: [PowerPC] Canonicalize shuffles on big endian targets as well

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9580
+      assert((isLittleEndian || IsFourByte) &&
+             "Unexpected permuted load on big endian target");
       SplatIdx += IsFourByte ? 2 : 1;
----------------
jsji wrote:
> Do we need to update the comments to be more specific about 8 bytes?
I will change it to: `Unexpected size for permuted load on big endian target`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:9597
+      // loading with an offset would load the wrong memory.
+      if (LD->getValueType(0).getSizeInBits() == (IsFourByte ? 32 : 64))
+        Offset = 0;
----------------
jsji wrote:
> Is this affecting little endian as well?  If so, any test that can show the behavior change?
In theory, yes. However, we cannot get any offset other than `0` here on LE.
`SplatIdx` can only be `{0, 1}` for 8-byte permutes and `{0, 1, 2, 3}` for 4 byte permutes.

If the splat and load are 8 bytes, `SplatIdx` must be `1` because otherwise we wouldn't be splatting the value we loaded.

If the splat and load are 4 bytes, `SplatIdx` must be 3 because otherwise we wouldn't be splatting the value we loaded.

So the conditional above will ensure that `1 - SplatIdx/3 - SplatIdx` is `0` respectively.
On BE, this is not necessarily true because we don't adjust `SplatIdx` in the condition on line 9578. I could of course add the code up there to account for this but felt that it is clearer to just set the offset to zero when the load size and the splat element size are the same.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14220
+                                            int RHSMaxIdx, int HalfVec,
+                                            int ElemSize,
+                                            const PPCSubtarget &Subtarget) {
----------------
jsji wrote:
> `unsigned ElemSizeInBytes`?
Sounds good.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14252
+      ResultInElt -= Subtarget.isLittleEndian() ? 0 : 1;
+      NewMask[ResultInElt] = Idx->getZExtValue();
       return DAG.getVectorShuffle(VT, dl, OrigVector, OrigVector, NewMask);
----------------
jsji wrote:
> Is it possible that `getVectorNumElements` is `1`? Then we will overflow the NewMask here?
I will add an assert for that.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14260
 
 // On little endian subtargets, combine shuffles such as:
 // vector_shuffle<16,1,17,3,18,5,19,7,20,9,21,11,22,13,23,15>, <zero>, %b
----------------
jsji wrote:
> comments need update?
I'll add a note about what this does on BE.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14267
 // to put the value into element zero. Adjust the shuffle mask so that the
 // vector can remain in permuted form (to prevent a swap prior to a shuffle).
 SDValue PPCTargetLowering::combineVectorShuffle(ShuffleVectorSDNode *SVN,
----------------
```
// On big endian targets, this is still useful for SCALAR_TO_VECTOR
// nodes with elements smaller than doubleword because all the ways
// of getting scalar data into a vector register put the value in the
// rightmost element of the left half of the vector.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14298
   SDValue SToVRHS = isScalarToVec(RHS);
+  int ElemSize = 0;
   if (SToVLHS || SToVRHS) {
----------------
jsji wrote:
> should be `unsigned` here.? getScalarSizeInBits returning `unsigned`.
> Also `ElemSizeInBits`?
OK.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14329
     if (SToVRHS) {
+      if (!Subtarget.isLittleEndian() && ElemSize >= 64)
+        return Res;
----------------
jsji wrote:
> nit: Should we use local var instead? `bool isLittleEndian = Subtarget.isLittleEndian();` 
Absolutely, will do. Thanks.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14402
 
-  Res = DAG.getVectorShuffle(SVN->getValueType(0), dl, LHS, RHS, ShuffV);
+  if (Subtarget.isLittleEndian())
+    Res = DAG.getVectorShuffle(SVN->getValueType(0), dl, LHS, TheSplat, ShuffV);
----------------
jsji wrote:
> nit:
> 
> How about something like:
> ```
> if(isLittleEndian)
>    RHS = TheSplat;
> else
>    LHS = TheSplat;
> 
> Res = DAG.getVectorShuffle(SVN->getValueType(0), dl, LHS, RHS, ShuffV);
> ```
Makes sense.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100478