This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Retire Linalg's Vectorization Pattern
ClosedPublic

Authored by guraypp on Sep 12 2022, 7:25 AM.

Details

Summary

This revision retires the LinalgCodegenStrategy vectorization pattern. Please see the context: https://discourse.llvm.org/t/psa-retire-linalg-filter-based-patterns/63785.
This revision improves the transform dialect's VectorizeOp in different ways below:

  • Adds LinalgDialect as a dependent dialect. When transform.structured.vectorize vectorizes tensor.pad, it generates linalg.init_tensor. In this case, linalg dialect must be registered.
  • Inserts CopyVectorizationPattern in order to vectorize memref.copy.
  • Creates two attributes: disable_multi_reduction_to_contract_patterns and disable_transfer_permutation_map_lowering_patterns. They are limiting the power of vectorization and are currently intended for testing purposes.

It also removes some of the "CHECK: vector.transfer_write" in the vectorization.mlir test. They are redundant writes, at the end of the code there is a rewrite to the same place. Transform dialect no longer generates them.

Depends on D133684 that retires the LinalgCodegenStrategy vectorization pass.

Diff Detail

Event Timeline

guraypp created this revision.Sep 12 2022, 7:25 AM
Herald added a project: Restricted Project. · View Herald Transcript
guraypp requested review of this revision.Sep 12 2022, 7:25 AM
guraypp updated this revision to Diff 459455.Sep 12 2022, 7:31 AM

removed accidental code in the test

guraypp updated this revision to Diff 459704.Sep 13 2022, 3:28 AM

update the pattern

guraypp updated this revision to Diff 459717.Sep 13 2022, 5:09 AM

Add two options reduction_to_contract and permutation_map to VectorizeOp so we can use them for the tests in vectorization.mlir. Nevertheless, there are still tests failing. This revision marks the failing tests as "CHECKFAILS".

mlir/test/lib/Dialect/Linalg/TestLinalgTransforms.cpp
438

This should go away completely.
Fine to do in a separate commit but please don't leave it behind.

mlir/test/Dialect/Linalg/vectorization.mlir
373

memref.copy vectorization can be added by adding patterns.add<CopyVectorizationPattern>(ctx); to the VectorizeOp.

794

tensor.pad vectorization can be activated with the vectorize_padding = true flag on the transform.structured.vectorize

guraypp updated this revision to Diff 460014.Sep 14 2022, 2:47 AM

This commit changes the followings:

Adds LinalgDialect as dependent dialect. Because tensor.pad generates linalg.init_tensor. 
Adds CopyVectorizationPattern pattern.
Deletes "CHECK: vector.transfer_write" in the test. They look redundant writes to me.
guraypp updated this revision to Diff 460015.Sep 14 2022, 2:50 AM

fix indendation

The description should be part of the commit message.

This commit changes the followings:

Adds LinalgDialect as dependent dialect. Because tensor.pad generates linalg.init_tensor.

This needs to be better explained, adding dialect dependencies is not a trivial thing.
What is the dependence added to, why is it necessary now and wasn't before etc.

Adds CopyVectorizationPattern pattern.

This revision also adds the CopyVectorizationPattern to the list of patterns applied by VectorizeOp::applyToOne.

Deletes "CHECK: vector.transfer_write" in the test. They look redundant writes to me.
mlir/include/mlir/Dialect/Linalg/TransformOps/LinalgTransformOps.td
784

The op semantics start to become more complex and it is unclear with the current doc what this does.

Please make these 2 new attributes very loud and spell them as UnitAttr:$disable_multi_reduction_to_contract_patterns and UnitAttr:$disable_transfer_permutation_map_lowering_patterns.
Please document the options and mark them as "limiting the power of vectorization and are currently intended for testing purposes".

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

Please put this in n anon namespace and document the fact that this only exists so we can call vectorize via a pattern inside of VectorizeOp::applyToOne.

mlir/test/Dialect/Linalg/vectorization.mlir
878

Please don't change the testing behavior in the same commit.
At best a followup commit could do this but I do not see value in removing those atm: this is what vectorization currently produces, if we need to improve this then let's change the algorithm in a followup commit.

Please rephrase this commit message into something more meaningful..

We retire linalg's strategy vectorize patterns. Our goal is to use transform dialect instead of passes.

This revision retires the LinalgCodegenStrategy vectorization patterns and passes.
Context: https://discourse.llvm.org/t/psa-retire-linalg-filter-based-patterns/63785.

The retiring the pattern is done, however, the vectorization.mlir needs to be changed drastically. I changed some of it and put it here. If that's what we want, I'll replace them all and update the revision.

This is stale, please drop.

It depends on D133684

This is how one specifies dependent commits: https://llvm.org/docs/Phabricator.html#using-patch-summaries

guraypp updated this revision to Diff 460325.Sep 15 2022, 1:40 AM
guraypp marked 2 inline comments as done.

Addressed nicolasvasilache's comments

guraypp edited the summary of this revision. (Show Details)Sep 15 2022, 1:42 AM
guraypp edited the summary of this revision. (Show Details)
guraypp edited the summary of this revision. (Show Details)
guraypp updated this revision to Diff 460329.Sep 15 2022, 2:04 AM
guraypp marked 2 inline comments as done.
guraypp edited the summary of this revision. (Show Details)

did rebase

nicolasvasilache accepted this revision.Sep 15 2022, 2:04 AM

Thanks! Let's land this and its parent.

This revision is now accepted and ready to land.Sep 15 2022, 2:04 AM
This revision was landed with ongoing or failed builds.Sep 15 2022, 2:24 AM
This revision was automatically updated to reflect the committed changes.