Skip to content

[mlir][nfc] Rename @genbool_* as @constant_mask_* #115335

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

Renames @genbool_* tests as @constant_mask_*. That's to better
highlight which Op is tested and for better consistency with other test.

In addition,@genbool_2d is moved above it's counterparts with
scalable vectors (again, for consistency).

@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][nfc] Rename @<!-- -->genbool_* as @<!-- -->constant_mask_*

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

2 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+8-10)
  • (modified) mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir (+29-29)
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/test/Conversion/VectorToLLVM/vector-to-llvm.mlir b/mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
index c1de24fd0403ce..8da4d351b02205 100644
--- a/mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
+++ b/mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
@@ -2685,61 +2685,77 @@ func.func @matrix_ops_index(%A: vector<64xindex>, %B: vector<48xindex>) -> vecto
 
 // -----
 
-func.func @genbool_0d_f() -> vector<i1> {
+func.func @constant_mask_0d_f() -> vector<i1> {
   %0 = vector.constant_mask [0] : vector<i1>
   return %0 : vector<i1>
 }
-// CHECK-LABEL: func @genbool_0d_f
+// CHECK-LABEL: func @constant_mask_0d_f
 // CHECK: %[[VAL_0:.*]] = arith.constant dense<false> : vector<i1>
 // CHECK: return %[[VAL_0]] : vector<i1>
 
 // -----
 
-func.func @genbool_0d_t() -> vector<i1> {
+func.func @constant_mask_0d_t() -> vector<i1> {
   %0 = vector.constant_mask [1] : vector<i1>
   return %0 : vector<i1>
 }
-// CHECK-LABEL: func @genbool_0d_t
+// CHECK-LABEL: func @constant_mask_0d_t
 // CHECK: %[[VAL_0:.*]] = arith.constant dense<true> : vector<i1>
 // CHECK: return %[[VAL_0]] : vector<i1>
 
 // -----
 
-func.func @genbool_1d() -> vector<8xi1> {
+func.func @constant_mask_1d() -> vector<8xi1> {
   %0 = vector.constant_mask [4] : vector<8xi1>
   return %0 : vector<8xi1>
 }
-// CHECK-LABEL: func @genbool_1d
+// CHECK-LABEL: func @constant_mask_1d
 // CHECK: %[[VAL_0:.*]] = arith.constant dense<[true, true, true, true, false, false, false, false]> : vector<8xi1>
 // CHECK: return %[[VAL_0]] : vector<8xi1>
 
 // -----
 
-func.func @genbool_1d_scalable_all_false() -> vector<[8]xi1> {
+func.func @constant_mask_1d_scalable_all_false() -> vector<[8]xi1> {
   %0 = vector.constant_mask [0] : vector<[8]xi1>
   return %0 : vector<[8]xi1>
 }
-// CHECK-LABEL: func @genbool_1d_scalable_all_false
+// CHECK-LABEL: func @constant_mask_1d_scalable_all_false
 // CHECK: %[[VAL_0:.*]] = arith.constant dense<false> : vector<[8]xi1>
 // CHECK: return %[[VAL_0]] : vector<[8]xi1>
 
 // -----
 
-func.func @genbool_1d_scalable_all_true() -> vector<[8]xi1> {
+func.func @constant_mask_1d_scalable_all_true() -> vector<[8]xi1> {
   %0 = vector.constant_mask [8] : vector<[8]xi1>
   return %0 : vector<[8]xi1>
 }
-// CHECK-LABEL: func @genbool_1d_scalable_all_true
+// CHECK-LABEL: func @constant_mask_1d_scalable_all_true
 // CHECK: %[[VAL_0:.*]] = arith.constant dense<true> : vector<[8]xi1>
 // CHECK: return %[[VAL_0]] : vector<[8]xi1>
 
 // -----
 
-func.func @genbool_2d_trailing_scalable() -> vector<4x[4]xi1> {
+func.func @constant_mask_2d() -> vector<4x4xi1> {
+  %v = vector.constant_mask [2, 2] : vector<4x4xi1>
+  return %v: vector<4x4xi1>
+}
+
+// CHECK-LABEL: func @constant_mask_2d
+// CHECK: %[[VAL_0:.*]] = arith.constant dense<[true, true, false, false]> : vector<4xi1>
+// CHECK: %[[VAL_1:.*]] = arith.constant dense<false> : vector<4x4xi1>
+// CHECK: %[[VAL_2:.*]] = builtin.unrealized_conversion_cast %[[VAL_1]] : vector<4x4xi1> to !llvm.array<4 x vector<4xi1>>
+// CHECK: %[[VAL_3:.*]] = llvm.insertvalue %[[VAL_0]], %[[VAL_2]][0] : !llvm.array<4 x vector<4xi1>>
+// CHECK: %[[VAL_4:.*]] = llvm.insertvalue %[[VAL_0]], %[[VAL_3]][1] : !llvm.array<4 x vector<4xi1>>
+// CHECK: %[[VAL_5:.*]] = builtin.unrealized_conversion_cast %[[VAL_4]] : !llvm.array<4 x vector<4xi1>> to vector<4x4xi1>
+// CHECK: return %[[VAL_5]] : vector<4x4xi1>
+
+// -----
+
+func.func @constant_mask_2d_trailing_scalable() -> vector<4x[4]xi1> {
   %0 = vector.constant_mask [2, 4] : vector<4x[4]xi1>
   return %0 : vector<4x[4]xi1>
 }
-// CHECK-LABEL:   func.func @genbool_2d_trailing_scalable
+// CHECK-LABEL:   func.func @constant_mask_2d_trailing_scalable
 // CHECK:           %[[VAL_0:.*]] = arith.constant dense<true> : vector<[4]xi1>
 // CHECK:           %[[VAL_1:.*]] = arith.constant dense<false> : vector<4x[4]xi1>
 // CHECK:           %[[VAL_2:.*]] = builtin.unrealized_conversion_cast %[[VAL_1]] : vector<4x[4]xi1> to !llvm.array<4 x vector<[4]xi1>>
@@ -2752,7 +2768,7 @@ func.func @genbool_2d_trailing_scalable() -> vector<4x[4]xi1> {
 
 /// Currently, this is not supported as generating the mask would require
 /// unrolling the leading scalable dimension at compile time.
-func.func @cannot_genbool_2d_leading_scalable() -> vector<[4]x4xi1> {
+func.func @cannot_constant_mask_2d_leading_scalable() -> vector<[4]x4xi1> {
   %0 = vector.constant_mask [4, 2] : vector<[4]x4xi1>
   return %0 : vector<[4]x4xi1>
 }
@@ -2762,22 +2778,6 @@ func.func @cannot_genbool_2d_leading_scalable() -> vector<[4]x4xi1> {
 
 // -----
 
-func.func @genbool_2d() -> vector<4x4xi1> {
-  %v = vector.constant_mask [2, 2] : vector<4x4xi1>
-  return %v: vector<4x4xi1>
-}
-
-// CHECK-LABEL: func @genbool_2d
-// CHECK: %[[VAL_0:.*]] = arith.constant dense<[true, true, false, false]> : vector<4xi1>
-// CHECK: %[[VAL_1:.*]] = arith.constant dense<false> : vector<4x4xi1>
-// CHECK: %[[VAL_2:.*]] = builtin.unrealized_conversion_cast %[[VAL_1]] : vector<4x4xi1> to !llvm.array<4 x vector<4xi1>>
-// CHECK: %[[VAL_3:.*]] = llvm.insertvalue %[[VAL_0]], %[[VAL_2]][0] : !llvm.array<4 x vector<4xi1>>
-// CHECK: %[[VAL_4:.*]] = llvm.insertvalue %[[VAL_0]], %[[VAL_3]][1] : !llvm.array<4 x vector<4xi1>>
-// CHECK: %[[VAL_5:.*]] = builtin.unrealized_conversion_cast %[[VAL_4]] : !llvm.array<4 x vector<4xi1>> to vector<4x4xi1>
-// CHECK: return %[[VAL_5]] : vector<4x4xi1>
-
-// -----
-
 func.func @create_mask_0d(%a : index) -> vector<i1> {
   %v = vector.create_mask %a : vector<i1>
   return %v: vector<i1>

@llvmbot
Copy link
Member

llvmbot commented Nov 7, 2024

@llvm/pr-subscribers-mlir

Author: Andrzej Warzyński (banach-space)

Changes
  • [ValueTracking] Don't special case depth for phi of select (#114996)
  • [mlir][nfc] Rename @<!-- -->genbool_* as @<!-- -->constant_mask_*

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

2 Files Affected:

  • (modified) llvm/lib/Analysis/ValueTracking.cpp (+8-10)
  • (modified) mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir (+29-29)
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/test/Conversion/VectorToLLVM/vector-to-llvm.mlir b/mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
index c1de24fd0403ce..8da4d351b02205 100644
--- a/mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
+++ b/mlir/test/Conversion/VectorToLLVM/vector-to-llvm.mlir
@@ -2685,61 +2685,77 @@ func.func @matrix_ops_index(%A: vector<64xindex>, %B: vector<48xindex>) -> vecto
 
 // -----
 
-func.func @genbool_0d_f() -> vector<i1> {
+func.func @constant_mask_0d_f() -> vector<i1> {
   %0 = vector.constant_mask [0] : vector<i1>
   return %0 : vector<i1>
 }
-// CHECK-LABEL: func @genbool_0d_f
+// CHECK-LABEL: func @constant_mask_0d_f
 // CHECK: %[[VAL_0:.*]] = arith.constant dense<false> : vector<i1>
 // CHECK: return %[[VAL_0]] : vector<i1>
 
 // -----
 
-func.func @genbool_0d_t() -> vector<i1> {
+func.func @constant_mask_0d_t() -> vector<i1> {
   %0 = vector.constant_mask [1] : vector<i1>
   return %0 : vector<i1>
 }
-// CHECK-LABEL: func @genbool_0d_t
+// CHECK-LABEL: func @constant_mask_0d_t
 // CHECK: %[[VAL_0:.*]] = arith.constant dense<true> : vector<i1>
 // CHECK: return %[[VAL_0]] : vector<i1>
 
 // -----
 
-func.func @genbool_1d() -> vector<8xi1> {
+func.func @constant_mask_1d() -> vector<8xi1> {
   %0 = vector.constant_mask [4] : vector<8xi1>
   return %0 : vector<8xi1>
 }
-// CHECK-LABEL: func @genbool_1d
+// CHECK-LABEL: func @constant_mask_1d
 // CHECK: %[[VAL_0:.*]] = arith.constant dense<[true, true, true, true, false, false, false, false]> : vector<8xi1>
 // CHECK: return %[[VAL_0]] : vector<8xi1>
 
 // -----
 
-func.func @genbool_1d_scalable_all_false() -> vector<[8]xi1> {
+func.func @constant_mask_1d_scalable_all_false() -> vector<[8]xi1> {
   %0 = vector.constant_mask [0] : vector<[8]xi1>
   return %0 : vector<[8]xi1>
 }
-// CHECK-LABEL: func @genbool_1d_scalable_all_false
+// CHECK-LABEL: func @constant_mask_1d_scalable_all_false
 // CHECK: %[[VAL_0:.*]] = arith.constant dense<false> : vector<[8]xi1>
 // CHECK: return %[[VAL_0]] : vector<[8]xi1>
 
 // -----
 
-func.func @genbool_1d_scalable_all_true() -> vector<[8]xi1> {
+func.func @constant_mask_1d_scalable_all_true() -> vector<[8]xi1> {
   %0 = vector.constant_mask [8] : vector<[8]xi1>
   return %0 : vector<[8]xi1>
 }
-// CHECK-LABEL: func @genbool_1d_scalable_all_true
+// CHECK-LABEL: func @constant_mask_1d_scalable_all_true
 // CHECK: %[[VAL_0:.*]] = arith.constant dense<true> : vector<[8]xi1>
 // CHECK: return %[[VAL_0]] : vector<[8]xi1>
 
 // -----
 
-func.func @genbool_2d_trailing_scalable() -> vector<4x[4]xi1> {
+func.func @constant_mask_2d() -> vector<4x4xi1> {
+  %v = vector.constant_mask [2, 2] : vector<4x4xi1>
+  return %v: vector<4x4xi1>
+}
+
+// CHECK-LABEL: func @constant_mask_2d
+// CHECK: %[[VAL_0:.*]] = arith.constant dense<[true, true, false, false]> : vector<4xi1>
+// CHECK: %[[VAL_1:.*]] = arith.constant dense<false> : vector<4x4xi1>
+// CHECK: %[[VAL_2:.*]] = builtin.unrealized_conversion_cast %[[VAL_1]] : vector<4x4xi1> to !llvm.array<4 x vector<4xi1>>
+// CHECK: %[[VAL_3:.*]] = llvm.insertvalue %[[VAL_0]], %[[VAL_2]][0] : !llvm.array<4 x vector<4xi1>>
+// CHECK: %[[VAL_4:.*]] = llvm.insertvalue %[[VAL_0]], %[[VAL_3]][1] : !llvm.array<4 x vector<4xi1>>
+// CHECK: %[[VAL_5:.*]] = builtin.unrealized_conversion_cast %[[VAL_4]] : !llvm.array<4 x vector<4xi1>> to vector<4x4xi1>
+// CHECK: return %[[VAL_5]] : vector<4x4xi1>
+
+// -----
+
+func.func @constant_mask_2d_trailing_scalable() -> vector<4x[4]xi1> {
   %0 = vector.constant_mask [2, 4] : vector<4x[4]xi1>
   return %0 : vector<4x[4]xi1>
 }
-// CHECK-LABEL:   func.func @genbool_2d_trailing_scalable
+// CHECK-LABEL:   func.func @constant_mask_2d_trailing_scalable
 // CHECK:           %[[VAL_0:.*]] = arith.constant dense<true> : vector<[4]xi1>
 // CHECK:           %[[VAL_1:.*]] = arith.constant dense<false> : vector<4x[4]xi1>
 // CHECK:           %[[VAL_2:.*]] = builtin.unrealized_conversion_cast %[[VAL_1]] : vector<4x[4]xi1> to !llvm.array<4 x vector<[4]xi1>>
@@ -2752,7 +2768,7 @@ func.func @genbool_2d_trailing_scalable() -> vector<4x[4]xi1> {
 
 /// Currently, this is not supported as generating the mask would require
 /// unrolling the leading scalable dimension at compile time.
-func.func @cannot_genbool_2d_leading_scalable() -> vector<[4]x4xi1> {
+func.func @cannot_constant_mask_2d_leading_scalable() -> vector<[4]x4xi1> {
   %0 = vector.constant_mask [4, 2] : vector<[4]x4xi1>
   return %0 : vector<[4]x4xi1>
 }
@@ -2762,22 +2778,6 @@ func.func @cannot_genbool_2d_leading_scalable() -> vector<[4]x4xi1> {
 
 // -----
 
-func.func @genbool_2d() -> vector<4x4xi1> {
-  %v = vector.constant_mask [2, 2] : vector<4x4xi1>
-  return %v: vector<4x4xi1>
-}
-
-// CHECK-LABEL: func @genbool_2d
-// CHECK: %[[VAL_0:.*]] = arith.constant dense<[true, true, false, false]> : vector<4xi1>
-// CHECK: %[[VAL_1:.*]] = arith.constant dense<false> : vector<4x4xi1>
-// CHECK: %[[VAL_2:.*]] = builtin.unrealized_conversion_cast %[[VAL_1]] : vector<4x4xi1> to !llvm.array<4 x vector<4xi1>>
-// CHECK: %[[VAL_3:.*]] = llvm.insertvalue %[[VAL_0]], %[[VAL_2]][0] : !llvm.array<4 x vector<4xi1>>
-// CHECK: %[[VAL_4:.*]] = llvm.insertvalue %[[VAL_0]], %[[VAL_3]][1] : !llvm.array<4 x vector<4xi1>>
-// CHECK: %[[VAL_5:.*]] = builtin.unrealized_conversion_cast %[[VAL_4]] : !llvm.array<4 x vector<4xi1>> to vector<4x4xi1>
-// CHECK: return %[[VAL_5]] : vector<4x4xi1>
-
-// -----
-
 func.func @create_mask_0d(%a : index) -> vector<i1> {
   %v = vector.create_mask %a : vector<i1>
   return %v: vector<i1>

Renames `@genbool_*` tests as `@constant_mask_*`. That's to better
highlight which Op is tested and for better consistency with other test.

In addition,`@genbool_2d` is moved _above_ it's counterparts with
scalable vectors (again, for consistency).
@banach-space banach-space force-pushed the andrzej/extend_vector_to_llvm_test_9 branch from 55b5a61 to 55762de Compare November 7, 2024 16:35
@banach-space banach-space removed the request for review from nikic November 7, 2024 16:35
@banach-space banach-space changed the title andrzej/extend vector to llvm test 9 [mlir][nfc] Rename @genbool_* as @constant_mask_* Nov 7, 2024
// CHECK-LABEL: func @constant_mask_2d
// CHECK: %[[VAL_0:.*]] = arith.constant dense<[true, true, false, false]> : vector<4xi1>
// CHECK: %[[VAL_1:.*]] = arith.constant dense<false> : vector<4x4xi1>
// CHECK: %[[VAL_2:.*]] = builtin.unrealized_conversion_cast %[[VAL_1]] : vector<4x4xi1> to !llvm.array<4 x vector<4xi1>>
Copy link
Contributor

Choose a reason for hiding this comment

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

just for my info - why convert-to-llvm results in unrealized_conversion_cast? I see it present everywhere in this file so its not a blocker for this diff AFAIU.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some things are lowered to a mix of Arith and LLVM, so without -convert-arith-to-llvm ... But there will be a lot of other nuances as well that I don't know yet 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for clarifying.

Copy link
Contributor

@dcaballe dcaballe left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup!

@banach-space
Copy link
Contributor Author

@javedabsar1 Are you OK with this change?

@javedabsar1
Copy link
Contributor

@javedabsar1 Are you OK with this change?

yes of course. Sorry i did not get back earlier.

@javedabsar1 javedabsar1 self-requested a review November 8, 2024 13:18
@banach-space banach-space merged commit 844fe8f into llvm:main Nov 8, 2024
8 checks passed
Groverkss pushed a commit to iree-org/llvm-project that referenced this pull request Nov 15, 2024
Renames `@genbool_*` tests as `@constant_mask_*`. That's to better
highlight which Op is tested and for better consistency with other test.

In addition,`@genbool_2d` is moved _above_ it's counterparts with
scalable vectors (again, for consistency).
@banach-space banach-space deleted the andrzej/extend_vector_to_llvm_test_9 branch November 22, 2024 08:52
@Saleh5515

This comment has been minimized.

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