Skip to content
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

[mlir][Interfaces][NFC] Add TableGen test op for value bounds tests #88717

Merged
merged 2 commits into from
Apr 15, 2024

Conversation

matthias-springer
Copy link
Member

This commit is a code cleanup. It defines the test ops the are used for the ValueBoundsOpInterface tests in TableGen, along with proper verifiers.

@llvmbot
Copy link
Collaborator

llvmbot commented Apr 15, 2024

@llvm/pr-subscribers-mlir-affine
@llvm/pr-subscribers-mlir-vector
@llvm/pr-subscribers-mlir-tensor

@llvm/pr-subscribers-mlir

Author: Matthias Springer (matthias-springer)

Changes

This commit is a code cleanup. It defines the test ops the are used for the ValueBoundsOpInterface tests in TableGen, along with proper verifiers.


Patch is 28.85 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/88717.diff

11 Files Affected:

  • (modified) mlir/test/Dialect/Affine/value-bounds-op-interface-impl.mlir (+1-1)
  • (modified) mlir/test/Dialect/Affine/value-bounds-reification.mlir (+3-3)
  • (modified) mlir/test/Dialect/Tensor/value-bounds-op-interface-impl.mlir (+2-2)
  • (modified) mlir/test/Dialect/Vector/test-scalable-bounds.mlir (+9-9)
  • (modified) mlir/test/lib/Dialect/Affine/CMakeLists.txt (+1)
  • (modified) mlir/test/lib/Dialect/Affine/TestReifyValueBounds.cpp (+104-186)
  • (modified) mlir/test/lib/Dialect/Test/CMakeLists.txt (+1)
  • (modified) mlir/test/lib/Dialect/Test/TestDialect.cpp (+48)
  • (modified) mlir/test/lib/Dialect/Test/TestDialect.h (+1)
  • (modified) mlir/test/lib/Dialect/Test/TestOps.td (+47)
  • (modified) utils/bazel/llvm-project-overlay/mlir/test/BUILD.bazel (+2)
diff --git a/mlir/test/Dialect/Affine/value-bounds-op-interface-impl.mlir b/mlir/test/Dialect/Affine/value-bounds-op-interface-impl.mlir
index 10da91870f49d9..23c6872dcebe94 100644
--- a/mlir/test/Dialect/Affine/value-bounds-op-interface-impl.mlir
+++ b/mlir/test/Dialect/Affine/value-bounds-op-interface-impl.mlir
@@ -74,7 +74,7 @@ func.func @composed_affine_apply(%i1 : index) -> (index) {
   %i2 = affine.apply affine_map<(d0) -> ((d0 floordiv 32) * 16)>(%i1)
   %i3 = affine.apply affine_map<(d0) -> ((d0 floordiv 32) * 16 + 8)>(%i1)
   %s = affine.apply affine_map<()[s0, s1] -> (s0 - s1)>()[%i2, %i3]
-  %reified = "test.reify_constant_bound"(%s) {type = "EQ"} : (index) -> (index)
+  %reified = "test.reify_bound"(%s) {type = "EQ", constant} : (index) -> (index)
   return %reified : index
 }
 
diff --git a/mlir/test/Dialect/Affine/value-bounds-reification.mlir b/mlir/test/Dialect/Affine/value-bounds-reification.mlir
index 909c9098c51607..75622f59af83be 100644
--- a/mlir/test/Dialect/Affine/value-bounds-reification.mlir
+++ b/mlir/test/Dialect/Affine/value-bounds-reification.mlir
@@ -47,7 +47,7 @@ func.func @reify_slice_bound(%t: tensor<?x?xi32>, %idx: index, %ub: index, %f: f
     %bound = "test.reify_bound"(%filled) {dim = 1, type = "UB"} : (tensor<1x?xi32>) -> (index)
     "test.some_use"(%bound) : (index) -> ()
 
-    %bound_const = "test.reify_constant_bound"(%filled) {dim = 1, type = "UB"} : (tensor<1x?xi32>) -> (index)
+    %bound_const = "test.reify_bound"(%filled) {dim = 1, type = "UB", constant} : (tensor<1x?xi32>) -> (index)
     "test.some_use"(%bound_const) : (index) -> ()
   }
   return
