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

[Jinsong Ji via Phabricator via llvm-commits]

jsji accepted this revision as: jsji.
jsji added a comment.
This revision is now accepted and ready to land.

LGTM with some nits.



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


================
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;
----------------
Is this affecting little endian as well?  If so, any test that can show the behavior change?


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


================
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);
----------------
Is it possible that `getVectorNumElements` is `1`? Then we will overflow the NewMask here?


================
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
----------------
comments need update?


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


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


================
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);
----------------
nit:

How about something like:
```
if(isLittleEndian)
   RHS = TheSplat;
else
   LHS = TheSplat;

Res = DAG.getVectorShuffle(SVN->getValueType(0), dl, LHS, RHS, ShuffV);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100478