[PATCH] D33404: [PowerPC] Fix a performance bug for PPC::XXPERMDI.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1669
+// Check that the mask is shuffling double words
+static bool isDoubleWordShuffleMask(ShuffleVectorSDNode *N) {
+   unsigned B[8];
----------------
Seems like a bit of code duplication with `isWordShuffleMask()`. Perhaps it would be more consistent to just implement something like `isNByteElemShuffleMask(unsigned Width)` which can be called for width 1, 2, 4, 8, 16 as needed.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1670
+static bool isDoubleWordShuffleMask(ShuffleVectorSDNode *N) {
+   unsigned B[8];
+  for (unsigned i = 0; i < 2; ++i) {
----------------
Nit: formatting. This is indented more than the rest of the function for some reason.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1671
+   unsigned B[8];
+  for (unsigned i = 0; i < 2; ++i) {
+    B[0] = N->getMaskElt(i * 8);
----------------
I'm not a fan of loops with 2 iterations. It's probably clearer if this is just straight-line code.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1743
+// Calculate the third parameter of XXPERMDI, which is DM in the ISA
+static unsigned getDMValue(unsigned M0, unsigned M1, bool IsLE) {
+  return IsLE ? (((~M1) & 1) << 1) + ((~M0) & 1) : ((M0 << 1) + (M1 & 1));
----------------
Some people really don't like boolean parameters :). I really don't think this function is needed - just do the computation inline.
Especially when I look at the call sites - it looks like there are 3 and you've already checked the endianness on 2 of them.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1748
+bool PPC::isXXPERMDIShuffleMask(ShuffleVectorSDNode *N, unsigned &DM,
+                               bool &Swap, bool IsLE) {
+  if (N->getValueType(0) != MVT::v16i8)
----------------
Nit: indentation.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1749
+                               bool &Swap, bool IsLE) {
+  if (N->getValueType(0) != MVT::v16i8)
+    return false;
----------------
Is there a reason for this check (i.e. vs. an assert)? It seems we should be safe to assert this is the case.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1758
+  unsigned M1 = N->getMaskElt(8) / 8;
+
+
----------------
I think maybe an assert is in order here... Perhaps:
`assert((M0 | M1 < 4) && "A mask element out of bounds?")`


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1763
+  if (N->getOperand(1).isUndef()) {
+    if ((M0 == 0 || M0 == 1) && (M1 == 0 || M1 == 1)) {
+      DM = getDMValue(M0, M1, IsLE);
----------------
Isn't this just `(M0 | M1) < 2`? Namely, neither is greater than 1.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1771
+
+  if (IsLE) {
+    if ((M0 == 3 || M0 == 2) || (M1 == 0 && M1 == 1)) {
----------------
This entire if statement makes my head hurt. At the very least, it should be documented. But I'm sure when you document it, it'll be obvious how it can be rewritten to be much simpler. Comment it with something along the lines of:
```
// If element 0 of the result comes from the first input (LE) or second input (BE)
// the inputs need to be swapped and elements adjusted accordingly.
```

I've suggested simplifications for the LE conditions, but you should be able to simplify the BE conditions accordingly.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1772
+  if (IsLE) {
+    if ((M0 == 3 || M0 == 2) || (M1 == 0 && M1 == 1)) {
+      Swap = false;
----------------
If we ignore the fact that the second half of this can never be satisfied (M1 can't be 0 and 1), I assume this should be just:
`if (M0 > 1 && M1 < 2)`


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1774
+      Swap = false;
+    } else if ((M0 == 1 && M1 == 3) || (M0 == 0 && M1 == 3) ||
+               (M0 == 1 && M1 == 2) || (M0 == 0 && M1 == 2)) {
----------------
Similarly, this one seems like it should just be:
`if (M0 < 2 && M1 > 1)`


https://reviews.llvm.org/D33404