[MLIR][Arith] Canonicalize cmpf(int to fp) to cmpi

[wsmoses]

Summary

Given a cmpf of either uitofp or sitofp and a constant, attempt to canonicalize it to a cmpi.

This PR rewrites equivalent code within LLVM to now apply to MLIR arith.

[rriddle]

mlir/lib/IR/BuiltinTypes.cpp

145

I'm not really comfortable referencing LLVM IR from inside of MLIR IR like this, I'd rather have proper documentation here that describes whatever the invariants are.

[rriddle]

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

1321

A lot of this is using LLVM coding style (e.g. CamelCase), can you clean it up? It's fine to "take inspiration" from other parts of the codebase, but we should make sure to clean them up and modernize them. Ideally we don't want to proliferate old things forward that we don't need to.

[rriddle]

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

1315

I would remove this. If either this or the other get out of sync (for some number of reasons), this comment is just confusing. Ideally all of the code here should be properly documented and lay out whatever invariants are driving the transformation. I wouldn't constrain/limit whatever we do here with what has been done in InstCombine.

[wsmoses]

mlir/lib/IR/BuiltinTypes.cpp

145

Honestly, here I'd prefer this to be implemented within fltSemantics (https://llvm.org/doxygen/structllvm_1_1fltSemantics.html) since this is already defined as precision. Unfortunately, the definition of fltSemantics isn't exposed outside APFloat and requires a core change to LLVM.

Assuming the core LLVM change isn't tenable, what would you suggest here: just removing the comment like getWidth (which also has the relevant comment in the header)?

[rriddle]

mlir/lib/IR/BuiltinTypes.cpp

145

You should be able to access the precision of an fltSemantics via APFloat::semanticsPrecision AFAICR.

[wsmoses]

Remove references to LLVM and fix variable style

[rriddle]

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

1341–1342

1345–1349

These look the same value, is there value to having them separate? If so, I would just assign this one to intWidth.

1359–1361

This if condition is a bit brain hurty, can you restructure this to read better?

1365–1368

nit: Please avoid floating statements in control flow.

1403

Can you add parameter comments here (and just below)? It isn't obvious what the true and 1 mean here.

1449

Same comments about param comments here and throughout.

1482

Can you split out some of these logic blocks into helper functions? This function is quite large and covers a lot of different cases.

mlir/test/Dialect/Arithmetic/canonicalize.mlir

706

Please indent the check lines, and can you also avoid using raw SSA values?

https://mlir.llvm.org/getting_started/TestingGuide/

Same for the other tests.

[wsmoses]

Address comments

[wsmoses]

Address comments

[wsmoses]

bumping @rriddle can you re-review?

[rriddle]

mlir/test/Dialect/Arithmetic/canonicalize.mlir

753

What is this rdar referencing?

[wsmoses]

Remove rdar

mlir/test/Dialect/Arithmetic/canonicalize.mlir

753

It's a remant from the LLVM version of these test, it is now removed.

[max-kudr]

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

1510–1511

We are getting warning here. I am sort of lost at how to "future proof" this switch statement without removing default label. Could you please fix this warning?

view on buildbot

/vol/worker/mlir-nvidia/mlir-nvidia/llvm.src/mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp:1597:9: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]

[max-kudr]

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

1510–1511

Created fix ready to commit: D120730

mlir/include/mlir/Dialect/Arithmetic/IR/ArithmeticOps.td

let builders = [
     }]>
   ];
 
   let extraClassDeclaration = [{
     static arith::CmpFPredicate getPredicateByName(StringRef name);
   }];
 
   let hasFolder = 1;
+  let hasCanonicalizer = 1;
 }
 
 #endif // ARITHMETIC_OPS

mlir/include/mlir/IR/BuiltinTypes.h

public:
   static FloatType getF128(MLIRContext *ctx);
 
   /// Methods for support type inquiry through isa, cast, and dyn_cast.
   static bool classof(Type type);
 
   /// Return the bitwidth of this float type.
   unsigned getWidth();
 
+  /// Return the width of the mantissa of this type.
+  unsigned getFPMantissaWidth();
+
   /// Get or create a new FloatType with bitwidth scaled by `scale`.
   /// Return null if the scaled element type cannot be represented.
   FloatType scaleElementBitwidth(unsigned scale);
 
   /// Return the floating semantics of this float type.
   const llvm::fltSemantics &getFloatSemantics();
 };
 

