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

[mlir][memref] Collapse on strided memref is conservative #112994

Open
nirvedhmeshram opened this issue Oct 18, 2024 · 1 comment
Open

[mlir][memref] Collapse on strided memref is conservative #112994

nirvedhmeshram opened this issue Oct 18, 2024 · 1 comment

Comments

@nirvedhmeshram
Copy link
Contributor

nirvedhmeshram commented Oct 18, 2024

Take this example

func.func @strided_collapse (%in : memref<1x1x16x4x18xf32>) -> memref<1x16x64xf32, strided<[1152, 72, 1]>> {
  %subview_7 = memref.subview %in[0, 0, 0, 0, 0] [1, 1, 16, 4, 16] [1, 1, 1, 1, 1] : memref<1x1x16x4x18xf32> to memref<1x1x16x4x16xf32, strided<[1152, 1152, 72, 18, 1]>>
  %collapse_shape = memref.collapse_shape %subview_7 [[0], [1, 2], [3, 4]] : memref<1x1x16x4x16xf32, strided<[1152, 1152, 72, 18, 1]>> into memref<1x16x64xf32, strided<[1152, 72, 1]>>
  return %collapse_shape : memref<1x16x64xf32, strided<[1152, 72, 1]>>
}

This is a correct op however we will get

> mlir-opt test.mlir 
test.mlir:3:21: error: 'memref.collapse_shape' op invalid source layout map or collapsing non-contiguous dims
  %collapse_shape = memref.collapse_shape %subview_7 [[0], [1, 2], [3, 4]] : memref<1x1x16x4x16xf32, strided<[1152, 1152, 72, 18, 1]>> into memref<1x16x64xf32, strided<[1152, 72, 1]>>

The reason is the verifier uses this function
https://github.com/llvm/llvm-project/blob/main/mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp#L2416
which checks if the src and result strides match as a proof of contiguous collapse but that doesnt seem necessary?

@github-actions github-actions bot added the mlir label Oct 18, 2024
nirvedhmeshram added a commit to iree-org/iree that referenced this issue Oct 24, 2024
…er (#18863)

This is unsupported by upstream and can lead to a compiler error.
llvm/llvm-project#112994

Progress towards: #18858

---------

Signed-off-by: Nirvedh <nirvedh@gmail.com>
@cxy-1993
Copy link
Contributor

It is reasonable for memref.collapse_shape to require contiguity. Its semantics is to collapse multiple dimensions into a single dimension. If the collapsed dimensions are not contiguous, the resulting dimension cannot be described by a single stride. In your scenario, where you only want to eliminate dimensions with a shape of 1, a rank-reduced subview or a direct reinterpret_cast operation is more recommended.

Eliasj42 pushed a commit to iree-org/iree that referenced this issue Oct 31, 2024
…er (#18863)

This is unsupported by upstream and can lead to a compiler error.
llvm/llvm-project#112994

Progress towards: #18858

---------

Signed-off-by: Nirvedh <nirvedh@gmail.com>
Signed-off-by: Elias Joseph <eljoseph@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants