Skip to content

[mlir][vector] Disable vector.flat_transpose for scalable vectors #115338

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

banach-space
Copy link
Contributor

@banach-space banach-space commented Nov 7, 2024

Disables vector.flat_transpose for scalable vectors. As per the docs:

This is the counterpart of llvm.matrix.transpose in MLIR

I'm not aware of any use of any matrix-multiply intrinsics in the
context of scalable vectors, hence disabling.

Note, this is a follow-on for #102573 in which I disabled
vector.matrix_multiply.

@llvmbot
Copy link
Member

llvmbot commented Nov 7, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-vector

Author: Andrzej Warzyński (banach-space)

Changes
  • [ValueTracking] Don't special case depth for phi of select (#114996)
  • [mlir][vector] Disable vector.flat_transpose for scalable vectors (#102573)

Full diff: https://github.com/llvm/llvm-project/pull/115338.diff

3 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+8-10)
  • (modified) mlir/include/mlir/Dialect/Vector/IR/VectorOps.td (+6-2)
  • (modified) mlir/test/Dialect/Vector/invalid.mlir (+9)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 37cd4caaca71df..ed3fa35c5b8610 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1565,20 +1565,12 @@ static void computeKnownBitsFromOperator(const Operator *I,
         // Skip direct self references.
         if (IncValue == P) continue;
 
-        // Recurse, but cap the recursion to one level, because we don't
-        // want to waste time spinning around in loops.
-        // TODO: See if we can base recursion limiter on number of incoming phi
-        // edges so we don't overly clamp analysis.
-        unsigned IncDepth = MaxAnalysisRecursionDepth - 1;
-
         // If the Use is a select of this phi, use the knownbit of the other
         // operand to break the recursion.
         if (auto *SI = dyn_cast<SelectInst>(IncValue)) {
-          if (SI->getTrueValue() == P || SI->getFalseValue() == P) {
+          if (SI->getTrueValue() == P || SI->getFalseValue() == P)
             IncValue = SI->getTrueValue() == P ? SI->getFalseValue()
                                                : SI->getTrueValue();
-            IncDepth = Depth + 1;
-          }
         }
 
         // Change the context instruction to the "edge" that flows into the
@@ -1589,7 +1581,13 @@ static void computeKnownBitsFromOperator(const Operator *I,
         RecQ.CxtI = P->getIncomingBlock(u)->getTerminator();
 
         Known2 = KnownBits(BitWidth);
-        computeKnownBits(IncValue, DemandedElts, Known2, IncDepth, RecQ);
+
+        // Recurse, but cap the recursion to one level, because we don't
+        // want to waste time spinning around in loops.
+        // TODO: See if we can base recursion limiter on number of incoming phi
+        // edges so we don't overly clamp analysis.
+        computeKnownBits(IncValue, DemandedElts, Known2,
+                         MaxAnalysisRecursionDepth - 1, RecQ);
 
         // See if we can further use a conditional branch into the phi
         // to help us determine the range of the value.
diff --git a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
index 3f45d0804e0450..6fe897cca79926 100644
--- a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
+++ b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
@@ -2770,11 +2770,11 @@ def Vector_FlatTransposeOp : Vector_Op<"flat_transpose", [Pure,
                  TCresVTEtIsSameAsOpBase<0, 0>>]>,
     Arguments<(
       // TODO: tighten vector element types that make sense.
-      ins VectorOfRankAndType<[1],
+      ins FixedVectorOfRankAndType<[1],
             [AnySignlessInteger, AnySignedInteger, Index, AnyFloat]>:$matrix,
           I32Attr:$rows, I32Attr:$columns)>,
     Results<(
-      outs VectorOfRankAndType<[1],
+      outs FixedVectorOfRankAndType<[1],
              [AnySignlessInteger, AnySignedInteger, Index, AnyFloat]>:$res)> {
   let summary = "Vector matrix transposition on flattened 1-D MLIR vectors";
   let description = [{
@@ -2789,6 +2789,10 @@ def Vector_FlatTransposeOp : Vector_Op<"flat_transpose", [Pure,
     a 2-D matrix with <rows> rows and <columns> columns, and returns the
     transposed matrix in flattened form in 'res'.
 
+    Note, the corresponding LLVM intrinsic, `@llvm.matrix.transpose.*`, does not
+    support scalable vectors. Hence, this Op is only available for fixed-width
+    vectors. Also see:
+
     Also see:
 
     http://llvm.org/docs/LangRef.html#llvm-matrix-transpose-intrinsic
diff --git a/mlir/test/Dialect/Vector/invalid.mlir b/mlir/test/Dialect/Vector/invalid.mlir
index 56039d04549aa5..d591c60acb64e7 100644
--- a/mlir/test/Dialect/Vector/invalid.mlir
+++ b/mlir/test/Dialect/Vector/invalid.mlir
@@ -1900,3 +1900,12 @@ func.func @matrix_multiply_scalable(%a: vector<[4]xf64>, %b: vector<4xf64>) {
 
   return
 }
+
+// -----
+
+func.func @flat_transpose_scalable(%arg0: vector<[16]xf32>) -> vector<[16]xf32> {
+  // expected-error @+1 {{'vector.flat_transpose' op operand #0 must be fixed-length vector of signless integer or signed integer or index or floating-point values of ranks 1, but got 'vector<[16]xf32>'}}
+  %0 = vector.flat_transpose %arg0 { rows = 4: i32, columns = 4: i32 }
+     : vector<[16]xf32> -> vector<[16]xf32>
+  return %0 : vector<[16]xf32>
+}

@llvmbot
Copy link
Member

llvmbot commented Nov 7, 2024

@llvm/pr-subscribers-llvm-analysis

Author: Andrzej Warzyński (banach-space)

Changes
  • [ValueTracking] Don't special case depth for phi of select (#114996)
  • [mlir][vector] Disable vector.flat_transpose for scalable vectors (#102573)

Full diff: https://github.com/llvm/llvm-project/pull/115338.diff

3 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+8-10)
  • (modified) mlir/include/mlir/Dialect/Vector/IR/VectorOps.td (+6-2)
  • (modified) mlir/test/Dialect/Vector/invalid.mlir (+9)
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 37cd4caaca71df..ed3fa35c5b8610 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -1565,20 +1565,12 @@ static void computeKnownBitsFromOperator(const Operator *I,
         // Skip direct self references.
         if (IncValue == P) continue;
 
-        // Recurse, but cap the recursion to one level, because we don't
-        // want to waste time spinning around in loops.
-        // TODO: See if we can base recursion limiter on number of incoming phi
-        // edges so we don't overly clamp analysis.
-        unsigned IncDepth = MaxAnalysisRecursionDepth - 1;
-
         // If the Use is a select of this phi, use the knownbit of the other
         // operand to break the recursion.
         if (auto *SI = dyn_cast<SelectInst>(IncValue)) {
-          if (SI->getTrueValue() == P || SI->getFalseValue() == P) {
+          if (SI->getTrueValue() == P || SI->getFalseValue() == P)
             IncValue = SI->getTrueValue() == P ? SI->getFalseValue()
                                                : SI->getTrueValue();
-            IncDepth = Depth + 1;
-          }
         }
 
         // Change the context instruction to the "edge" that flows into the
@@ -1589,7 +1581,13 @@ static void computeKnownBitsFromOperator(const Operator *I,
         RecQ.CxtI = P->getIncomingBlock(u)->getTerminator();
 
         Known2 = KnownBits(BitWidth);
-        computeKnownBits(IncValue, DemandedElts, Known2, IncDepth, RecQ);
+
+        // Recurse, but cap the recursion to one level, because we don't
+        // want to waste time spinning around in loops.
+        // TODO: See if we can base recursion limiter on number of incoming phi
+        // edges so we don't overly clamp analysis.
+        computeKnownBits(IncValue, DemandedElts, Known2,
+                         MaxAnalysisRecursionDepth - 1, RecQ);
 
         // See if we can further use a conditional branch into the phi
         // to help us determine the range of the value.
diff --git a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
index 3f45d0804e0450..6fe897cca79926 100644
--- a/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
+++ b/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
@@ -2770,11 +2770,11 @@ def Vector_FlatTransposeOp : Vector_Op<"flat_transpose", [Pure,
                  TCresVTEtIsSameAsOpBase<0, 0>>]>,
     Arguments<(
       // TODO: tighten vector element types that make sense.
-      ins VectorOfRankAndType<[1],
+      ins FixedVectorOfRankAndType<[1],
             [AnySignlessInteger, AnySignedInteger, Index, AnyFloat]>:$matrix,
           I32Attr:$rows, I32Attr:$columns)>,
     Results<(
-      outs VectorOfRankAndType<[1],
+      outs FixedVectorOfRankAndType<[1],
              [AnySignlessInteger, AnySignedInteger, Index, AnyFloat]>:$res)> {
   let summary = "Vector matrix transposition on flattened 1-D MLIR vectors";
   let description = [{
@@ -2789,6 +2789,10 @@ def Vector_FlatTransposeOp : Vector_Op<"flat_transpose", [Pure,
     a 2-D matrix with <rows> rows and <columns> columns, and returns the
     transposed matrix in flattened form in 'res'.
 
+    Note, the corresponding LLVM intrinsic, `@llvm.matrix.transpose.*`, does not
+    support scalable vectors. Hence, this Op is only available for fixed-width
+    vectors. Also see:
+
     Also see:
 
     http://llvm.org/docs/LangRef.html#llvm-matrix-transpose-intrinsic
diff --git a/mlir/test/Dialect/Vector/invalid.mlir b/mlir/test/Dialect/Vector/invalid.mlir
index 56039d04549aa5..d591c60acb64e7 100644
--- a/mlir/test/Dialect/Vector/invalid.mlir
+++ b/mlir/test/Dialect/Vector/invalid.mlir
@@ -1900,3 +1900,12 @@ func.func @matrix_multiply_scalable(%a: vector<[4]xf64>, %b: vector<4xf64>) {
 
   return
 }
+
+// -----
+
+func.func @flat_transpose_scalable(%arg0: vector<[16]xf32>) -> vector<[16]xf32> {
+  // expected-error @+1 {{'vector.flat_transpose' op operand #0 must be fixed-length vector of signless integer or signed integer or index or floating-point values of ranks 1, but got 'vector<[16]xf32>'}}
+  %0 = vector.flat_transpose %arg0 { rows = 4: i32, columns = 4: i32 }
+     : vector<[16]xf32> -> vector<[16]xf32>
+  return %0 : vector<[16]xf32>
+}

@banach-space banach-space marked this pull request as draft November 7, 2024 16:30
@banach-space banach-space force-pushed the andrzej/disable_flat_transpose_scalable branch from 2dc8ee0 to 3d6c62c Compare November 7, 2024 16:32
@banach-space banach-space changed the title andrzej/disable flat transpose scalable [mlir][vector] Disable vector.flat_transpose for scalable vectors Nov 7, 2024
@banach-space banach-space removed the request for review from nikic November 7, 2024 16:33
@banach-space banach-space marked this pull request as ready for review November 7, 2024 16:33
Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % documentation

…lvm#102573)

Disables `vector.flat_transpose` for scalable vectors. As per the docs:

>  This is the counterpart of llvm.matrix.transpose in MLIR

I'm not aware of any use of any matrix-multiply intrinsics in the
context of scalable vectors, hence disabling.

Note, this is a follow-on for llvm#102573 in which I disabled
`vector.matrix_multiply`.
@banach-space banach-space force-pushed the andrzej/disable_flat_transpose_scalable branch from f05e10d to c518e3b Compare November 7, 2024 18:22
@banach-space banach-space merged commit d6d73ec into llvm:main Nov 8, 2024
8 checks passed
@banach-space banach-space deleted the andrzej/disable_flat_transpose_scalable branch November 8, 2024 08:25
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
…vm#115338)

Disables `vector.flat_transpose` for scalable vectors. As per the docs:

>  This is the counterpart of llvm.matrix.transpose in MLIR

I'm not aware of any use of any matrix-multiply intrinsics in the
context of scalable vectors, hence disabling.

Note, this is a follow-on for llvm#102573 in which I disabled
`vector.matrix_multiply`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants