LGTM! @ftynse could you help to double check because I am not super confident either.

I left some comments but I am also pretty new to this. Hope someone who's expert on this help to take a look.

Move the comment about dynamic stride to a more propriate place

Thanks for the review!

Address comments.

Fixed the previous error about dominance.

Calculate with the new logic at runtime. I got an error about dominance here https://gist.github.com/949dafb96801866cf067b5baba4ac614. It's complaining the last mem ref descriptor doesn't dominate the use tensor.collapse_shape but it seems to me it does dominate tensor.collapse_shape.

Fix test cases

Address comments

Factored out the part fixing size 1 cases into https://reviews.llvm.org/D124137.
The remaining memref to llvm lowering part needs to be updated to
take into consideration where the dynamic size is 1 as well.

Fix testcases format

Clean ups

• Fix cases where all collapsed dims in an association are size 1.
• Fix tests.

LGTM. Thanks for the patch!
@gysit Could you help to double check?

address comments

minor fix

remove redundant argument

minor changes to commit title

fix with canonicalizer pattern instead of folder

I think a better way to fix this is to use a canonicalizing pattern. Will modify the patch to do that.

@mravishankar Thanks a lot for reviewing! I don't have commit permission. Could you please also help me to land the change?

In D106658#2900540, @gysit wrote:

Will do that!

Btw, the commit message should usually contain some tags, e.g. "[mlir][linalg] Add pooling_nchw_max, conv_2d_nchw as yaml ops." to clarify which parts of llvm it is touching.

Will do it before landing.

@gysit Hey Tobias, thanks a lot for reviewing! Could you please also help me land the change? I don't have commit access yet.

update description

clean up format

minor changes

LGTM

remove the integration test and add IR unit test instead

Add an integration test for dynamic shape case when testing padtensor
lowering. Add comments to be more descriptive.

In D105642#2864966, @silvas wrote:

Do you know why previous tests didn't need this? Should we add a new test?

delete redundant comments as suggested

move the new pattern's source code next to PadTensorOpTransformationPattern for
consistency

add the pattern into linalg bufferization pass

@silvas @nicolasvasilache @springerm Any suggestion on which pass to put this pattern in? There is a LinalgGeneralizationPass which currently only applies to patterns lowering to Linalg.generic.

update comments that's no longer relevant.

refactor and reuse existing code as suggested.

@nicolasvasilache seems with the rewrite pattern version the integration test passed but there are some failures in the unit test. Will need to take a closer look tmr. Here is the unit test.

func @pad_tensor(%arg0: tensor<4x?x2x?xf32>, %arg1: index) -> tensor<4x?x?x?xf32> {
  %cst = constant 0.0 : f32
  %out = linalg.pad_tensor %arg0 low[0, 0, %arg1, 0] high[0, 0, 0, %arg1]  {
  ^bb0(%gen_arg1: index, %gen_arg2: index, %gen_arg3: index, %gen_arg4: index):  // no predecessors
    linalg.yield %cst : f32
  } : tensor<4x?x2x?xf32> to tensor<4x?x?x?xf32>
  return %out : tensor<4x?x?x?xf32>
}

use rewrite pattern instead

In D105293#2854084, @silvas wrote:

In D105293#2854047, @nicolasvasilache wrote:

Note that I asked for a similar rewrite in https://reviews.llvm.org/D102804 but it was eventually not done.
I am wondering whether the pattern you add here should be more generally be the rewrite on tensors I mentioned and then just let existing bufferization kick in?

That makes sense to me!

address comments

remove redundant type check

delete redundant comments

didn't see https://github.com/llvm/llvm-project/commit/113baa2b9fd3c8db30d33ecc3f068af48dcce52d. closing this one.