mlir/lib/Dialect/Arithmetic/IR/ArithmeticOps.cpp

OpFoldResult arith::CmpFOp::fold(ArrayRef<Attribute> operands) {
 
   if (!lhs || !rhs)
     return {};
 
   auto val = applyCmpPredicate(getPredicate(), lhs.getValue(), rhs.getValue());
   return BoolAttr::get(getContext(), val);
 }
 
+class CmpFIntToFPConst final : public OpRewritePattern<CmpFOp> {
+public:
+  using OpRewritePattern<CmpFOp>::OpRewritePattern;
+
+  LogicalResult matchAndRewrite(CmpFOp op,
+                                PatternRewriter &rewriter) const override {
+    FloatAttr flt;
+    if (!matchPattern(op.getRhs(), m_Constant(&flt)))
+      return failure();
+
+    const APFloat &rhs = flt.getValue();
+
+    // Don't attempt to fold a nan.
+    if (rhs.isNaN())
+      return failure();
+
+    // Get the width of the mantissa.  We don't want to hack on conversions that
+    // might lose information from the integer, e.g. "i64 -> float"
+    FloatType floatTy = op.getRhs().getType().cast<FloatType>();
+    int mantissaWidth = floatTy.getFPMantissaWidth();
+    if (mantissaWidth <= 0)
+      return failure();
+
+    bool isUnsigned;
+    Value intVal;
+
+    if (auto si = op.getLhs().getDefiningOp<SIToFPOp>()) {
+      isUnsigned = false;
+      intVal = si.getIn();
+    } else if (auto ui = op.getLhs().getDefiningOp<UIToFPOp>()) {
+      isUnsigned = true;
+      intVal = ui.getIn();
+    } else
+      return failure();
+
+    auto intTy = intVal.getType().cast<IntegerType>();
+    auto intWidth = intTy.getWidth();
+
+    // Check to see that the input is converted from an integer type that is
+    // small enough that preserves all bits.
+    unsigned inputSize = intTy.getWidth();
+
+    // Following test does NOT adjust inputSize downwards for signed inputs,
+    // because the most negative value still requires all the mantissa bits
+    // to distinguish it from one less than that value.
+    if ((int)inputSize > mantissaWidth) {
+      // Conversion would lose accuracy. Check if loss can impact comparison.
+      int exponent = ilogb(rhs);
+      if (exponent == APFloat::IEK_Inf) {
+        int maxExponent = ilogb(APFloat::getLargest(rhs.getSemantics()));
+        if (maxExponent < (int)inputSize - !isUnsigned)
+          // Conversion could create infinity.
+          return failure();
+      } else {
+        // Note that if rhs is zero or NaN, then Exp is negative
+        // and first condition is trivially false.
+        if (mantissaWidth <= exponent &&
+            exponent <= (int)inputSize - !isUnsigned)
+          // Conversion could affect comparison.
+          return failure();
+      }
+    }
+
+    // Convert to equivalent cmpi predicate
+    CmpIPredicate pred;
+    switch (op.getPredicate()) {
+    default:
+      llvm_unreachable("Unexpected predicate!");
+    case CmpFPredicate::UEQ:
+    case CmpFPredicate::OEQ:
+      pred = CmpIPredicate::eq;
+      break;
+    case CmpFPredicate::UGT:
+    case CmpFPredicate::OGT:
+      pred = isUnsigned ? CmpIPredicate::ugt : CmpIPredicate::sgt;
+      break;
+    case CmpFPredicate::UGE:
+    case CmpFPredicate::OGE:
+      pred = isUnsigned ? CmpIPredicate::uge : CmpIPredicate::sge;
+      break;
+    case CmpFPredicate::ULT:
+    case CmpFPredicate::OLT:
+      pred = isUnsigned ? CmpIPredicate::ult : CmpIPredicate::slt;
+      break;
+    case CmpFPredicate::ULE:
+    case CmpFPredicate::OLE:
+      pred = isUnsigned ? CmpIPredicate::ule : CmpIPredicate::sle;
+      break;
+    case CmpFPredicate::UNE:
+    case CmpFPredicate::ONE:
+      pred = CmpIPredicate::ne;
+      break;
+    case CmpFPredicate::ORD:
+      // Int to fp conversion doesn't create a nan (ord checks neither is a nan)
+      rewriter.replaceOpWithNewOp<ConstantIntOp>(op, true, 1);
+      return success();
+    case CmpFPredicate::UNO:
+      // Int to fp conversion doesn't create a nan (uno checks either is a nan)
+      rewriter.replaceOpWithNewOp<ConstantIntOp>(op, false, 1);
+      return success();
+    }
+
+    if (!isUnsigned) {
+      // If the rhs value is > SignedMax, fold the comparison.  This handles
+      // +INF and large values.
+      APFloat signedMax(rhs.getSemantics());
+      signedMax.convertFromAPInt(APInt::getSignedMaxValue(intWidth), true,
+                                 APFloat::rmNearestTiesToEven);
+      if (signedMax < rhs) { // smax < 13123.0
+        if (pred == CmpIPredicate::ne || pred == CmpIPredicate::slt ||
+            pred == CmpIPredicate::sle)
+          rewriter.replaceOpWithNewOp<ConstantIntOp>(op, true, 1);
+        else
+          rewriter.replaceOpWithNewOp<ConstantIntOp>(op, false, 1);
+        return success();
+      }
+    } else {
+      // If the rhs value is > UnsignedMax, fold the comparison. This handles
+      // +INF and large values.
+      APFloat unsignedMax(rhs.getSemantics());
+      unsignedMax.convertFromAPInt(APInt::getMaxValue(intWidth), false,
+                                   APFloat::rmNearestTiesToEven);
+      if (unsignedMax < rhs) { // umax < 13123.0
+        if (pred == CmpIPredicate::ne || pred == CmpIPredicate::ult ||
+            pred == CmpIPredicate::ule)
+          rewriter.replaceOpWithNewOp<ConstantIntOp>(op, true, 1);
+        else
+          rewriter.replaceOpWithNewOp<ConstantIntOp>(op, false, 1);
+        return success();
+      }
+    }
+
+    if (!isUnsigned) {
+      // See if the rhs value is < SignedMin.
+      APFloat signedMin(rhs.getSemantics());
+      signedMin.convertFromAPInt(APInt::getSignedMinValue(intWidth), true,
+                                 APFloat::rmNearestTiesToEven);
+      if (signedMin > rhs) { // smin > 12312.0
+        if (pred == CmpIPredicate::ne || pred == CmpIPredicate::sgt ||
+            pred == CmpIPredicate::sge)
+          rewriter.replaceOpWithNewOp<ConstantIntOp>(op, true, 1);
+        else
+          rewriter.replaceOpWithNewOp<ConstantIntOp>(op, false, 1);
+        return success();
+      }
+    } else {
+      // See if the rhs value is < UnsignedMin.
+      APFloat unsignedMin(rhs.getSemantics());
+      unsignedMin.convertFromAPInt(APInt::getMinValue(intWidth), false,
+                                   APFloat::rmNearestTiesToEven);
+      if (unsignedMin > rhs) { // umin > 12312.0
+        if (pred == CmpIPredicate::ne || pred == CmpIPredicate::ugt ||
+            pred == CmpIPredicate::uge)
+          rewriter.replaceOpWithNewOp<ConstantIntOp>(op, true, 1);
+        else
+          rewriter.replaceOpWithNewOp<ConstantIntOp>(op, false, 1);
+        return success();
+      }
+    }
+
+    // Okay, now we know that the FP constant fits in the range [SMIN, SMAX] or
+    // [0, UMAX], but it may still be fractional.  See if it is fractional by
+    // casting the FP value to the integer value and back, checking for
+    // equality. Don't do this for zero, because -0.0 is not fractional.
+    bool ignored;
+    APSInt rhsInt(intWidth, isUnsigned);
+    if (APFloat::opInvalidOp ==
+        rhs.convertToInteger(rhsInt, APFloat::rmTowardZero, &ignored)) {
+      // Undefined behavior invoked - the destination type can't represent
+      // the input constant.
+      return failure();
+    }
+
+    if (!rhs.isZero()) {
+      APFloat apf(floatTy.getFloatSemantics(),
+                  APInt::getZero(floatTy.getWidth()));
+      apf.convertFromAPInt(rhsInt, !isUnsigned, APFloat::rmNearestTiesToEven);
+
+      bool equal = apf == rhs;
+      if (!equal) {
+        // If we had a comparison against a fractional value, we have to adjust
+        // the compare predicate and sometimes the value.  rhsInt is rounded
+        // towards zero at this point.
+        switch (pred) {
+        default:
+          llvm_unreachable("Unexpected integer comparison!");
+        case CmpIPredicate::ne: // (float)int != 4.4   --> true
+          rewriter.replaceOpWithNewOp<ConstantIntOp>(op, true, 1);
+          return success();
+        case CmpIPredicate::eq: // (float)int == 4.4   --> false
+          rewriter.replaceOpWithNewOp<ConstantIntOp>(op, false, 1);
+          return success();
+        case CmpIPredicate::ule:
+          // (float)int <= 4.4   --> int <= 4
+          // (float)int <= -4.4  --> false
+          if (rhs.isNegative()) {
+            rewriter.replaceOpWithNewOp<ConstantIntOp>(op, false, 1);
+            return success();
+          }
+          break;
+        case CmpIPredicate::sle:
+          // (float)int <= 4.4   --> int <= 4
+          // (float)int <= -4.4  --> int < -4
+          if (rhs.isNegative())
+            pred = CmpIPredicate::slt;
+          break;
+        case CmpIPredicate::ult:
+          // (float)int < -4.4   --> false
+          // (float)int < 4.4    --> int <= 4
+          if (rhs.isNegative()) {
+            rewriter.replaceOpWithNewOp<ConstantIntOp>(op, false, 1);
+            return success();
+          }
+          pred = CmpIPredicate::ule;
+          break;
+        case CmpIPredicate::slt:
+          // (float)int < -4.4   --> int < -4
+          // (float)int < 4.4    --> int <= 4
+          if (!rhs.isNegative())
+            pred = CmpIPredicate::sle;
+          break;
+        case CmpIPredicate::ugt:
+          // (float)int > 4.4    --> int > 4
+          // (float)int > -4.4   --> true
+          if (rhs.isNegative()) {
+            rewriter.replaceOpWithNewOp<ConstantIntOp>(op, true, 1);
+            return success();
+          }
+          break;
+        case CmpIPredicate::sgt:
+          // (float)int > 4.4    --> int > 4
+          // (float)int > -4.4   --> int >= -4
+          if (rhs.isNegative())
+            pred = CmpIPredicate::sge;
+          break;
+        case CmpIPredicate::uge:
+          // (float)int >= -4.4   --> true
+          // (float)int >= 4.4    --> int > 4
+          if (rhs.isNegative()) {
+            rewriter.replaceOpWithNewOp<ConstantIntOp>(op, true, 1);
+            return success();
+          }
+          pred = CmpIPredicate::ugt;
+          break;
+        case CmpIPredicate::sge:
+          // (float)int >= -4.4   --> int >= -4
+          // (float)int >= 4.4    --> int > 4
+          if (!rhs.isNegative())
+            pred = CmpIPredicate::sgt;
+          break;
+        }
+      }
+    }
+
+    // Lower this FP comparison into an appropriate integer version of the
+    // comparison.
+    rewriter.replaceOpWithNewOp<CmpIOp>(
+        op, pred, intVal,
+        rewriter.create<ConstantOp>(
+            op.getLoc(), intVal.getType(),
+            rewriter.getIntegerAttr(intVal.getType(), rhsInt)));
+    return success();
+  }
+};
+
+void arith::CmpFOp::getCanonicalizationPatterns(
+    OwningRewritePatternList &patterns, MLIRContext *context) {
+  patterns.insert<CmpFIntToFPConst>(context);
+}
+
 //===----------------------------------------------------------------------===//
 // Atomic Enum
 //===----------------------------------------------------------------------===//
 
 /// Returns the identity value attribute associated with an AtomicRMWKind op.
 Attribute mlir::arith::getIdentityValueAttr(AtomicRMWKind kind, Type resultType,
                                             OpBuilder &builder, Location loc) {
   switch (kind) {

mlir/lib/IR/BuiltinTypes.cpp

if (scale == 4)
       return FloatType::getF64(ctx);
   }
   if (isF32())
     if (scale == 2)
       return FloatType::getF64(ctx);
   return FloatType();
 }
 
