[Mlir-commits] [mlir] e502f4f - [mlir][Arith] Remove expansions of integer min and max ops

[Krzysztof Drewniak]

Author: Krzysztof Drewniak
Date: 2023-01-06T20:32:29Z
New Revision: e502f4fc2e25eedaca6bd8c1eacc6e0365ef403a

URL: https://github.com/llvm/llvm-project/commit/e502f4fc2e25eedaca6bd8c1eacc6e0365ef403a
DIFF: https://github.com/llvm/llvm-project/commit/e502f4fc2e25eedaca6bd8c1eacc6e0365ef403a.diff

LOG: [mlir][Arith] Remove expansions of integer min and max ops

As of several months ago, both ArithToLLVM and ArithToSPIRV have
native support for integer min and max operations. Since these are all
the targets available in MLIR core, the need to "expand" arith.minui,
arith.minsi, arith,maxsi, and arith.manxui to more primitive
operations is to longer present.

Therefore, the expanding of integer min and max operations in Arith,
while correct, is likely to lead to performance loss by way of
misoptimization further down the line, and is no longer needed for
anyone's correctness.

This change may break downstream tests, but will not affect the
semantics of MLIR programs.

arith.minf and arith.maxf have a lot of underlying complexity due to
the many different possible NaN and signed zero semantics available on
various platforms, and so removing their expansion is left to a future
commit.

Reviewed By: ThomasRaoux, Mogball

Differential Revision: https://reviews.llvm.org/D140856

Added: 
    

Modified: 
    mlir/lib/Dialect/Arith/Transforms/ExpandOps.cpp
    mlir/test/Dialect/Arith/expand-ops.mlir

Removed: 
    


################################################################################
diff  --git a/mlir/lib/Dialect/Arith/Transforms/ExpandOps.cpp b/mlir/lib/Dialect/Arith/Transforms/ExpandOps.cpp
index 1a5911c1685e4..b70110cbce913 100644
--- a/mlir/lib/Dialect/Arith/Transforms/ExpandOps.cpp
+++ b/mlir/lib/Dialect/Arith/Transforms/ExpandOps.cpp
@@ -179,22 +179,6 @@ struct MaxMinFOpConverter : public OpRewritePattern<OpTy> {
   }
 };
 
-template <typename OpTy, arith::CmpIPredicate pred>
-struct MaxMinIOpConverter : public OpRewritePattern<OpTy> {
-public:
-  using OpRewritePattern<OpTy>::OpRewritePattern;
-  LogicalResult matchAndRewrite(OpTy op,
-                                PatternRewriter &rewriter) const final {
-    Value lhs = op.getLhs();
-    Value rhs = op.getRhs();
-
-    Location loc = op.getLoc();
-    Value cmp = rewriter.create<arith::CmpIOp>(loc, pred, lhs, rhs);
-    rewriter.replaceOpWithNewOp<arith::SelectOp>(op, cmp, lhs, rhs);
-    return success();
-  }
-};
-
 struct ArithExpandOpsPass
     : public arith::impl::ArithExpandOpsBase<ArithExpandOpsPass> {
   void runOnOperation() override {
@@ -210,11 +194,7 @@ struct ArithExpandOpsPass
       arith::CeilDivUIOp,
       arith::FloorDivSIOp,
       arith::MaxFOp,
-      arith::MaxSIOp,
-      arith::MaxUIOp,
-      arith::MinFOp,
-      arith::MinSIOp,
-      arith::MinUIOp
+      arith::MinFOp
     >();
     // clang-format on
     if (failed(applyPartialConversion(getOperation(), target,
@@ -237,11 +217,7 @@ void mlir::arith::populateArithExpandOpsPatterns(RewritePatternSet &patterns) {
   // clang-format off
   patterns.add<
     MaxMinFOpConverter<MaxFOp, arith::CmpFPredicate::UGT>,
-    MaxMinFOpConverter<MinFOp, arith::CmpFPredicate::ULT>,
-    MaxMinIOpConverter<MaxSIOp, arith::CmpIPredicate::sgt>,
-    MaxMinIOpConverter<MaxUIOp, arith::CmpIPredicate::ugt>,
-    MaxMinIOpConverter<MinSIOp, arith::CmpIPredicate::slt>,
-    MaxMinIOpConverter<MinUIOp, arith::CmpIPredicate::ult>
+    MaxMinFOpConverter<MinFOp, arith::CmpFPredicate::ULT>
    >(patterns.getContext());
   // clang-format on
 }

diff  --git a/mlir/test/Dialect/Arith/expand-ops.mlir b/mlir/test/Dialect/Arith/expand-ops.mlir
index e0990fdec5656..3d55c2068f24e 100644
--- a/mlir/test/Dialect/Arith/expand-ops.mlir
+++ b/mlir/test/Dialect/Arith/expand-ops.mlir
@@ -187,46 +187,3 @@ func.func @minf(%a: f32, %b: f32) -> f32 {
 // CHECK-NEXT: %[[IS_NAN:.*]] = arith.cmpf uno, %[[RHS]], %[[RHS]] : f32
 // CHECK-NEXT: %[[RESULT:.*]] = arith.select %[[IS_NAN]], %[[RHS]], %[[SELECT]] : f32
 // CHECK-NEXT: return %[[RESULT]] : f32
-
-
-// -----
-
-// CHECK-LABEL: func @maxsi
-func.func @maxsi(%a: i32, %b: i32) -> i32 {
-  %result = arith.maxsi %a, %b : i32
-  return %result : i32
-}
-// CHECK-SAME: %[[LHS:.*]]: i32, %[[RHS:.*]]: i32)
-// CHECK-NEXT: %[[CMP:.*]] = arith.cmpi sgt, %[[LHS]], %[[RHS]] : i32
-
-// -----
-
-// CHECK-LABEL: func @minsi
-func.func @minsi(%a: i32, %b: i32) -> i32 {
-  %result = arith.minsi %a, %b : i32
-  return %result : i32
-}
-// CHECK-SAME: %[[LHS:.*]]: i32, %[[RHS:.*]]: i32)
-// CHECK-NEXT: %[[CMP:.*]] = arith.cmpi slt, %[[LHS]], %[[RHS]] : i32
-
-
-// -----
-
-// CHECK-LABEL: func @maxui
-func.func @maxui(%a: i32, %b: i32) -> i32 {
-  %result = arith.maxui %a, %b : i32
-  return %result : i32
-}
-// CHECK-SAME: %[[LHS:.*]]: i32, %[[RHS:.*]]: i32)
-// CHECK-NEXT: %[[CMP:.*]] = arith.cmpi ugt, %[[LHS]], %[[RHS]] : i32
-
-
-// -----
-
-// CHECK-LABEL: func @minui
-func.func @minui(%a: i32, %b: i32) -> i32 {
-  %result = arith.minui %a, %b : i32
-  return %result : i32
-}
-// CHECK-SAME: %[[LHS:.*]]: i32, %[[RHS:.*]]: i32)
-// CHECK-NEXT: %[[CMP:.*]] = arith.cmpi ult, %[[LHS]], %[[RHS]] : i32