@@ -93,7 +93,7 @@ func.func @reify_slice_bound2(%lb0: index, %ub0: index, %step0: index,
 
     // CHECK: %[[c129:.*]] = arith.constant 129 : index
     // CHECK: "test.some_use"(%[[c129]])
-    %lb1_ub_const = "test.reify_constant_bound"(%lb1) {type = "UB"} : (index) -> (index)
+    %lb1_ub_const = "test.reify_bound"(%lb1) {type = "UB", constant} : (index) -> (index)
     "test.some_use"(%lb1_ub_const) : (index) -> ()
 
     scf.for %iv1 = %lb1 to %ub1 step %c32 {
@@ -116,7 +116,7 @@ func.func @reify_slice_bound2(%lb0: index, %ub0: index, %step0: index,
 
         // CHECK: %[[c32:.*]] = arith.constant 32 : index
         // CHECK: "test.some_use"(%[[c32]])
-        %matmul_ub_const = "test.reify_constant_bound"(%matmul) {dim = 1, type = "UB"} : (tensor<1x?xi32>) -> (index)
+        %matmul_ub_const = "test.reify_bound"(%matmul) {dim = 1, type = "UB", constant} : (tensor<1x?xi32>) -> (index)
         "test.some_use"(%matmul_ub_const) : (index) -> ()
       }
     }
diff --git a/mlir/test/Dialect/Tensor/value-bounds-op-interface-impl.mlir b/mlir/test/Dialect/Tensor/value-bounds-op-interface-impl.mlir
index 0c90bcdb42028c..0ba9983723a0a1 100644
--- a/mlir/test/Dialect/Tensor/value-bounds-op-interface-impl.mlir
+++ b/mlir/test/Dialect/Tensor/value-bounds-op-interface-impl.mlir
@@ -83,7 +83,7 @@ func.func @extract_slice_static(%t: tensor<?xf32>) -> index {
 func.func @extract_slice_dynamic_constant(%t: tensor<?xf32>, %sz: index) -> index {
   %0 = tensor.extract_slice %t[2][%sz][1] : tensor<?xf32> to tensor<?xf32>
   // expected-error @below{{could not reify bound}}
-  %1 = "test.reify_constant_bound"(%0) {dim = 0} : (tensor<?xf32>) -> (index)
+  %1 = "test.reify_bound"(%0) {dim = 0, constant} : (tensor<?xf32>) -> (index)
   return %1 : index
 }
 
@@ -95,7 +95,7 @@ func.func @extract_slice_dynamic_constant(%t: tensor<?xf32>, %sz: index) -> inde
 //       CHECK:   return %[[c5]]
 func.func @extract_slice_static_constant(%t: tensor<?xf32>) -> index {
   %0 = tensor.extract_slice %t[2][5][1] : tensor<?xf32> to tensor<5xf32>
-  %1 = "test.reify_constant_bound"(%0) {dim = 0} : (tensor<5xf32>) -> (index)
+  %1 = "test.reify_bound"(%0) {dim = 0, constant} : (tensor<5xf32>) -> (index)
   return %1 : index
 }
 
diff --git a/mlir/test/Dialect/Vector/test-scalable-bounds.mlir b/mlir/test/Dialect/Vector/test-scalable-bounds.mlir
index 245a6f5c13ac3d..d549c5bd1c3785 100644
--- a/mlir/test/Dialect/Vector/test-scalable-bounds.mlir
+++ b/mlir/test/Dialect/Vector/test-scalable-bounds.mlir
@@ -26,8 +26,8 @@ func.func @fixed_size_loop_nest() {
     %min_i = affine.min #map_dim_i(%i)[%c4_vscale]
     scf.for %j = %c0 to %c16 step %c4_vscale {
       %min_j = affine.min #map_dim_j(%j)[%c4_vscale]
-      %bound_i = "test.reify_scalable_bound"(%min_i) {type = "UB", vscale_min = 1, vscale_max = 16} : (index) -> index
-      %bound_j = "test.reify_scalable_bound"(%min_j) {type = "UB", vscale_min = 1, vscale_max = 16} : (index) -> index
+      %bound_i = "test.reify_bound"(%min_i) {type = "UB", vscale_min = 1, vscale_max = 16, scalable} : (index) -> index
+      %bound_j = "test.reify_bound"(%min_j) {type = "UB", vscale_min = 1, vscale_max = 16, scalable} : (index) -> index
       "test.some_use"(%bound_i, %bound_j) : (index, index) -> ()
     }
   }
@@ -58,8 +58,8 @@ func.func @dynamic_size_loop_nest(%dim0: index, %dim1: index) {
     %min_i = affine.min #map_dynamic_dim(%i)[%c4_vscale, %dim0]
     scf.for %j = %c0 to %dim1 step %c4_vscale {
       %min_j = affine.min #map_dynamic_dim(%j)[%c4_vscale, %dim1]
-      %bound_i = "test.reify_scalable_bound"(%min_i) {type = "UB", vscale_min = 1, vscale_max = 16} : (index) -> index
-      %bound_j = "test.reify_scalable_bound"(%min_j) {type = "UB", vscale_min = 1, vscale_max = 16} : (index) -> index
+      %bound_i = "test.reify_bound"(%min_i) {type = "UB", vscale_min = 1, vscale_max = 16, scalable} : (index) -> index
+      %bound_j = "test.reify_bound"(%min_j) {type = "UB", vscale_min = 1, vscale_max = 16, scalable} : (index) -> index
       "test.some_use"(%bound_i, %bound_j) : (index, index) -> ()
     }
   }
@@ -80,7 +80,7 @@ func.func @add_to_vscale() {
   %vscale = vector.vscale
   %c8 = arith.constant 8 : index
   %vscale_plus_c8 = arith.addi %vscale, %c8 : index
-  %bound = "test.reify_scalable_bound"(%vscale_plus_c8) {type = "EQ", vscale_min = 1, vscale_max = 16} : (index) -> index
+  %bound = "test.reify_bound"(%vscale_plus_c8) {type = "EQ", vscale_min = 1, vscale_max = 16, scalable} : (index) -> index
   "test.some_use"(%bound) : (index) -> ()
   return
 }
@@ -94,7 +94,7 @@ func.func @add_to_vscale() {
 //       CHECK:   "test.some_use"(%[[C2]]) : (index) -> ()
 func.func @vscale_fixed_size() {
   %vscale = vector.vscale
-  %bound = "test.reify_scalable_bound"(%vscale) {type = "EQ", vscale_min = 2, vscale_max = 2} : (index) -> index
+  %bound = "test.reify_bound"(%vscale) {type = "EQ", vscale_min = 2, vscale_max = 2, scalable} : (index) -> index
   "test.some_use"(%bound) : (index) -> ()
   return
 }
@@ -107,7 +107,7 @@ func.func @unknown_bound(%a: index) {
   %vscale = vector.vscale
   %vscale_plus_a = arith.muli %vscale, %a : index
   // expected-error @below{{could not reify bound}}
-  %bound = "test.reify_scalable_bound"(%vscale_plus_a) {type = "UB", vscale_min = 1, vscale_max = 16} : (index) -> index
+  %bound = "test.reify_bound"(%vscale_plus_a) {type = "UB", vscale_min = 1, vscale_max = 16, scalable} : (index) -> index
   "test.some_use"(%bound) : (index) -> ()
   return
 }
@@ -134,7 +134,7 @@ func.func @duplicate_vscale_values() {
   %c2_vscale = arith.muli %vscale_1, %c2 : index
   %add = arith.addi %c2_vscale, %c4_vscale : index
 
-  %bound = "test.reify_scalable_bound"(%add) {type = "EQ", vscale_min = 1, vscale_max = 16} : (index) -> index
+  %bound = "test.reify_bound"(%add) {type = "EQ", vscale_min = 1, vscale_max = 16, scalable} : (index) -> index
   "test.some_use"(%bound) : (index) -> ()
   return
 }
@@ -154,7 +154,7 @@ func.func @non_scalable_code() {
   %c0 = arith.constant 0 : index
   scf.for %i = %c0 to %c1024 step %c4 {
     %min_i = affine.min #map_dim_i(%i)[%c4]
-    %bound_i = "test.reify_scalable_bound"(%min_i) {type = "UB", vscale_min = 1, vscale_max = 16} : (index) -> index
+    %bound_i = "test.reify_bound"(%min_i) {type = "UB", vscale_min = 1, vscale_max = 16, scalable} : (index) -> index
     "test.some_use"(%bound_i) : (index) -> ()
   }
   return
diff --git a/mlir/test/lib/Dialect/Affine/CMakeLists.txt b/mlir/test/lib/Dialect/Affine/CMakeLists.txt
index 14960a45d39bab..a8af7285573456 100644
--- a/mlir/test/lib/Dialect/Affine/CMakeLists.txt
+++ b/mlir/test/lib/Dialect/Affine/CMakeLists.txt
@@ -30,5 +30,6 @@ add_mlir_library(MLIRAffineTransformsTestPasses
   MLIRSupport
   MLIRMemRefDialect
   MLIRTensorDialect
+  MLIRTestDialect
   MLIRVectorUtils
   )
diff --git a/mlir/test/lib/Dialect/Affine/TestReifyValueBounds.cpp b/mlir/test/lib/Dialect/Affine/TestReifyValueBounds.cpp
index f38631054fb3c1..d6f8c5dabaa49d 100644
--- a/mlir/test/lib/Dialect/Affine/TestReifyValueBounds.cpp
+++ b/mlir/test/lib/Dialect/Affine/TestReifyValueBounds.cpp
@@ -6,6 +6,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "TestDialect.h"
 #include "mlir/Dialect/Affine/IR/AffineOps.h"
 #include "mlir/Dialect/Affine/IR/ValueBoundsOpInterfaceImpl.h"
 #include "mlir/Dialect/Affine/Transforms/Transforms.h"
@@ -57,16 +58,6 @@ struct TestReifyValueBounds
 
 } // namespace
 
-static FailureOr<BoundType> parseBoundType(const std::string &type) {
-  if (type == "EQ")
-    return BoundType::EQ;
-  if (type == "LB")
-    return BoundType::LB;
-  if (type == "UB")
-    return BoundType::UB;
-  return failure();
-}
-
 static FailureOr<ValueBoundsConstraintSet::ComparisonOperator>
 parseComparisonOperator(const std::string &type) {
   if (type == "EQ")
@@ -101,144 +92,89 @@ static LogicalResult testReifyValueBounds(func::FuncOp funcOp,
                                           bool reifyToFuncArgs,
                                           bool useArithOps) {
   IRRewriter rewriter(funcOp.getContext());
-  WalkResult result = funcOp.walk([&](Operation *op) {
-    // Look for test.reify_bound ops.
-    if (op->getName().getStringRef() == "test.reify_bound" ||
-        op->getName().getStringRef() == "test.reify_constant_bound" ||
-        op->getName().getStringRef() == "test.reify_scalable_bound") {
-      if (op->getNumOperands() != 1 || op->getNumResults() != 1 ||
-          !op->getResultTypes()[0].isIndex()) {
-        op->emitOpError("invalid op");
-        return WalkResult::skip();
-      }
-      Value value = op->getOperand(0);
-      if (isa<IndexType>(value.getType()) !=
-          !op->hasAttrOfType<IntegerAttr>("dim")) {
-        // Op should have "dim" attribute if and only if the operand is an
-        // index-typed value.
-        op->emitOpError("invalid op");
-        return WalkResult::skip();
-      }
-
-      // Get bound type.
-      std::string boundTypeStr = "EQ";
-      if (auto boundTypeAttr = op->getAttrOfType<StringAttr>("type"))
-        boundTypeStr = boundTypeAttr.str();
-      auto boundType = parseBoundType(boundTypeStr);
-      if (failed(boundType)) {
-        op->emitOpError("invalid op");
-        return WalkResult::interrupt();
-      }
-
-      // Get shape dimension (if any).
-      auto dim = value.getType().isIndex()
-                     ? std::nullopt
-                     : std::make_optional<int64_t>(
-                           op->getAttrOfType<IntegerAttr>("dim").getInt());
-
-      // Check if a constant was requested.
-      bool constant =
-          op->getName().getStringRef() == "test.reify_constant_bound";
-
-      bool scalable = !constant && op->getName().getStringRef() ==
-                                       "test.reify_scalable_bound";
-
-      // Prepare stop condition. By default, reify in terms of the op's
-      // operands. No stop condition is used when a constant was requested.
-      std::function<bool(Value, std::optional<int64_t>,
-                         ValueBoundsConstraintSet & cstr)>
-          stopCondition = [&](Value v, std::optional<int64_t> d,
-                              ValueBoundsConstraintSet &cstr) {
-            // Reify in terms of SSA values that are different from `value`.
-            return v != value;
-          };
-      if (reifyToFuncArgs) {
-        // Reify in terms of function block arguments.
-        stopCondition = [](Value v, std::optional<int64_t> d,
-                           ValueBoundsConstraintSet &cstr) {
-          auto bbArg = dyn_cast<BlockArgument>(v);
-          if (!bbArg)
-            return false;
-          return isa<FunctionOpInterface>(
-              bbArg.getParentBlock()->getParentOp());
+  WalkResult result = funcOp.walk([&](test::ReifyBoundOp op) {
+    auto boundType = op.getBoundType();
+    Value value = op.getVar();
+    std::optional<int64_t> dim = op.getDim();
+    bool constant = op.getConstant();
+    bool scalable = op.getScalable();
+
+    // Prepare stop condition. By default, reify in terms of the op's
+    // operands. No stop condition is used when a constant was requested.
+    std::function<bool(Value, std::optional<int64_t>,
+                       ValueBoundsConstraintSet & cstr)>
+        stopCondition = [&](Value v, std::optional<int64_t> d,
+                            ValueBoundsConstraintSet &cstr) {
+          // Reify in terms of SSA values that are different from `value`.
+          return v != value;
         };
-      }
+    if (reifyToFuncArgs) {
+      // Reify in terms of function block arguments.
+      stopCondition = [](Value v, std::optional<int64_t> d,
+                         ValueBoundsConstraintSet &cstr) {
+        auto bbArg = dyn_cast<BlockArgument>(v);
+        if (!bbArg)
+          return false;
+        return isa<FunctionOpInterface>(bbArg.getParentBlock()->getParentOp());
+      };
+    }
 
-      // Reify value bound
-      rewriter.setInsertionPointAfter(op);
-      FailureOr<OpFoldResult> reified = failure();
-      if (constant) {
-        auto reifiedConst = ValueBoundsConstraintSet::computeConstantBound(
-            *boundType, value, dim, /*stopCondition=*/nullptr);
-        if (succeeded(reifiedConst))
-          reified =
-              FailureOr<OpFoldResult>(rewriter.getIndexAttr(*reifiedConst));
-      } else if (scalable) {
-        unsigned vscaleMin = 0;
-        unsigned vscaleMax = 0;
-        if (auto attr = "vscale_min"; op->hasAttrOfType<IntegerAttr>(attr)) {
-          vscaleMin = unsigned(op->getAttrOfType<IntegerAttr>(attr).getInt());
-        } else {
-          op->emitOpError("expected `vscale_min` to be provided");
-          return WalkResult::skip();
+    // Reify value bound
+    rewriter.setInsertionPointAfter(op);
+    FailureOr<OpFoldResult> reified = failure();
+    if (constant) {
+      auto reifiedConst = ValueBoundsConstraintSet::computeConstantBound(
+          boundType, value, dim, /*stopCondition=*/nullptr);
+      if (succeeded(reifiedConst))
+        reified = FailureOr<OpFoldResult>(rewriter.getIndexAttr(*reifiedConst));
+    } else if (scalable) {
+      auto loc = op->getLoc();
+      auto reifiedScalable =
+          vector::ScalableValueBoundsConstraintSet::computeScalableBound(
+              value, dim, *op.getVscaleMin(), *op.getVscaleMax(), boundType);
+      if (succeeded(reifiedScalable)) {
+        SmallVector<std::pair<Value, std::optional<int64_t>>, 1> vscaleOperand;
+        if (reifiedScalable->map.getNumInputs() == 1) {
+          // The only possible input to the bound is vscale.
+          vscaleOperand.push_back(std::make_pair(
+              rewriter.create<vector::VectorScaleOp>(loc), std::nullopt));
         }
-        if (auto attr = "vscale_max"; op->hasAttrOfType<IntegerAttr>(attr)) {
-          vscaleMax = unsigned(op->getAttrOfType<IntegerAttr>(attr).getInt());
+        reified = affine::materializeComputedBound(
+            rewriter, loc, reifiedScalable->map, vscaleOperand);
+      }
+    } else {
+      if (dim) {
+        if (useArithOps) {
+          reified = arith::reifyShapedValueDimBound(
+              rewriter, op->getLoc(), boundType, value, *dim, stopCondition);
         } else {
-          op->emitOpError("expected `vscale_max` to be provided");
-          return WalkResult::skip();
-        }
-
-        auto loc = op->getLoc();
-        auto reifiedScalable =
-            vector::ScalableValueBoundsConstraintSet::computeScalableBound(
-                value, dim, vscaleMin, vscaleMax, *boundType);
-        if (succeeded(reifiedScalable)) {
-          SmallVector<std::pair<Value, std::optional<int64_t>>, 1>
-              vscaleOperand;
-          if (reifiedScalable->map.getNumInputs() == 1) {
-            // The only possible input to the bound is vscale.
-            vscaleOperand.push_back(std::make_pair(
-                rewriter.create<vector::VectorScaleOp>(loc), std::nullopt));
-          }
-          reified = affine::materializeComputedBound(
-              rewriter, loc, reifiedScalable->map, vscaleOperand);
+          reified = reifyShapedValueDimBound(rewriter, op->getLoc(), boundType,
+                                             value, *dim, stopCondition);
         }
       } else {
-        if (dim) {
-          if (useArithOps) {
-            reified = arith::reifyShapedValueDimBound(
-                rewriter, op->getLoc(), *boundType, value, *dim, stopCondition);
-          } else {
-            reified = reifyShapedValueDimBound(
-                rewriter, op->getLoc(), *boundType, value, *dim, stopCondition);
-          }
+        if (useArithOps) {
+          reified = arith::reifyIndexValueBound(
+              rewriter, op->getLoc(), boundType, value, stopCondition);
         } else {
-          if (useArithOps) {
-            reified = arith::reifyIndexValueBound(
-                rewriter, op->getLoc(), *boundType, value, stopCondition);
-          } else {
-            reified = reifyIndexValueBound(rewriter, op->getLoc(), *boundType,
-                                           value, stopCondition);
-          }
+          reified = reifyIndexValueBound(rewriter, op->getLoc(), boundType,
+                                         value, stopCondition);
         }
       }
-      if (failed(reified)) {
-        op->emitOpError("could not reify bound");
-        return WalkResult::interrupt();
-      }
+    }
+    if (failed(reified)) {
+      op->emitOpError("could not reify bound");
+      return WalkResult::interrupt();
+    }
 
-      // Replace the op with the reified bound.
-      if (auto val = llvm::dyn_cast_if_present<Value>(*reified)) {
-        rewriter.replaceOp(op, val);
-        return WalkResult::skip();
-      }
-      Value constOp = rewriter.create<arith::ConstantIndexOp>(
-          op->getLoc(), cast<IntegerAttr>(reified->get<Attribute>()).getInt());
-      rewriter.replaceOp(op, constOp);
+    // Replace the op with the reified bound.
+    if (auto val = llvm::dyn_cast_if_present<Value>(*reified)) {
+      rewriter.replaceOp(op, val);
       return WalkResult::skip();
     }
-    return WalkResult::advance();
+    Value constOp = rewriter.create<arith::ConstantIndexOp>(
+        op->getLoc(), cast<IntegerAttr>(reified->get<Attribute>()).getInt());
+    rewriter.replaceOp(op, constOp);
+    return WalkResult::skip();
   });
   return failure(result.wasInterrupted());
 }
@@ -246,60 +182,42 @@ static LogicalResult testReifyValueBounds(func::FuncOp funcOp,
 /// Look for "test.compare" ops and emit errors/remarks.
 static LogicalResult testEquality(func::FuncOp funcOp) {
   IRRewriter rewriter(funcOp.getContext());
-  WalkResult result = funcOp.walk([&](Operation *op) {
-    // Look for test.compare ops.
-    if (op->getName().getStringRef() == "test.compare") {
-      if (op->getNumOperands() != 2 || !op->getOperand(0).getType().isIndex() ||
-          !op->getOperand(1).getType().isIndex()) {
-        op->emitOpError("invalid op");
-        return WalkResult::skip();
-      }
-
-      // Get comparison operator.
-      std::string cmpStr = "EQ";
-      if (auto cmpAttr = op->getAttrOfType<StringAttr>("cmp"))
-        cmpStr = cmpAttr.str();
-      auto cmpType = parseComparisonOperator(cmpStr);
-      if (failed(cmpType)) {
-        op->emitOpError("invalid comparison operator");
+  WalkResult result = funcOp.walk([&](test::CompareOp op) {
+    auto cmpType = op.getComparisonOperator();
+    if (op.getCompose()) {
+      if (cmpType != ValueBoundsConstraintSet::EQ) {
+        op->emitOpError(
+            "comparison operator must be EQ when 'composed' is specified");
         return WalkResult::interrupt();
       }
-
-      if (op->hasAttr("compose")) {
-        if (cmpType != ValueBoundsConstraintSet::EQ) {
-  ...
[truncated]

This commit is a code cleanup. It defines the test ops the are used for the `ValueBoundsOpInterface` tests in TableGen, along with proper verifiers.
@matthias-springer matthias-springer force-pushed the users/matthias-springer/value_bounds_test_op branch from b59af1d to bc3b298 Compare April 15, 2024 13:13
Copy link
Member

@MacDue MacDue left a comment

Choose a reason for hiding this comment

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

LGTM, seems like a decent improvement.
I'd maybe make enums for the bound/comparator, though I think a StrAttr is fine for test ops.

mlir/test/lib/Dialect/Test/TestOps.td Outdated Show resolved Hide resolved
Co-authored-by: Benjamin Maxwell <benjamin.maxwell@arm.com>
@matthias-springer
Copy link
Member Author

I'd maybe make enums for the bound/comparator, though I think a StrAttr is fine for test ops.

I thought about that but many test cases would have to be updated because the syntax changes slightly.

@matthias-springer matthias-springer merged commit f8d314f into main Apr 15, 2024
3 of 4 checks passed
@matthias-springer matthias-springer deleted the users/matthias-springer/value_bounds_test_op branch April 15, 2024 16:14
aniplcc pushed a commit to aniplcc/llvm-project that referenced this pull request Apr 15, 2024
…lvm#88717)

This commit is a code cleanup. It defines the test ops the are used for
the `ValueBoundsOpInterface` tests in TableGen, along with proper
verifiers.

---------

Co-authored-by: Benjamin Maxwell <benjamin.maxwell@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel mlir:affine mlir:tensor mlir:vector mlir:vectorops mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants