This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Allow outer dims perm and untiled dims in pack/unpack generalization
ClosedPublic

Authored by qedawkins on Apr 4 2023, 11:45 AM.

Details

Summary

Extends the pack/unpack generalization patterns to work for any packing
op with only full tiles. This produces a combination of rank-reduced
insert/extract slice ops paired with a transpose on the reduced shape,
similar to what the pattern currently produces for fully tiled
pack/unpacks. Note that only the outer dims are rank-reduced in this
pattern, leaving the shape of the inner tile intact.

Diff Detail

Event Timeline

qedawkins created this revision.Apr 4 2023, 11:45 AM
qedawkins requested review of this revision.Apr 4 2023, 11:45 AM
hanchung accepted this revision.Apr 24 2023, 10:57 AM

Sorry for that late review. Overall looks good to me, just few nits!

mlir/lib/Dialect/Linalg/Transforms/Transforms.cpp
1272–1280

style nit: we can add continue to the end of the first if statement. It can save us one level of nesting. And maybe to others, that could save us more level of nestings.

1419–1423

style nit: wrap with braces because it spans to multiple lines.

1442–1446

delete the comment

This revision is now accepted and ready to land.Apr 24 2023, 10:57 AM
qedawkins updated this revision to Diff 516518.Apr 24 2023, 1:19 PM

Address nits

Sorry for that late review. Overall looks good to me, just few nits!

No problem, thanks for the review!

@hanchung I noticed the pack/unpack lowering patterns exposed in D148867 and am wondering what the purpose of these generalization patterns are given that those patterns exist. It seems to me like duplicated functionality so before landing this I wanted to see if it made more sense to replace the generalization patterns rather than expanding them with this patch.

@hanchung I noticed the pack/unpack lowering patterns exposed in D148867 and am wondering what the purpose of these generalization patterns are given that those patterns exist. It seems to me like duplicated functionality so before landing this I wanted to see if it made more sense to replace the generalization patterns rather than expanding them with this patch.

They cover different subset of generalization. I've been working on pack op codegen and the other one supports some of my use cases. So I start using some of them. I think eventually they should be unified. This one does not generate expand_shape/collapse_shape op while the other does. I think we should extend the other one to handle dynamic cases as well. After that, we can migrate some logics to there, and we will have unified patterns/utils.

This revision was landed with ongoing or failed builds.May 2 2023, 9:29 AM
This revision was automatically updated to reflect the committed changes.

@hanchung I noticed the pack/unpack lowering patterns exposed in D148867 and am wondering what the purpose of these generalization patterns are given that those patterns exist. It seems to me like duplicated functionality so before landing this I wanted to see if it made more sense to replace the generalization patterns rather than expanding them with this patch.

They cover different subset of generalization. I've been working on pack op codegen and the other one supports some of my use cases. So I start using some of them. I think eventually they should be unified. This one does not generate expand_shape/collapse_shape op while the other does. I think we should extend the other one to handle dynamic cases as well. After that, we can migrate some logics to there, and we will have unified patterns/utils.

I see, makes sense, thanks!