Skip to content

Commit 9460305

Browse files
committed
[mlir][arith] Fix overflow bug in arith::CeilDivSIOp::fold
The folder for arith::CeilDivSIOp should only be applied when it can be guaranteed that no overflow would happen. The current implementation works fine when both dividends are positive and the only arithmetic operation is the division itself. However, in cases where at least one of the dividends is negative, the division is split into multiple operations, e.g.: `- ( -a / b)`. That's additional 2 operations on top of the actual division that can overflow - the folder should check all 3 ops for overflow. The current logic doesn't do that - it effectively only the last operation (i.e. the division). It breaks when using e.g. MININT values (e.g. -128 for 8-bit integers) - negating such values overflows. This PR makes sure that no folding happens if any of the intermediate arithmetic operations overflows.
1 parent 33e16ca commit 9460305

File tree

2 files changed

+47
-9
lines changed

2 files changed

+47
-9
lines changed

mlir/lib/Dialect/Arith/IR/ArithOps.cpp

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -701,22 +701,34 @@ OpFoldResult arith::CeilDivSIOp::fold(FoldAdaptor adaptor) {
701701
// Both positive, return ceil(a, b).
702702
return signedCeilNonnegInputs(a, b, overflowOrDiv0);
703703
}
704+
705+
bool overflowNegA = false;
706+
bool overflowNegB = false;
707+
bool overflowNegDiv = false;
708+
bool overflowDiv = false;
704709
if (!aGtZero && !bGtZero) {
705710
// Both negative, return ceil(-a, -b).
706-
APInt posA = zero.ssub_ov(a, overflowOrDiv0);
707-
APInt posB = zero.ssub_ov(b, overflowOrDiv0);
708-
return signedCeilNonnegInputs(posA, posB, overflowOrDiv0);
711+
APInt posA = zero.ssub_ov(a, overflowNegA);
712+
APInt posB = zero.ssub_ov(b, overflowNegB);
713+
APInt res = signedCeilNonnegInputs(posA, posB, overflowDiv);
714+
overflowOrDiv0 = (overflowNegA || overflowNegB || overflowDiv);
715+
return res;
709716
}
710717
if (!aGtZero && bGtZero) {
711718
// A is negative, b is positive, return - ( -a / b).
712-
APInt posA = zero.ssub_ov(a, overflowOrDiv0);
713-
APInt div = posA.sdiv_ov(b, overflowOrDiv0);
714-
return zero.ssub_ov(div, overflowOrDiv0);
719+
APInt posA = zero.ssub_ov(a, overflowNegA);
720+
APInt div = posA.sdiv_ov(b, overflowDiv);
721+
APInt res = zero.ssub_ov(div, overflowNegDiv);
722+
overflowOrDiv0 = (overflowNegA || overflowDiv || overflowNegDiv);
723+
return res;
715724
}
716725
// A is positive, b is negative, return - (a / -b).
717-
APInt posB = zero.ssub_ov(b, overflowOrDiv0);
718-
APInt div = a.sdiv_ov(posB, overflowOrDiv0);
719-
return zero.ssub_ov(div, overflowOrDiv0);
726+
APInt posB = zero.ssub_ov(b, overflowNegB);
727+
APInt div = a.sdiv_ov(posB, overflowDiv);
728+
APInt res = zero.ssub_ov(div, overflowNegDiv);
729+
730+
overflowOrDiv0 = (overflowNegB || overflowDiv || overflowNegDiv);
731+
return res;
720732
});
721733

722734
return overflowOrDiv0 ? Attribute() : result;

mlir/test/Transforms/constant-fold.mlir

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,6 +478,32 @@ func.func @simple_arith.ceildivsi() -> (i32, i32, i32, i32, i32) {
478478

479479
// -----
480480

481+
// CHECK-LABEL: func @simple_arith.ceildivsi_overflow
482+
func.func @simple_arith.ceildivsi_overflow() -> (i8, i16, i32) {
483+
// The negative values below are MININTs for the corresponding bit-width. The
484+
// folder will try to negate them (so that the division operartes on two
485+
// positive numbers), but that would cause overflow (negating MININT
486+
// overflows). Hence folding should not happen and the original ceildivsi is
487+
// preserved.
488+
489+
// CHECK-COUNT-3: arith.ceildivsi
490+
%0 = arith.constant 7 : i8
491+
%min_int_i8 = arith.constant -128 : i8
492+
%2 = arith.ceildivsi %min_int_i8, %0 : i8
493+
494+
%3 = arith.constant 7 : i16
495+
%min_int_i16 = arith.constant -32768 : i16
496+
%5 = arith.ceildivsi %min_int_i16, %3 : i16
497+
498+
%6 = arith.constant 7 : i32
499+
%min_int_i32 = arith.constant -2147483648 : i32
500+
%8 = arith.ceildivsi %min_int_i32, %6 : i32
501+
502+
return %2, %5, %8 : i8, i16, i32
503+
}
504+
505+
// -----
506+
481507
// CHECK-LABEL: func @simple_arith.ceildivui
482508
func.func @simple_arith.ceildivui() -> (i32, i32, i32, i32, i32) {
483509
// CHECK-DAG: [[C0:%.+]] = arith.constant 0

0 commit comments

Comments
 (0)