This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Add a transform.structured.lower_pack op
ClosedPublic

Authored by nicolasvasilache on Jan 30 2023, 3:05 AM.

Details

Summary

This revision introduces transform.structured.lower_pack which allows
rewriting a tensor.pack to tensor.pad + tensor.expand_shape + linalg.transpose.

The implementation is currently limited to static pack ops that do not have outer_dims permutations.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Jan 30 2023, 3:05 AM
nicolasvasilache retitled this revision from [mlir][Linalg] Add a transform.structured.pack_to_pad op to [mlir][Linalg] Add a transform.structured.lower_pack op.
nicolasvasilache edited the summary of this revision. (Show Details)

Update commit message.

chelini added inline comments.Jan 30 2023, 7:06 AM
mlir/include/mlir/Dialect/Linalg/Transforms/Transforms.h
1145

I think here should be pack and not packTranspose.

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
147

llvm::zip_equal?

180

I find this example a bit hard to follow, why even considering an offset of 0?. But if I understood the code basically we want to have the "new" position for the point loops to bring back the tensor in the form: AaBbCD where 'a' and 'b' are the point loops.

823

why do we need to assert? Can we fail before?

mlir/test/Dialect/Linalg/transform-lower-pack.mlir
2

For now, I would drop split-input-file.

7

I would remove "First" as there is only one pack.

13

why not linalg.transpose?

nicolasvasilache marked 7 inline comments as done.Jan 30 2023, 12:09 PM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
180

Improved doc to explain the offset part, please lmk if that helps and if you see a better way to explain.

nicolasvasilache marked an inline comment as done.
nicolasvasilache edited the summary of this revision. (Show Details)

Address comments.

hanchung added inline comments.Jan 30 2023, 8:34 PM
mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
785–786

maybe it's worth refactoring some logic from GeneralizeOuterUnitDimsPackOpPattern pattern and call the method here.
https://github.com/llvm/llvm-project/blob/5f1c6d4444f770c3442a8f00ae813678e180b20a/mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp#L526

It is a special case that all outer dims are 1. In the case, we don't need tensor.collapse_shape op. The pack op can be converted into tensor.extract_slice + linalg.transpose + tensor.insert_slice ops.

(I'm not asking you for doing it in the commit. I'd like to learn if it's on your radar or not. I can help for doing it if it sounds good.)

861–863

delete the comment?

nicolasvasilache marked 2 inline comments as done.

Drop comment.

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
785–786

Thanks @hanchung !
It is not on my radar as I am focusing on the pure op version atm.
However you refactoring proposal makes sense to me, the more logic can be commonalized into well-identified reusable helpers, the better.

chelini accepted this revision.Jan 31 2023, 8:39 AM

Thanks, looks good to me.

mlir/lib/Dialect/Linalg/TransformOps/LinalgTransformOps.cpp
179

nit: wether -> whether

This revision is now accepted and ready to land.Jan 31 2023, 8:39 AM
This revision was landed with ongoing or failed builds.Jan 31 2023, 10:06 AM
This revision was automatically updated to reflect the committed changes.