Page MenuHomePhabricator

Support buffers in LinalgFoldUnitExtentDims
ClosedPublic

Authored by tpopp on Wed, Jun 2, 4:51 AM.

Details

Summary

This doesn't add any canonicalizations, but executes the same
simplification on bufferSemantic linalg.generic ops by using
linalg::ReshapeOp instead of linalg::TensorReshapeOp.

Diff Detail

Event Timeline

tpopp created this revision.Wed, Jun 2, 4:51 AM
tpopp requested review of this revision.Wed, Jun 2, 4:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jun 2, 4:51 AM
tpopp updated this revision to Diff 349235.Wed, Jun 2, 4:53 AM

Update a comment

mravishankar added a comment.EditedWed, Jun 2, 4:01 PM

Interesting that this makes it work for memref types. A quick glance at it, it looks mostly OK. One of the things though is that I am not sure how valid this will be when we have memref with strides. Maybe to be conservative, only do this when the operands dont have memrefs with affine maps?

mravishankar requested changes to this revision.Wed, Jun 2, 4:03 PM

Have a comment above (clicked enter early there). Will review more, but if we are going down this path, do we just replicate all the tests here for tensors with memrefs as well?

This revision now requires changes to proceed.Wed, Jun 2, 4:03 PM
tpopp added a comment.Mon, Jun 7, 3:05 AM

Have a comment above (clicked enter early there). Will review more, but if we are going down this path, do we just replicate all the tests here for tensors with memrefs as well?

If it's agreed upon as an acceptable idea, I can create a clone of all relevant tests (with an additional mixed mode that would be supported in the latest patch). Primarily I don't want to do the additional work while there is risk of it being discarded.

Interesting that this makes it work for memref types. A quick glance at it, it looks mostly OK. One of the things though is that I am not sure how valid this will be when we have memref with strides. Maybe to be conservative, only do this when the operands dont have memrefs with affine maps?

Good catch. This is added. To add the additional check no longer creates the simple templating, so I needed to setup 2 classes with mostly shared functionality or to have one class do everything. I went with a single class because this also supports linalg.generics with mixed types.

Also, please feel free to add any other reviewers if they are appropriate. I wasn't sure who was appropriate as refactors have created many authors of this file.

tpopp updated this revision to Diff 350223.Mon, Jun 7, 3:08 AM

Create a single method to support both Tensor and MemRef operands.

mravishankar accepted this revision.Fri, Jun 11, 12:49 PM

LGTM-ing this. But needs to rebased on ToT before landing.

mlir/lib/Dialect/Linalg/Transforms/DropUnitDims.cpp
413

Nit: please use { } when the body spans multiple lines (even though its one statement)

416

This might not work on ToT now since reshape and tensor_reshape are split into collapse_* and expand_* variants. Suggest moving the reshape generation logic (to compute the reshapedValue ) into a separate method.

This revision is now accepted and ready to land.Fri, Jun 11, 12:49 PM
tpopp updated this revision to Diff 351804.Mon, Jun 14, 2:58 AM
tpopp marked 2 inline comments as done.

Rebase, add tests, and extract reshape logic into separate methods.

This revision was landed with ongoing or failed builds.Mon, Jun 14, 11:22 PM
This revision was automatically updated to reflect the committed changes.