[VectorUtils] move x86's scaleShuffleMask to generic VectorUtils

[spatel]

Summary

We have some long-standing missing shuffle optimizations that could use this transform via VectorCombine now:
https://bugs.llvm.org/show_bug.cgi?id=35454
(and we still don't get that case in the backend either)

This function is apparently templated because there's existing code in IR that treats mask values as unsigned and backend code that treats masks values as signed?

The mask values are not endian-dependent IIUC.

[lebedev.ri]

I'm not convinced this endian-agnostic, but i agree this is correct for little-endian.

[efriedma]

Whether a given transform is endian-agnostic depends on the specific transform. If you're talking about reordering a bitcast and a shuffle, in particular, I'm pretty sure that's endian-agnostic. Trivial example:

%shufflefirst = shufflevector <1 x i64> %x, <1 x i64> %x, <1 x i32> <i32 0>
%z = bitcast <1 x i64> %shufflefirst to <2 x i32>

vs.

%bitcastfirst = bitcast <1 x i64> %x to <2 x i32>
%result = shufflevector <2 x i32> %bitcastfirst, <1 x i32> %bitcastfirst, <2 x i32> <i32 0, i32 1>

The shuffle is a no-op; an identity shuffle is spelled the same way on big-endian and little-endian targets.

[RKSimon]

This shouldn't be endian specific - DAGCombiner::visitVECTOR_SHUFFLE already does SHUFFLE(BITCAST,UNDEF)->BITCAST(SHUFFLE) with its own almost-identical version of ScaleShuffleMask.

[spatel]

In D76508#1934379, @RKSimon wrote:

This shouldn't be endian specific - DAGCombiner::visitVECTOR_SHUFFLE already does SHUFFLE(BITCAST,UNDEF)->BITCAST(SHUFFLE) with its own almost-identical version of ScaleShuffleMask.

Thanks - I knew that we had this somewhere else, but I missed that. I can remove that duplication as part of this patch if that improves the motivation.

[spatel]

Patch updated:
Replace duplicate shuffle mask scaling from DAGCombiner with calls to the generic util.

[lebedev.ri]

Thanks, that helped convince me of this not being endian-specific.
LG

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

19759–19760 ↗

(On Diff #251894)

Do we want to preserve fastpath?

19765 ↗

(On Diff #251894)

I'd personally keep ternary variant, not have two loops.

[RKSimon]

LGTM

llvm/include/llvm/Analysis/VectorUtils.h

346

Retaining a fast copy path for Scale == 1 would make sense

[spatel]

I'll check this in as-is and follow-up with the implementation improvements.

llvm/include/llvm/Analysis/VectorUtils.h

 /// Return true if each element of the vector value \p V is poisoned or equal to
 /// every other non-poisoned element. If an index element is specified, either
 /// every element of the vector is poisoned or the element at that index is not
 /// poisoned and equal to every other non-poisoned element.
 /// This may be more powerful than the related getSplatValue() because it is
 /// not limited by finding a scalar source value to a splatted vector.
 bool isSplatValue(const Value *V, int Index = -1, unsigned Depth = 0);
 
+/// Scale a shuffle or target shuffle mask, replacing each mask index with the
+/// scaled sequential indices for an equivalent mask of narrowed elements.
+/// Mask elements that are less than 0 (sentinel values) are repeated in the
+/// output mask.
+///
+/// Example with Scale = 4:
+///   <4 x i32> <3, 2, 0, -1> -->
+///   <16 x i8> <12, 13, 14, 15, 8, 9, 10, 11, 0, 1, 2, 3, -1, -1, -1, -1>
+///
+/// This is the reverse process of "canWidenShuffleElements", but can always
+/// succeed.
+template <typename T>
+void scaleShuffleMask(size_t Scale, ArrayRef<T> Mask,
+                      SmallVectorImpl<T> &ScaledMask) {
+  assert(Scale > 0 && "Unexpected scaling factor");
+  size_t NumElts = Mask.size();
+  ScaledMask.assign(NumElts * Scale, -1);
+
+  for (size_t i = 0; i != NumElts; ++i) {
+    int M = Mask[i];
+
+    // Repeat sentinel values in every mask element.
+    if (M < 0) {
+      for (size_t s = 0; s != Scale; ++s)
+        ScaledMask[(Scale * i) + s] = M;
+      continue;
+    }
+
+    // Scale mask element and increment across each mask element.
+    for (size_t s = 0; s != Scale; ++s)
+      ScaledMask[(Scale * i) + s] = (Scale * M) + s;
+  }
+}
+
 /// Compute a map of integer instructions to their minimum legal type
 /// size.
 ///
 /// C semantics force sub-int-sized values (e.g. i8, i16) to be promoted to int
 /// type (e.g. i32) whenever arithmetic is performed on them.
 ///
 /// For targets with native i8 or i16 operations, usually InstCombine can shrink
 /// the arithmetic type down again. However InstCombine refuses to create

llvm/lib/Target/X86/X86ISelLowering.h

void createSplat2ShuffleMask(MVT VT, SmallVectorImpl<T> &Mask, bool Lo) {
     int NumElts = VT.getVectorNumElements();
     for (int i = 0; i < NumElts; ++i) {
       int Pos = i / 2;
       Pos += (Lo ? 0 : NumElts / 2);
       Mask.push_back(Pos);
     }
   }
 
-  /// Helper function to scale a shuffle or target shuffle mask, replacing each
-  /// mask index with the scaled sequential indices for an equivalent narrowed
-  /// mask. This is the reverse process to canWidenShuffleElements, but can
-  /// always succeed.
-  template <typename T>
-  void scaleShuffleMask(size_t Scale, ArrayRef<T> Mask,
-                        SmallVectorImpl<T> &ScaledMask) {
-    assert(0 < Scale && "Unexpected scaling factor");
-    size_t NumElts = Mask.size();
-    ScaledMask.assign(NumElts * Scale, -1);
-
-    for (size_t i = 0; i != NumElts; ++i) {
-      int M = Mask[i];
-
-      // Repeat sentinel values in every mask element.
-      if (M < 0) {
-        for (size_t s = 0; s != Scale; ++s)
-          ScaledMask[(Scale * i) + s] = M;
-        continue;
-      }
-
-      // Scale mask element and increment across each mask element.
-      for (size_t s = 0; s != Scale; ++s)
-        ScaledMask[(Scale * i) + s] = (Scale * M) + s;
-    }
-  }
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_X86_X86ISELLOWERING_H

llvm/unittests/Analysis/VectorUtilsTest.cpp

TEST_F(BasicTest, isSplat) {
   Value *SplatC = IRB.CreateVectorSplat(5, ScalarC);
   EXPECT_TRUE(isSplatValue(SplatC));
 
   // FIXME: Constant splat analysis does not allow undef elements.
   Constant *SplatWithUndefC = ConstantVector::get({ScalarC, UndefScalar});
   EXPECT_FALSE(isSplatValue(SplatWithUndefC));
 }
 
+TEST_F(BasicTest, scaleShuffleMask) {
+  SmallVector<int, 16> ScaledMask;
+  scaleShuffleMask<int>(4, {3,2,0,-1}, ScaledMask);
+  EXPECT_EQ(makeArrayRef<int>(ScaledMask), makeArrayRef<int>({12,13,14,15,8,9,10,11,0,1,2,3,-1,-1,-1,-1}));
+}
+
 TEST_F(BasicTest, getSplatIndex) {
   EXPECT_EQ(getSplatIndex({0,0,0}), 0);
   EXPECT_EQ(getSplatIndex({1,0,0}), -1);     // no splat
   EXPECT_EQ(getSplatIndex({0,1,1}), -1);     // no splat
   EXPECT_EQ(getSplatIndex({42,42,42}), 42);  // array size is independent of splat index
   EXPECT_EQ(getSplatIndex({42,42,-1}), 42);  // ignore negative
   EXPECT_EQ(getSplatIndex({-1,42,-1}), 42);  // ignore negatives
   EXPECT_EQ(getSplatIndex({-4,42,-42}), 42); // ignore all negatives