Page MenuHomePhabricator

[mlir] Add a bufferization pattern for linalg.tiled_loop.
ClosedPublic

Authored by pifon2a on Aug 9 2021, 6:01 AM.

Diff Detail

Event Timeline

pifon2a created this revision.Aug 9 2021, 6:01 AM
pifon2a requested review of this revision.Aug 9 2021, 6:01 AM
herhut accepted this revision.Aug 9 2021, 11:05 AM

The type converter is a bit of a hack but ok within a test pass to test expected behavior I think.

Otherwise just some nits.

mlir/lib/Dialect/Linalg/Transforms/Bufferize.cpp
269

Should this also check that the block is owned by the TiledLoopOp? You could check whether it is a block arg and then whether the immediate parent op of the blockarg is a TiledLoopOp.

319

Here, too.

449

That change will likely not land. Maybe rephrase that this is needed while dialect conversion does not properly support memrefs with maps.

This revision is now accepted and ready to land.Aug 9 2021, 11:05 AM
pifon2a updated this revision to Diff 365269.Aug 9 2021, 12:49 PM
pifon2a marked 3 inline comments as done.

Address the comments.

This revision was landed with ongoing or failed builds.Aug 9 2021, 12:57 PM
This revision was automatically updated to reflect the committed changes.
silvas added a subscriber: silvas.EditedAug 9 2021, 4:25 PM

It feels like this bufferization pattern would miscompile if we were to write integration tests analogous to these:

https://github.com/llvm/llvm-project/blob/main/mlir/test/Integration/Dialect/Linalg/CPU/test-subtensor-insert.mlir
https://github.com/llvm/llvm-project/blob/main/mlir/test/Integration/Dialect/Linalg/CPU/test-subtensor-insert-multiple-uses.mlir

We've tried multiple times to reconcile the desire for in-placeness with the bufferization framework via local changes to patterns, and it never ends well. I feel like we should put more effort into Nicolas' comprehensive bufferize which does the necessary analysis, or implementing a suitable generalization of the current bufferization framework (basically, each pattern would declare if it was going to create aliases / operate in place, and the framework would need to insert copies as needed, similar to how comprehensive bufferize does). Either way, I feel like this patch is a step in the wrong direction. Also, the fact that adding a bufferization pattern for TiledLoop required changing the bufferization pattern for LinalgOp seems like a strong smell to me.

Can we please revert this and work on a more sustainable direction?

@silvas Agreed. I am working on a less invasive pattern to bufferize TiledLoopOp, it does not require changes to Extract/InsertSlice patterns and to BufferizeAnyLinalgOp one. If I am not ready to send it out for review today, then I will revert this one.

It feels like this bufferization pattern would miscompile if we were to write integration tests analogous to these:

https://github.com/llvm/llvm-project/blob/main/mlir/test/Integration/Dialect/Linalg/CPU/test-subtensor-insert.mlir
https://github.com/llvm/llvm-project/blob/main/mlir/test/Integration/Dialect/Linalg/CPU/test-subtensor-insert-multiple-uses.mlir

We've tried multiple times to reconcile the desire for in-placeness with the bufferization framework via local changes to patterns, and it never ends well. I feel like we should put more effort into Nicolas' comprehensive bufferize which does the necessary analysis, or implementing a suitable generalization of the current bufferization framework (basically, each pattern would declare if it was going to create aliases / operate in place, and the framework would need to insert copies as needed, similar to how comprehensive bufferize does). Either way, I feel like this patch is a step in the wrong direction. Also, the fact that adding a bufferization pattern for TiledLoop required changing the bufferization pattern for LinalgOp seems like a strong smell to me.

Can we please revert this and work on a more sustainable direction?