[Mlir-commits] [mlir] [mlir] Compute ceildivs consistently for INT_MIN operands (PR #215696)

[Hung Kuan Tseng]

================
@@ -54,25 +54,18 @@ func.func @ceildivs(%n: index, %m: index) -> index {
   // CHECK-DAG: %[[N:.*]] = builtin.unrealized_conversion_cast %[[NI]]
   // CHECK-DAG: %[[M:.*]] = builtin.unrealized_conversion_cast %[[MI]]
   // CHECK: %[[ZERO:.*]] = llvm.mlir.constant(0 :
-  // CHECK: %[[POS_ONE:.*]] = llvm.mlir.constant(1 :
-  // CHECK: %[[NEG_ONE:.*]] = llvm.mlir.constant(-1 :
-
-  // CHECK: %[[M_POS:.*]] = llvm.icmp "sgt" %[[M]], %[[ZERO]]
-  // CHECK: %[[X:.*]] = llvm.select %[[M_POS]], %[[NEG_ONE]], %[[POS_ONE]]
-
-  // CHECK: %[[N_PLUS_X:.*]] = llvm.add %[[N]], %[[X]]
-  // CHECK: %[[N_PLUS_X_DIV_M:.*]] = llvm.sdiv %[[N_PLUS_X]], %[[M]]
-  // CHECK: %[[POS_RES:.*]] = llvm.add %[[N_PLUS_X_DIV_M]], %[[POS_ONE]]
+  // CHECK: %[[ONE:.*]] = llvm.mlir.constant(1 :
 
-  // CHECK: %[[NEG_N:.*]] = llvm.sub %[[ZERO]], %[[N]]
-  // CHECK: %[[NEG_N_DIV_M:.*]] = llvm.sdiv %[[NEG_N]], %[[M]]
-  // CHECK: %[[NEG_RES:.*]] = llvm.sub %[[ZERO]], %[[NEG_N_DIV_M]]
+  // CHECK: %[[QUOTIENT:.*]] = llvm.sdiv %[[N]], %[[M]]
+  // CHECK: %[[QUOTIENT_PLUS_ONE:.*]] = llvm.add %[[QUOTIENT]], %[[ONE]]
 
-  // CHECK: %[[N_POS:.*]] = llvm.icmp "sgt" %[[N]], %[[ZERO]]
-  // CHECK: %[[SAME_SIGN:.*]] = llvm.icmp "eq" %[[N_POS]], %[[M_POS]]
-  // CHECK: %[[N_NON_ZERO:.*]] = llvm.icmp "ne" %[[N]], %[[ZERO]]
-  // CHECK: %[[CMP:.*]] = llvm.and %[[SAME_SIGN]], %[[N_NON_ZERO]]
-  // CHECK: %[[RESULT:.*]] = llvm.select %[[CMP]], %[[POS_RES]], %[[NEG_RES]]
+  // CHECK: %[[PRODUCT:.*]] = llvm.mul %[[QUOTIENT]], %[[M]]
+  // CHECK: %[[INEXACT:.*]] = llvm.icmp "ne" %[[N]], %[[PRODUCT]]
+  // CHECK: %[[N_NEG:.*]] = llvm.icmp "slt" %[[N]], %[[ZERO]]
+  // CHECK: %[[M_NEG:.*]] = llvm.icmp "slt" %[[M]], %[[ZERO]]
+  // CHECK: %[[SAME_SIGN:.*]] = llvm.icmp "eq" %[[N_NEG]], %[[M_NEG]]
+  // CHECK: %[[CMP:.*]] = llvm.and %[[INEXACT]], %[[SAME_SIGN]]
+  // CHECK: %[[RESULT:.*]] = llvm.select %[[CMP]], %[[QUOTIENT_PLUS_ONE]], %[[QUOTIENT]]
   %result = index.ceildivs %n, %m
----------------
Tim096 wrote:

Added, under the existing `INDEX32`/`INDEX64` runs rather than a new one: `ceildivs(-2147483648, 7)` now checks `llvm.mlir.constant(-306783378 : i32)` and the `i64` counterpart. The conversion alone already emits the constant, so it does not need `-canonicalize`; that pass only deletes the two dead operand constants.

It is worth recording what the check pins, because it is not the emitted sequence. `ConversionConfig::foldingMode` defaults to `BeforePatterns`, so `OperationLegalizer` folds an illegal op before it looks for a pattern (`DialectConversion.cpp:2601`). With constant operands `ConvertIndexCeilDivS` therefore never runs, and the value comes from `calculateCeilDivS`.

It still fails on main, which is what you asked for: there the fold bails on an `INT_MIN` dividend, so the pattern does run, and nothing folds its output afterwards -- `llvm.sdiv`, `llvm.add`, `llvm.mul` and `llvm.select` have no folders. So the check catches "the fold and the lowering disagree about this input", which is exactly the bug. What it cannot catch is a lowering that regresses while the folder stays correct; the operation-sequence checks above it are still what covers that.


https://github.com/llvm/llvm-project/pull/215696