This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][Tensor] Add canonicalization patterns for `tensor.pack`
ClosedPublic

Authored by chelini on Jan 6 2023, 4:49 AM.

Details

Summary
  • Fold an unpack(pack(x)) to x.
  • Rewrite a tensor.pack to an tensor.expand_shape if only one dimension is packed.

Diff Detail

Event Timeline

chelini created this revision.Jan 6 2023, 4:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2023, 4:49 AM
chelini requested review of this revision.Jan 6 2023, 4:49 AM
chelini updated this revision to Diff 486817.Jan 6 2023, 4:57 AM

Take into account padding semantics.

chelini updated this revision to Diff 486818.Jan 6 2023, 5:05 AM

Add test for padding

rengolin added inline comments.Jan 6 2023, 5:39 AM
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
3440

Not clear why you outlined this code. Seems simple enough to be inside matchAndRewrite.

mravishankar requested changes to this revision.Jan 6 2023, 12:08 PM
mravishankar added inline comments.
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
3424

Does this need to check for padding semantics? Maybe not, but I am not sure.

3437

This doesnt look like a canonicalization to me. I'd rather add a separate "populate*" method to pull this in.

This revision now requires changes to proceed.Jan 6 2023, 12:08 PM
chelini updated this revision to Diff 487447.Jan 9 2023, 8:09 AM

Address comments:

  • Add a populate* method to simplify tensor.pack.
rengolin added inline comments.Jan 9 2023, 8:44 AM
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
3424

I think we do. Both padding and inner tiles.

If the pack has padding, than the packed tensor may be "unpackable" unless we define the operations on the padding irrelevant. But if that's true, then we need to let unpack know that this is what we've done, so that it can match its output shape to be the same as the original (not sure how, since pack doesn't have an amount to pad, just a constant to use as padding).

If the unpack has different tiles as the pack, then the order of elements will be different and will create a different output tensor, so we can't collapse the pair.

The size of the output tensor has to validate from the shape of the type. In both pad and uneven tiling cases, we have to make sure that the pre-packed shape is identical to the post-unpacked shape. We can do that via use-def chains or by writing a strict shape policy and carry padding sizes around.

None of those things are simple, so I'd just avoid them for now. Just make sure there's no padding and the inner tiles are the same, too.

hanchung added inline comments.Jan 9 2023, 10:47 AM
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
3037–3038

I think using RankedTensorType is a bit more accurate.

3053

I looked into other file and think this should be // namespace.

mlir/test/lib/Dialect/Tensor/TestTensorTransforms.cpp
310–312
chelini updated this revision to Diff 487730.Jan 10 2023, 2:54 AM
  • Bail out if tensor.pack has padding
  • Check tiles for pack and unpack to be the same when canonicalize chain
  • Use single-braces in TestTensorTransforms.cpp
  • Fix namespace: end namespace -> namespace
  • Add more tests with dynamic tiles/dims
  • Use RankedTensorType instead if ShapedType in SimplifyPackToExandShape
  • Improve code reuse between pack::canonicalize and unpack::canonicalize
chelini marked 5 inline comments as done.Jan 10 2023, 2:55 AM
chelini marked an inline comment as done.

Thanks @chelini, LGTM now, but I'll let others make the final approval.

hanchung accepted this revision.Jan 10 2023, 10:36 PM
hanchung added inline comments.
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
3028–3030

[optional] The operandType is only used once; it does not improve readability. The method itself describes it. I think we can save one line here.

3422

My experience tells me that we seldom add braces to it (though I could not find style guide about it).

tyb0807 accepted this revision.Jan 11 2023, 12:37 AM

Thank you, LGTM

chelini updated this revision to Diff 488104.Jan 11 2023, 12:50 AM
  • Remove braces
  • Address comment on operandType
chelini updated this revision to Diff 488105.Jan 11 2023, 12:51 AM

Fix compile error

chelini marked 2 inline comments as done.Jan 11 2023, 12:52 AM
mravishankar accepted this revision.Jan 11 2023, 6:06 PM
mravishankar added inline comments.
mlir/lib/Dialect/Tensor/IR/TensorOps.cpp
3424

Agreed. I didnt see that the padding semantics is checked in the pattern. If the pack has padding, then we cant simply fold the pack and unpack away. Cause pack is doing more than what unpack is doing. THats what my concern was. I dont fully follow everything stated above, but the code itself looks fine.

This revision is now accepted and ready to land.Jan 11 2023, 6:06 PM