This is an archive of the discontinued LLVM Phabricator instance.

[mlir][tensor][linalg] Enhance pack op propagation across generic ops.
ClosedPublic

Authored by hanchung on Dec 8 2022, 3:57 PM.

Details

Summary

Considering the case that generic + pack (with outer_dim_perms), the
truth is that it is equipvelent to generic + pack + transpose. There are
two steps to bubble up the pack op accross the generic op.

Step 1. swap generic + pack -> pack + generic.

In this step, we can bind the packing information to dimensions of
iteration domain. With the information, we can pack the operands with
corresponding data tile sizes; the packed inner dimensions will be
appended to the indexing_maps. Note that the outer dimensions of
indexing maps are not changed at all.

Step 2. Fold the transpose into generic op.

The step two is just updating the indexing map, so we do not have to
handle outer_dim_perms anymore.

There could be step 3 to extract the transpose op out (i.e., generic ->
transpose + generic), then we can fold the transpose into the pack op.
This step is not done in the revision.

Co-authored-by: Lorenzo Chelini <l.chelini@icloud.com>

Diff Detail

Event Timeline

hanchung created this revision.Dec 8 2022, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 3:57 PM
hanchung requested review of this revision.Dec 8 2022, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 8 2022, 3:57 PM
hanchung edited the summary of this revision. (Show Details)Dec 8 2022, 3:58 PM

Small nit, but this looks great, thanks!

Give some time for other people to have their comments, but looks good from my side.

mlir/lib/Dialect/Linalg/Transforms/DataLayoutPropagation.cpp
245

This is a bit fragile, being the only element that isn't extracted from getPackingInfoFromConsumer.

Since we're passing multiple args, it could be a last arg, default empty, and we just pass packOp.getPaddingValue() here.

chelini added inline comments.Dec 9 2022, 4:52 AM
mlir/lib/Dialect/Linalg/Transforms/DataLayoutPropagation.cpp
38

I don't think we need this, or at least for now. It is the number of dims in the output map.

44

I would prefer a different name. Maybe tiledDimsPos?

92–93

Assumption 3 here does not apply anymore.

121–122

int64_t?

145–148

int64_t?

152

Yes, I think this is desirable if we want to fuse in the packed layout (i.e., a packed GEMM followed by a RELU).

chelini added inline comments.Dec 9 2022, 10:29 AM
mlir/lib/Dialect/Linalg/Transforms/DataLayoutPropagation.cpp
42

packedDims -> tileToPointMapping?

chelini added inline comments.Dec 9 2022, 10:32 AM
mlir/lib/Dialect/Linalg/Transforms/DataLayoutPropagation.cpp
40

Thinking about name. What do you think of: packedTileSize -> domDimAndTileMapping?

rengolin added inline comments.Dec 12 2022, 2:29 AM
mlir/lib/Dialect/Linalg/Transforms/DataLayoutPropagation.cpp
38

This is used al over the place below, to append the dims in packedDims, build affine maps, etc. What do you mean "we don't need this"?

chelini added inline comments.Dec 12 2022, 2:33 AM
mlir/lib/Dialect/Linalg/Transforms/DataLayoutPropagation.cpp
38

I don't think is needed as a member variable in the struct. It is used in line 56/64/256 and it is the number of dimensions in the output map, we can simply get from the map itself instead of storing it in the struct.

hanchung updated this revision to Diff 482319.Dec 12 2022, 5:21 PM
hanchung marked 10 inline comments as done.

address comments

also thanks for the better names!

mlir/lib/Dialect/Linalg/Transforms/DataLayoutPropagation.cpp
38

removed

152

I'm still trying to figure out how to make it work. Let's do it in a separate revision?

245

good point, thanks!

chelini added inline comments.Dec 13 2022, 7:03 AM
mlir/lib/Dialect/Linalg/Transforms/DataLayoutPropagation.cpp
152

Sounds good to me.

chelini accepted this revision.Dec 13 2022, 12:01 PM
This revision is now accepted and ready to land.Dec 13 2022, 12:01 PM