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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Ok, this looks about right.
A few high-level remarks:

- There is an obvious pattern in these `is*Mask()` functions. This makes them slightly self-duplicating. I wonder if it would be possible to refactor them somehow to reduce duplication.
- There is //some// correlation between those functions. I would personally add an exhaustive test (with googletest's `Combine`, if it is not disabled?) to check for those correlations.



================
Comment at: include/llvm/IR/Instructions.h:2467
+  /// Example: <undef,1,undef,3>
+  static bool isIdentityMask(Constant *Mask);
+
----------------
All these functions should be able to take `const Constant *Mask`.


================
Comment at: lib/IR/Instructions.cpp:1740-1741
+      continue;
+    UsesLHS &= (MaskEltVal == (LastEltIndex - i));
+    UsesRHS &= (MaskEltVal == (NumElts + LastEltIndex - i));
+    if (!UsesLHS && !UsesRHS)
----------------
This is slightly inconsistent, why not use helper variables here?
```
    bool IsLHS = MaskEltVal == (LastEltIndex - i);
    bool IsRHS = MaskEltVal == (NumElts + LastEltIndex - i);
    UsesLHS &= IsLHS;
    UsesRHS &= IsRHS;
```


https://reviews.llvm.org/D48236