Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix simplification of x + x//c*-c to x mod c. #98909

Merged
merged 1 commit into from
Jul 15, 2024
Merged

Conversation

jreiffers
Copy link
Contributor

There was no check that rhs is actually a multiplication.

There was no check that rhs is actually a multiplication.
@jreiffers jreiffers requested a review from d0k July 15, 2024 14:26
@llvmbot llvmbot added mlir:core MLIR Core Infrastructure mlir labels Jul 15, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 15, 2024

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Johannes Reifferscheid (jreiffers)

Changes

There was no check that rhs is actually a multiplication.


Full diff: https://github.com/llvm/llvm-project/pull/98909.diff

2 Files Affected:

  • (modified) mlir/lib/IR/AffineExpr.cpp (+3-1)
  • (modified) mlir/unittests/IR/AffineExprTest.cpp (+8)
diff --git a/mlir/lib/IR/AffineExpr.cpp b/mlir/lib/IR/AffineExpr.cpp
index bfb7c4849356e..75cc01ee9a146 100644
--- a/mlir/lib/IR/AffineExpr.cpp
+++ b/mlir/lib/IR/AffineExpr.cpp
@@ -751,8 +751,10 @@ static AffineExpr simplifyAdd(AffineExpr lhs, AffineExpr rhs) {
   }
 
   // Process lrhs, which is 'expr floordiv c'.
+  // expr + (expr // c * -c) = expr % c
   AffineBinaryOpExpr lrBinOpExpr = dyn_cast<AffineBinaryOpExpr>(lrhs);
-  if (!lrBinOpExpr || lrBinOpExpr.getKind() != AffineExprKind::FloorDiv)
+  if (!lrBinOpExpr || rhs.getKind() != AffineExprKind::Mul ||
+      lrBinOpExpr.getKind() != AffineExprKind::FloorDiv)
     return nullptr;
 
   llrhs = lrBinOpExpr.getLHS();
diff --git a/mlir/unittests/IR/AffineExprTest.cpp b/mlir/unittests/IR/AffineExprTest.cpp
index a0affc4341b0b..75c893334943d 100644
--- a/mlir/unittests/IR/AffineExprTest.cpp
+++ b/mlir/unittests/IR/AffineExprTest.cpp
@@ -98,3 +98,11 @@ TEST(AffineExprTest, divisionSimplification) {
   ASSERT_EQ((d0 * 6).ceilDiv(4).getKind(), AffineExprKind::CeilDiv);
   ASSERT_EQ((d0 * 6).ceilDiv(-2), d0 * -3);
 }
+
+TEST(AffineExprTest, modSimplificationRegression) {
+  MLIRContext ctx;
+  OpBuilder b(&ctx);
+  auto d0 = b.getAffineDimExpr(0);
+  auto sum = d0 + d0.floorDiv(3).floorDiv(-3);
+  ASSERT_EQ(sum.getKind(), AffineExprKind::Add);
+}

@jreiffers jreiffers merged commit dd7d81e into llvm:main Jul 15, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:core MLIR Core Infrastructure mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants