Skip to content

Conversation

@nirvedhmeshram
Copy link
Contributor

Other convolutions such as conv_2d_nchw_fchw have a output affine map with no permutations and the input and the filter map are adjusted accordingly. This makes conv_3d_ncdhw_fcdhw a stand out so this PR changes the affine map to be consistent with other variants.

@llvmbot
Copy link
Member

llvmbot commented Mar 3, 2025

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-linalg

Author: Nirvedh Meshram (nirvedhmeshram)

Changes

Other convolutions such as conv_2d_nchw_fchw have a output affine map with no permutations and the input and the filter map are adjusted accordingly. This makes conv_3d_ncdhw_fcdhw a stand out so this PR changes the affine map to be consistent with other variants.


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

1 Files Affected:

  • (modified) mlir/include/mlir/Dialect/Linalg/IR/LinalgNamedStructuredOps.yaml (+4-4)
diff --git a/mlir/include/mlir/Dialect/Linalg/IR/LinalgNamedStructuredOps.yaml b/mlir/include/mlir/Dialect/Linalg/IR/LinalgNamedStructuredOps.yaml
index 496a323249e85..2bae43f887b04 100644
--- a/mlir/include/mlir/Dialect/Linalg/IR/LinalgNamedStructuredOps.yaml
+++ b/mlir/include/mlir/Dialect/Linalg/IR/LinalgNamedStructuredOps.yaml
@@ -4072,12 +4072,12 @@ structured_op: !LinalgStructuredOpConfig
   indexing_maps: !LinalgIndexingMapsConfig
     static_indexing_maps:
     - affine_map<(d0, d1, d2, d3, d4, d5, d6, d7, d8)[s0, s1, s2, s3, s4, s5, s6,
-      s7, s8, s9, s10, s11, s12, s13, s14] -> (d0, d8, d1 * s3 + d5 * s5, d2 * s7
-      + d6 * s9, d3 * s11 + d7 * s13)>
+      s7, s8, s9, s10, s11, s12, s13, s14] -> (d0, d8, d2 * s3 + d5 * s5, d3 * s7
+      + d6 * s9, d4 * s11 + d7 * s13)>
     - affine_map<(d0, d1, d2, d3, d4, d5, d6, d7, d8)[s0, s1, s2, s3, s4, s5, s6,
-      s7, s8, s9, s10, s11, s12, s13, s14] -> (d4, d8, d5, d6, d7)>
+      s7, s8, s9, s10, s11, s12, s13, s14] -> (d1, d8, d5, d6, d7)>
     - affine_map<(d0, d1, d2, d3, d4, d5, d6, d7, d8)[s0, s1, s2, s3, s4, s5, s6,
-      s7, s8, s9, s10, s11, s12, s13, s14] -> (d0, d4, d1, d2, d3)>
+      s7, s8, s9, s10, s11, s12, s13, s14] -> (d0, d1, d2, d3, d4)>
   iterator_types:
   - parallel
   - parallel

@jerryyin
Copy link
Member

jerryyin commented Mar 3, 2025

So effectively, for (d0, d1, d2, d3, d4, d5, d6, d7, d8) the interpretation:

  • Previously: (N, D, H, W, F, Df, Hf, Wf, C)
  • Now: (N, F, D, H, W, Df, Hf, Wf, C)

For 2d nchw_fchw convolution's interpretations is (N, F, H, W, C, Hf, Wf), per

indexing_maps: !LinalgIndexingMapsConfig
static_indexing_maps:
- affine_map<(d0, d1, d2, d3, d4, d5, d6)[s0, s1, s2, s3, s4, s5, s6, s7, s8,
s9, s10] -> (d0, d4, d2 * s3 + d5 * s5, d3 * s7 + d6 * s9)>
- affine_map<(d0, d1, d2, d3, d4, d5, d6)[s0, s1, s2, s3, s4, s5, s6, s7, s8,
s9, s10] -> (d1, d4, d5, d6)>
- affine_map<(d0, d1, d2, d3, d4, d5, d6)[s0, s1, s2, s3, s4, s5, s6, s7, s8,
s9, s10] -> (d0, d1, d2, d3)>

