This is an archive of the discontinued LLVM Phabricator instance.

Relax FuseTensorReshapeOpAsproducer identity mapping constraint
ClosedPublic

Authored by asaadaldien on Oct 5 2020, 8:55 PM.

Details

Summary

Allow fusing linalg.tensor_reshape with its consumer when the consumer is applying a permutation map (e.g linalg.generic implementing a transpose)

Diff Detail

Event Timeline

asaadaldien created this revision.Oct 5 2020, 8:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2020, 8:55 PM
asaadaldien requested review of this revision.Oct 5 2020, 8:55 PM
asaadaldien edited the summary of this revision. (Show Details)Oct 5 2020, 8:57 PM
asaadaldien added a reviewer: maheshkhanwalkar.
hanchung added inline comments.
mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp
328

I think isPermutation covers isIdentity, so maybe no need to check the first case, ie, useIndexMap.isIdentity() || ?

384–386

I think we actually just want to make sure the map is invertible, so the comment is a bit not accurate. The identity map or permutation map is guaranteed to be invertible, so you don't need to check if invMap is null.

comments...

mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp
328

Checked, isPermutation doesn't exclude identity.

384–386

The comment above implies consumer map fusedIndexMaps[consumerIdx] is invertible it's just phrased differently.

mravishankar requested changes to this revision.Oct 6 2020, 10:18 AM

Thanks Ahmed, just one minor comment.

mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp
328

Do we need to make a distinction for when it is asProducer and not. I think you can make it isPermutation always? Then you can maybe also add a test where the reshape is the consumer?

This revision now requires changes to proceed.Oct 6 2020, 10:18 AM
asaadaldien added a comment.EditedOct 6 2020, 12:52 PM

Thanks Ahmed, just one minor comment.

Thanks Ahmed, just one minor comment.

I need to look closely into FuseTensorReshapeOpAsConsumer to see why it isn't producing the correct indexing map for the permutation case. I can do this in a follow up diff.

NVM, that was a silly test bug :D, ignore my comment.

enable premutations for tensor_reshape as consumer

mravishankar accepted this revision.Oct 6 2020, 2:46 PM
This revision is now accepted and ready to land.Oct 6 2020, 2:46 PM