[llvm] Add SD matchers and unit test coverage for ISD::VECTOR_SHUFFLE (PR #119592)

Min-Yih Hsu via llvm-commits llvm-commits at lists.llvm.org
Thu Dec 26 10:22:02 PST 2024


================
@@ -548,49 +548,39 @@ struct BinaryOpc_match {
 };
 
 /// Matching while capturing mask
-template <typename T0, typename T1> struct SDShuffle_match {
+template <typename T0, typename T1, typename T2> struct SDShuffle_match {
   T0 Op1;
   T1 Op2;
+  T2 Mask;
 
-  ArrayRef<int> &CapturedMask;
-
-  // capturing mask
-  SDShuffle_match(const T0 &Op1, const T1 &Op2, ArrayRef<int> &MaskRef)
-      : Op1(Op1), Op2(Op2), CapturedMask(MaskRef) {}
+  SDShuffle_match(const T0 &Op1, const T1 &Op2, const T2 &Mask)
+      : Op1(Op1), Op2(Op2), Mask(Mask) {}
 
   template <typename MatchContext>
   bool match(const MatchContext &Ctx, SDValue N) {
     if (auto *I = dyn_cast<ShuffleVectorSDNode>(N)) {
-      if (Op1.match(Ctx, I->getOperand(0)) &&
-          Op2.match(Ctx, I->getOperand(1))) {
-        CapturedMask = I->getMask();
-        return true;
-      }
+      return Op1.match(Ctx, I->getOperand(0)) &&
+             Op2.match(Ctx, I->getOperand(1)) && Mask.match(I->getMask());
     }
     return false;
   }
 };
 
-/// Matching against a specific match
-template <typename T0, typename T1> struct SDShuffle_maskMatch {
-  T0 Op1;
-  T1 Op2;
-  ArrayRef<int> SpecificMask;
-
-  SDShuffle_maskMatch(const T0 &Op1, const T1 &Op2, ArrayRef<int> Mask)
-      : Op1(Op1), Op2(Op2), SpecificMask(Mask) {}
-
-  template <typename MatchContext>
-  bool match(const MatchContext &Ctx, SDValue N) {
-    if (auto *I = dyn_cast<ShuffleVectorSDNode>(N)) {
-      return Op1.match(Ctx, I->getOperand(0)) &&
-             Op2.match(Ctx, I->getOperand(1)) &&
-             std::equal(SpecificMask.begin(), SpecificMask.end(),
-                        I->getMask().begin(), I->getMask().end());
-    }
-    return false;
+struct m_Mask {
+  ArrayRef<int> &MaskRef;
+  m_Mask(ArrayRef<int> &MaskRef) : MaskRef(MaskRef) {}
+  bool match(ArrayRef<int> Mask) {
+    MaskRef = Mask;
+    return true;
   }
 };
+
+struct m_SpecificMask {
+  ArrayRef<int> &MaskRef;
----------------
mshockwave wrote:

I would recommend you to step back a little bit and think about the purpose of `m_Mask` and `m_SpecificMask`: in both IR PatternMatch and SD PatternMatch, the former one captures the mask, so you need to pass the ArrayRef that carries the _outgoing_ value by reference. Remember, reference in C++ is almost like a pointer, you write the captured mask to the underlying memory pointed by this reference in order to "export" the value.

The latter one, m_SpecificMask, is passing in a mask you want to _compare_ against with, so that's why it's pass by value. 

Therefore, I think https://github.com/llvm/llvm-project/pull/119592/commits/80e64f6e30f1a5568bea6fbc62751f7c456d7e7c is wrong. And the CI passed because you also incorrectly changed the test from `ArrayRef<int> CaptureMask;` to `ArrayRef<int> CaptureMask(MaskData);` -- the `CaptureMask` here should be a variable storing the outgoing mask captured by m_Mask, so you really shouldn't initialize it.

https://github.com/llvm/llvm-project/pull/119592


More information about the llvm-commits mailing list