To be aligned in both cases I assume you also need to make 3d conv's C dimension d5 rather than the d8 of affine map?

@nirvedhmeshram
Copy link
Contributor Author

So effectively, for (d0, d1, d2, d3, d4, d5, d6, d7, d8) the interpretation:

  • Previously: (N, D, H, W, F, Df, Hf, Wf, C)
  • Now: (N, F, D, H, W, Df, Hf, Wf, C)

For 2d nchw_fchw convolution's interpretations is (N, F, H, W, C, Hf, Wf), per

indexing_maps: !LinalgIndexingMapsConfig
static_indexing_maps:
- affine_map<(d0, d1, d2, d3, d4, d5, d6)[s0, s1, s2, s3, s4, s5, s6, s7, s8,
s9, s10] -> (d0, d4, d2 * s3 + d5 * s5, d3 * s7 + d6 * s9)>
- affine_map<(d0, d1, d2, d3, d4, d5, d6)[s0, s1, s2, s3, s4, s5, s6, s7, s8,
s9, s10] -> (d1, d4, d5, d6)>
- affine_map<(d0, d1, d2, d3, d4, d5, d6)[s0, s1, s2, s3, s4, s5, s6, s7, s8,
s9, s10] -> (d0, d1, d2, d3)>

To be aligned in both cases I assume you also need to make 3d conv's C dimension d5 rather than the d8 of affine map?

Good catch, fixed that as well!

@nirvedhmeshram nirvedhmeshram merged commit 186ae8c into llvm:main Mar 3, 2025
11 checks passed
jerryyin pushed a commit to iree-org/iree that referenced this pull request Mar 6, 2025
…20150)

The orginal generic im2col implementation assumed that the filter and
output affine map would not have permutations. While this is true in
most named ops, it is not necessarily required. See for example
llvm/llvm-project#129547 where it was not the
case for `conv_3d_ncdhw_fcdhw` . While in this case we just "normalized"
the map upstream, this PR adds support for such maps directly.
Fixes :   #20139

---------

Signed-off-by: Nirvedh Meshram <[email protected]>
@HerrCai0907
Copy link
Contributor

When I update LinalgNamedStructuredOps.yaml with ./bin/update_core_linalg_named_ops.sh. The change in this PR is reverted. Could you also fix the auto generator for it?

nirvedhmeshram added a commit that referenced this pull request Apr 1, 2025
#129547 changed the IR directly
without updating the auto generate script.

Signed-off-by: Nirvedh <[email protected]>
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Apr 1, 2025
… (#133927)

llvm/llvm-project#129547 changed the IR directly
without updating the auto generate script.

Signed-off-by: Nirvedh <[email protected]>
makslevental pushed a commit to makslevental/python_bindings_fork that referenced this pull request Aug 14, 2025
llvm/llvm-project#129547 changed the IR directly
without updating the auto generate script.

Signed-off-by: Nirvedh <[email protected]>
makslevental pushed a commit to makslevental/python_bindings_fork that referenced this pull request Aug 14, 2025
llvm/llvm-project#129547 changed the IR directly
without updating the auto generate script.

Signed-off-by: Nirvedh <[email protected]>
makslevental pushed a commit to makslevental/python_bindings_fork that referenced this pull request Aug 14, 2025
llvm/llvm-project#129547 changed the IR directly
without updating the auto generate script.

Signed-off-by: Nirvedh <[email protected]>
bump-llvm bot pushed a commit to makslevental/python_bindings_fork that referenced this pull request Aug 14, 2025
llvm/llvm-project#129547 changed the IR directly
without updating the auto generate script.

Signed-off-by: Nirvedh <[email protected]>
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.

4 participants