[PATCH] D48236: [IR] move shuffle mask queries from TTI to ShuffleVectorInst

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: lib/IR/Instructions.cpp:1693-1694
+    int MaskEltVal = getMaskValue(Mask, i);
+    if (MaskEltVal < 0)
+      continue;
+    bool IsLHS = (unsigned)MaskEltVal == i;
----------------
Negative values represent `undef`?


================
Comment at: lib/IR/Instructions.cpp:1704-1705
+      return false;
+  }
+  return true;
+}
----------------
Just as a sanity check, i would have expected to see an assert here:
```
assert((FoundLHS ^ FoundRHS) && "Should have selected from exactly one source");
```


================
Comment at: lib/IR/Instructions.cpp:1735-1738
+    ReverseLHS &= ((unsigned)MaskEltVal == (NumElts - 1 - i));
+    ReverseRHS &= ((unsigned)MaskEltVal == (NumElts + NumElts - 1 - i));
+    if (!ReverseLHS && !ReverseRHS)
+      return false;
----------------
This method was declared with:
```
  /// Return true if this shuffle mask swaps the order of elements from exactly
  /// one source vector.
  /// Example: <7,6,undef,4>
  /// This assumes that vector operands are the same length as the mask.
```
You are sure this does what that comment says it does?


================
Comment at: lib/IR/Instructions.cpp:1739-1740
+      return false;
+  }
+  return true;
+}
----------------
Just as a sanity check, i would have expected to see an assert here:
```
assert((ReverseLHS ^ ReverseRHS) && "Should have selected from exactly one source");
```


================
Comment at: lib/IR/Instructions.cpp:1757-1758
+      return false;
+  }
+  return true;
+}
----------------
Just as a sanity check, i would have expected to see an assert here:
```
assert(!(ShuffleLHS && ShuffleRHS) && "Should not have selected from both sources");
```


================
Comment at: lib/IR/Instructions.cpp:1773-1774
+      return false;
+  }
+  return true;
+}
----------------
Just as a sanity check, i would have expected to see an assert here:
```
assert((SplatLHS || SplatRHS) && "Should have splatted from at least one of sources");
```


================
Comment at: lib/IR/Instructions.cpp:1783-1787
+  // 2. The first element of the mask must be either a zero (for the
+  // even-numbered vector elements) or a one (for the odd-numbered vector
+  // elements).
+  if (getMaskValue(Mask, 0) != 0 && getMaskValue(Mask, 0) != 1)
+    return false;
----------------
```
unsigned NumSourceElts = Op<0>()->getType()->getVectorNumElements();
```

But this does not seem to take the `NumSourceElts & 1` into account, like the comment says?


https://reviews.llvm.org/D48236