[Mlir-commits] [mlir] [mlir] Fold ceil/floordiv with negative RHS. (PR #97031)

[Uday Bondhugula]

================
@@ -855,8 +855,7 @@ static AffineExpr simplifyFloorDiv(AffineExpr lhs, AffineExpr rhs) {
   auto lhsConst = dyn_cast<AffineConstantExpr>(lhs);
   auto rhsConst = dyn_cast<AffineConstantExpr>(rhs);
 
-  // mlir floordiv by zero or negative numbers is undefined and preserved as is.
-  if (!rhsConst || rhsConst.getValue() < 1)
+  if (!rhsConst || rhsConst.getValue() == 0)
----------------
bondhugula wrote:

@jreiffers This change is actually based on an incorrect assumption. The divisor of MLIR floordiv, ceildiv, and mod are all expected to be positive (undefined behavior if they aren't) - this is documented in the spec: https://mlir.llvm.org/docs/Dialects/Affine/#affine-expressions
Note that it'd be inconsistent to have floordiv and ceildiv support negative RHSs but not have mod support it, because they are all connected mathematically. The `mod` in MLIR is defined to be the mathematical modulo (not C % operator):   
`a mod b = a - (a floordiv b) * b` (the floordiv rounds towards -ve infinity and is not the same as integer division //, which rounds towards zero), and one should be able to go from mod form to the div form mathematically. So `a mod b` always takes the sign of the divisor and is thus always positive since we define it for positive `b` for all affine math meaning. It'd be a broken/inconsistent design if we had allowed floordiv and ceildiv with negative RHSs, and so from the start, they were defined for positive divisors. The comment above wasn't copied over from `simplifyMod` and that's the documented design.

That said, your other PR to not simplify a - (a floordiv b) * b to `a mod b` for negative `b` is fine. Since it's undefined behavior, the compiler can choose to leave it that way. Even in the case of this PR, one could argue that the method is free to simplify it, but to be consistent, it can simply leave it alone.

But we shouldn't be dropping the comment above and there is a cleanup needed for the inconsistency with negative RHSs for div operations.



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