Skip to content

Conversation

@jtuyls
Copy link
Contributor

@jtuyls jtuyls commented Oct 27, 2025

We shouldn't just consider the dynamic dimensions, but all output dimensions for the value bounds constraints. The previous test just worked because the dynamic dimension was on the first position.

@llvmbot
Copy link
Member

llvmbot commented Oct 27, 2025

@llvm/pr-subscribers-mlir-memref

@llvm/pr-subscribers-mlir

Author: Jorn Tuyls (jtuyls)

Changes

We shouldn't just consider the dynamic dimensions, but all output dimensions for the value bounds constraints. The previous test just worked because the dynamic dimension was on the first position.


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

2 Files Affected:

  • (modified) mlir/lib/Dialect/MemRef/IR/ValueBoundsOpInterfaceImpl.cpp (+1-1)
  • (modified) mlir/test/Dialect/MemRef/value-bounds-op-interface-impl.mlir (+4-4)
diff --git a/mlir/lib/Dialect/MemRef/IR/ValueBoundsOpInterfaceImpl.cpp b/mlir/lib/Dialect/MemRef/IR/ValueBoundsOpInterfaceImpl.cpp
index a15bf891dd596..6fa8ce4efff3b 100644
--- a/mlir/lib/Dialect/MemRef/IR/ValueBoundsOpInterfaceImpl.cpp
+++ b/mlir/lib/Dialect/MemRef/IR/ValueBoundsOpInterfaceImpl.cpp
@@ -66,7 +66,7 @@ struct ExpandShapeOpInterface
                                        ValueBoundsConstraintSet &cstr) const {
     auto expandOp = cast<memref::ExpandShapeOp>(op);
     assert(value == expandOp.getResult() && "invalid value");
-    cstr.bound(value)[dim] == expandOp.getOutputShape()[dim];
+    cstr.bound(value)[dim] == expandOp.getMixedOutputShape()[dim];
   }
 };
 
diff --git a/mlir/test/Dialect/MemRef/value-bounds-op-interface-impl.mlir b/mlir/test/Dialect/MemRef/value-bounds-op-interface-impl.mlir
index ac1f22b68b1e1..f9b81dfc7d468 100644
--- a/mlir/test/Dialect/MemRef/value-bounds-op-interface-impl.mlir
+++ b/mlir/test/Dialect/MemRef/value-bounds-op-interface-impl.mlir
@@ -67,11 +67,11 @@ func.func @memref_dim_all_positive(%m: memref<?xf32>, %x: index) {
 //  CHECK-SAME:     %[[m:[a-zA-Z0-9]+]]: memref<?xf32>
 //  CHECK-SAME:     %[[sz:[a-zA-Z0-9]+]]: index
 //       CHECK:   %[[c4:.*]] = arith.constant 4 : index
-//       CHECK:   return %[[sz]], %[[c4]]
+//       CHECK:   return %[[c4]], %[[sz]]
 func.func @memref_expand(%m: memref<?xf32>, %sz: index) -> (index, index) {
-  %0 = memref.expand_shape %m [[0, 1]] output_shape [%sz, 4]: memref<?xf32> into memref<?x4xf32>
-  %1 = "test.reify_bound"(%0) {dim = 0} : (memref<?x4xf32>) -> (index)
-  %2 = "test.reify_bound"(%0) {dim = 1} : (memref<?x4xf32>) -> (index)
+  %0 = memref.expand_shape %m [[0, 1]] output_shape [4, %sz]: memref<?xf32> into memref<4x?xf32>
+  %1 = "test.reify_bound"(%0) {dim = 0} : (memref<4x?xf32>) -> (index)
+  %2 = "test.reify_bound"(%0) {dim = 1} : (memref<4x?xf32>) -> (index)
   return %1, %2 : index, index
 }
 

@jtuyls
Copy link
Contributor Author

jtuyls commented Oct 27, 2025

@matthias-springer Could you help merge this?

@matthias-springer matthias-springer merged commit 66acd04 into llvm:main Oct 27, 2025
12 of 13 checks passed
@jtuyls jtuyls deleted the fix-expand-shape-value-bounds branch October 27, 2025 23:43
Lukacma pushed a commit to Lukacma/llvm-project that referenced this pull request Oct 29, 2025
We shouldn't just consider the dynamic dimensions, but all output
dimensions for the value bounds constraints. The previous test just
worked because the dynamic dimension was on the first position.
aokblast pushed a commit to aokblast/llvm-project that referenced this pull request Oct 30, 2025
We shouldn't just consider the dynamic dimensions, but all output
dimensions for the value bounds constraints. The previous test just
worked because the dynamic dimension was on the first position.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants