Page MenuHomePhabricator

[mlir] Add file to implement bufferization for shape ops.
ClosedPublic

Authored by tpopp on Sep 22 2020, 3:25 AM.

Details

Summary

This adds a shape-bufferize pass and implements the pattern for
shape.assuming.

Diff Detail

Event Timeline

tpopp created this revision.Sep 22 2020, 3:25 AM
Herald added a reviewer: silvas. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
tpopp requested review of this revision.Sep 22 2020, 3:25 AM
silvas requested changes to this revision.Sep 22 2020, 10:53 AM
silvas added inline comments.
mlir/lib/Dialect/Shape/Transforms/ShapeBufferize.cpp
27 ↗(On Diff #293400)

I think this pattern is actually not specific to bufferization.

I think this pattern can be like populateFuncOpTypeConversionPattern (works with any type converter, not just a BufferAssignmentTypeConverter)

A pattern has a member typeConverter already (you can also call getTypeConverter()), which is all you need here.

I have a couple patterns like this in npcomp that I have yet to upstream and there doesn't seem to be a need for it to be tied specifically to buffer conversion: https://github.com/llvm/mlir-npcomp/blob/681c4e1d4ac879921a852e6243288fd159de222a/lib/E2E/TensorToMemref/LowerStructuralToMemref.cpp#L31

Proposal:

expose populateShapeOpsTypeConversionPatterns which takes a generic TypeConverter.
make this a regular OpConversionPattern

93 ↗(On Diff #293400)

Is "bufferize" the consistent terminology we are going to use from now on upstream for tensor->memref conversion? I don't see that term upstream yet and just wanted to make sure we are agreed on using that term from now on.

mlir/test/Dialect/Shape/shape-bufferize.mlir
4 ↗(On Diff #293400)

typo assumign

This revision now requires changes to proceed.Sep 22 2020, 10:53 AM
pifon2a accepted this revision.Sep 26 2020, 3:10 AM
pifon2a added inline comments.
mlir/lib/Dialect/Shape/Transforms/ShapeBufferize.cpp
27 ↗(On Diff #293400)

Actually, that's a very good point: do we actually need to use BufferAssignmentOpConversionPattern as a base class for ops conversion when we want to go from Tensors to MemRefs if, of course, the op is not a FuncOp, CallOp or ReturnOp. Currently, i don't think there is anything special code in BufferAssignmentOpConversionPattern. In future we might add some custom logging or smth else for the buffer assignment and then it would be useful.

So, I propose to continue using BufferAssignmentOpConversionPatter but create an issue about this. Because, if it's really not needed, then we have to refactor all of the ops that did not really need to derive from BufferAssignmentOpConversionPattern.

36 ↗(On Diff #293400)

just curious: here you keep implementation of the method within the BufferizeAssumingOpConverter definition, but for ShapeBufferizePass you implemented runOnFunction separately.

mlir/test/Dialect/Shape/shape-bufferize.mlir
1 ↗(On Diff #293400)
7 ↗(On Diff #293400)

nit: s/@f()/@shape_assign_returns_memref()and remove the comment

18 ↗(On Diff #293400)

super-nit: remove these 2 empty lines.

tpopp updated this revision to Diff 295821.Oct 2 2020, 6:58 AM
tpopp marked 3 inline comments as done.

Clarify the transformation of tensors to memrefs.
Leave TODOs to generalize this the rest of the way in regards to supported type conversions.

tpopp added inline comments.Oct 2 2020, 7:00 AM
mlir/lib/Dialect/Shape/Transforms/ShapeBufferize.cpp
36 ↗(On Diff #293400)

I don't think I had any specific reason to do them differently.

This revision was not accepted when it landed; it landed in state Needs Review.Oct 6 2020, 2:36 AM
This revision was automatically updated to reflect the committed changes.

Title: "Add file to implement bufferization for shape ops."

You meant to write "Add *pass* to ..." ?

mlir/include/mlir/Dialect/Shape/Transforms/Passes.td
24

FYI we don't usually put usernames in TODO.

mlir/lib/Dialect/Shape/Transforms/ShapeTypeConversion.cpp
2

You have a formatting issue to fix here.

85

(same)

mlir/test/Dialect/Shape/shape-type-conversion.mlir
1

Can you please rework the test to remove the allow-unregistered-dialect? This losen the verifier in a way that isn't really desirable for tests in general.

silvas added inline comments.Oct 6 2020, 12:16 PM
mlir/test/Dialect/Shape/shape-type-conversion.mlir
1

I thought that was the preferred pattern for source/sink ops? May need to revisit my source/sink op proposal? https://llvm.discourse.group/t/rfc-add-std-source-and-std-sink-for-test-case-writing-reduction/1058/16

mehdi_amini added inline comments.Oct 6 2020, 12:59 PM
mlir/test/Dialect/Shape/shape-type-conversion.mlir
1

I make a distinction between "unregistered op" and "unregistered dialect" though.
In-tree you can write a test with test.custom_shape_sink(...) for example without registering the operation and you don't need to allow unregistered dialect for this for example.

silvas added inline comments.Oct 6 2020, 1:08 PM
mlir/test/Dialect/Shape/shape-type-conversion.mlir
1

Ah, got it!

mehdi_amini added inline comments.Oct 6 2020, 1:13 PM
mlir/test/Dialect/Shape/shape-type-conversion.mlir
1

(and it wasn't needed here actually, removed in https://github.com/llvm/llvm-project/commit/5a305f81bfc3 )

tpopp marked 8 inline comments as done.Oct 7 2020, 1:03 AM