This is an archive of the discontinued LLVM Phabricator instance.

[mlir] [vector] Add an optional filter to vector contract lowering patterns.
ClosedPublic

Authored by poechsel on Jul 16 2020, 9:06 AM.

Details

Summary

Vector contract patterns were only parameterized by a vectorTransformsOptions. As a result, even if an mlir file was containing several occurrences of vector.contract, all of them would be lowered in the same way. More granularity might be required . This Diff adds a constraint argument to each of these patterns which allows the user to specify with more precision on which vector.contract should each of the lowering apply.

Diff Detail

Event Timeline

poechsel created this revision.Jul 16 2020, 9:06 AM
poechsel retitled this revision from Add an optional filter to vector contract lowering patterns. to [mlir] [vector] Add an optional filter to vector contract lowering patterns..Jul 16 2020, 9:07 AM
mlir/include/mlir/Dialect/Vector/VectorTransforms.h
180

default should always be just success() plz to avoid footgun scenarios.

mlir/lib/Dialect/Vector/VectorTransforms.cpp
1646

Please don't hoist these out: they are pretty essential for the patterns to compose and we don't want to rely on the user to have to set the conjunction of these things properly: it is too easy to shoot oneself in the foot.

nicolasvasilache requested changes to this revision.Jul 16 2020, 10:03 AM
This revision now requires changes to proceed.Jul 16 2020, 10:03 AM
poechsel marked an inline comment as done.Jul 17 2020, 1:12 AM
poechsel added inline comments.
mlir/lib/Dialect/Vector/VectorTransforms.cpp
1646

I don't necessarily agree. The goal of the future is to override this hardcoded check. You might have two vector.contract in the same file and one might be lowered to the FMA form, the other to outerproduct. The problem with the current approach is that vectorTransformsOptions is invariant for all vector.contract. The filter added in this diff is here for more control. In any case the previous behavior is still accessible using the static defaultFilter member of each pattern.

However, I also agree that the way it's done now is error prone especially since it presents a change in behavior.
I propose the following solution:

  • The constructor for these lowering takes a new option, overrideDefaultCheck, that will default to false.
  • The filter looses its vectortransformsOptions argument and defaults to success()
  • the filter test is done before the check on vectorContractLowering, but the latter will not be executed if overrideDefaultCheck is true.
poechsel updated this revision to Diff 278784.Jul 17 2020, 8:29 AM

Simplified the interface -- filter should not override the default behavior.

poechsel marked 2 inline comments as done.Jul 17 2020, 8:29 AM
nicolasvasilache accepted this revision.Jul 17 2020, 8:33 AM

Looks good thanks!

I agree this part looks redundant from an API perspective.

VectorContractLowering lowering = VectorContractLowering::OuterProduct;
VectorTransformsOptions options{lowering};
patterns.insert<ContractionOpToOuterProductOpLowering>(

In the future we'll probably end up with a single entry-point higher-level directive like Vectorize<OpType>(lowering, ctx, filter);

This revision is now accepted and ready to land.Jul 17 2020, 8:33 AM
This revision was automatically updated to reflect the committed changes.

LGTM

Thanks, this will make finer grained experimenting a little easier. But, needless to say, ultimately all this "flag stuff" should be replaced by a proper cost model.