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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
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.
|
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);
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.
default should always be just success() plz to avoid footgun scenarios.