+unsigned FloatType::getFPMantissaWidth() {
+  return APFloat::semanticsPrecision(getFloatSemantics());
+}
+
 //===----------------------------------------------------------------------===//
 // FunctionType
 //===----------------------------------------------------------------------===//
 
 unsigned FunctionType::getNumInputs() const { return getImpl()->numInputs; }
 
 ArrayRef<Type> FunctionType::getInputs() const {
   return getImpl()->getInputs();

mlir/test/Dialect/Arithmetic/canonicalize.mlir

func @constant_MinMax(%arg0 : f32) -> f32 {
   // CHECK:  %[[min:.+]] = arith.minf %arg0, %[[const]] : f32
   // CHECK:  %[[res:.+]] = arith.maxf %[[min]], %[[const]] : f32
   // CHECK:   return %[[res]]
   %const = arith.constant 0.0 : f32
   %min = arith.minf %const, %arg0 : f32
   %res = arith.maxf %const, %min : f32
   return %res : f32
 }
+
+// -----
+
+// Tests rewritten from https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/InstCombine/2008-11-08-FCmp.ll
+// When inst combining an FCMP with the LHS coming from a arith.uitofp instruction, we
+// can lower it to signed ICMP instructions.
+
+// CHECK-LABEL: @test1(
+func @test1(%arg0: i32) -> i1 {
+  %cst = arith.constant 0.000000e+00 : f64
+  %1 = arith.uitofp %arg0: i32 to f64
+  %2 = arith.cmpf ole, %1, %cst : f64
+// CHECK: arith.cmpi ule, %arg0, %c0_i32 : i32
+  return %2 : i1
+}
+
+// CHECK-LABEL: @test2(
+func @test2(%arg0: i32) -> i1 {
+  %cst = arith.constant 0.000000e+00 : f64
+  %1 = arith.uitofp %arg0: i32 to f64
+  %2 = arith.cmpf olt, %1, %cst : f64
+  return %2 : i1
+// CHECK: arith.cmpi ult, %arg0, %c0_i32 : i32
+}
+
+// CHECK-LABEL: @test3(
+func @test3(%arg0: i32) -> i1 {
+  %cst = arith.constant 0.000000e+00 : f64
+  %1 = arith.uitofp %arg0: i32 to f64
+  %2 = arith.cmpf oge, %1, %cst : f64
+  return %2 : i1
+// CHECK: arith.cmpi uge, %arg0, %c0_i32 : i32
+}
+
+// CHECK-LABEL: @test4(
+func @test4(%arg0: i32) -> i1 {
+  %cst = arith.constant 0.000000e+00 : f64
+  %1 = arith.uitofp %arg0: i32 to f64
+  %2 = arith.cmpf ogt, %1, %cst : f64
+// CHECK: arith.cmpi ugt, %arg0, %c0_i32 : i32
+  return %2 : i1
+}
+
+// CHECK-LABEL: @test5(
+func @test5(%arg0: i32) -> i1 {
+  %cst = arith.constant -4.400000e+00 : f64
+  %1 = arith.uitofp %arg0: i32 to f64
+  %2 = arith.cmpf ogt, %1, %cst : f64
+  return %2 : i1
+// CHECK: return %true : i1
+}
+
+// CHECK-LABEL: @test6(
+func @test6(%arg0: i32) -> i1 {
+  %cst = arith.constant -4.400000e+00 : f64
+  %1 = arith.uitofp %arg0: i32 to f64
+  %2 = arith.cmpf olt, %1, %cst : f64
+  return %2 : i1
+// CHECK: return %false : i1
+}
+
+// Check that optimizing unsigned >= comparisons correctly distinguishes
+// positive and negative constants.  <rdar://problem/12029145>
+// CHECK-LABEL: @test7(
+func @test7(%arg0: i32) -> i1 {
+  %cst = arith.constant 3.200000e+00 : f64
+  %1 = arith.uitofp %arg0: i32 to f64
+  %2 = arith.cmpf oge, %1, %cst : f64
+  return %2 : i1
+// CHECK: arith.cmpi ugt, %arg0, %c3_i32 : i32
+}