This adds a shape-bufferize pass and implements the pattern for
shape.assuming.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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 |
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) | --dump-input=fail is the default value |
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. |
Clarify the transformation of tensors to memrefs.
Leave TODOs to generalize this the rest of the way in regards to supported type conversions.
mlir/lib/Dialect/Shape/Transforms/ShapeBufferize.cpp | ||
---|---|---|
36 ↗ | (On Diff #293400) | I don't think I had any specific reason to do them differently. |
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. |
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 |
mlir/test/Dialect/Shape/shape-type-conversion.mlir | ||
---|---|---|
1 | I make a distinction between "unregistered op" and "unregistered dialect" though. |
mlir/test/Dialect/Shape/shape-type-conversion.mlir | ||
---|---|---|
1 | Ah, got it! |
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 ) |
FYI we don't usually put usernames in TODO.