Page MenuHomePhabricator

[mlir][Linalg] Allow tiling of batch dimension for convolution ops with padding.
ClosedPublic

Authored by mravishankar on Mon, Mar 23, 11:32 AM.

Details

Summary

Existing tiling implementation of Linalg would still work for tiling
the batch dimensions of the convolution op.

Depends On D76415

Diff Detail

Event Timeline

mravishankar created this revision.Mon, Mar 23, 11:32 AM
bondhugula requested changes to this revision.Mon, Mar 23, 12:23 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
340

Nit: Missing period at the end.

436

You don't need the getOperation().

This revision now requires changes to proceed.Mon, Mar 23, 12:23 PM
bondhugula added inline comments.Mon, Mar 23, 12:30 PM
mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
436

Sorry, you do need it here.

hanchung requested changes to this revision.Mon, Mar 23, 3:22 PM

Add a test?

mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
343

Use isZero(val)?

Shouldn't there be a test case here?

mravishankar marked 4 inline comments as done.

Addressing comments and adding test

Sorry for the delay. I thought I was blocked on pushing on this (but I really wasnt).

hanchung accepted this revision.Thu, Mar 26, 5:04 PM

LGTM, would be good to wait until Nicolas taking a look.

mlir/lib/Dialect/Linalg/Transforms/Tiling.cpp
437–438

same here, missing a period at the end.

Addressing minor comment and rebasing

@bondhugula : Gentle ping on this patch. I think it is ready to land now. PTAL.

@bondhugula : Gentle ping on this patch. I think it is ready to land now. PTAL.

Hi Mahesh, you don't have to wait for my approval; I just commented superficially on what I noticed. Please rely on the deeper reviews from others.

This revision is now accepted and ready to land.Tue, Mar 31, 12:28 AM
nicolasvasilache accepted this revision.Tue, Mar 31, 6:08 AM
This revision was automatically updated to reflect the committed changes.