This is an archive of the discontinued LLVM Phabricator instance.

[mlir] [VectorOps] Progressively lower vector.outerproduct to LLVM
ClosedPublic

Authored by aartbik on Mar 10 2020, 1:06 PM.

Details

Summary

This replaces the direct lowering of vector.outerproduct to LLVM with progressive lowering into elementary vectors ops to avoid having the similar lowering logic at several places.

NOTE1: with the new progressive rule, the lowered llvm is slightly more elaborate than with the direct lowering, but the generated assembly is just as optimized; still if we want to stay closer to the original, we should add a "broadcast on extract" to shuffle rewrite (rather than special cases all the lowering steps)

NOTE2: the original outerproduct lowering code should now be removed but some linalg test work directly on vector and contain some dead code, so this requires another CL

Diff Detail

Event Timeline

aartbik created this revision.Mar 10 2020, 1:06 PM
rriddle added inline comments.Mar 10 2020, 1:13 PM
mlir/include/mlir/Dialect/VectorOps/VectorOps.h
77

Let's remove this now.

mlir/lib/Dialect/VectorOps/VectorTransforms.cpp
901

nit: if (acc) {

aartbik edited the summary of this revision. (Show Details)Mar 10 2020, 1:14 PM

Thanks Aart. Is this going to simplify some upcoming patterns or existing ones?

aartbik marked 2 inline comments as done.Mar 10 2020, 1:27 PM

Thanks Aart. Is this going to simplify some upcoming patterns or existing ones?

This is supposed to remove the VectorOuterProductOpConversion class completely from ConvertVectorToLLVM.cpp (following the usual MLIR philosophy that optimizations should complement each other, not duplicate each other). I was going to do that removal in this CL already, but than got stuck on some (imho somewhat poorly phrased) linalg tests that (1) insert vector dialect in the test without vector lowing (2) have "dead code" in their tests so that I cannot immediately pull in the vector lowering, since that also does DCE as an intentional side effect.

So the removal + linalg tests will be a follow up CL, so to keep things separate.

mlir/include/mlir/Dialect/VectorOps/VectorOps.h
77

Ok there is only one other place (outside this repo) that I know uses this, so I will take care of that. Removed here now.

aartbik updated this revision to Diff 249488.Mar 10 2020, 1:36 PM

addressed comments

Nicolas, Andy? Any feedback?

andydavis1 accepted this revision.Mar 12 2020, 10:04 AM

Aart. This looks good to me, but please wait for Nicolas' feedback as well. I think the two of you have chatted more about the outer-product stuff...

This revision is now accepted and ready to land.Mar 12 2020, 10:04 AM

LGTM except the "Changes" name.
I'll defer to you to use either of my 2 proposals or find something better, in light of we will have multiple such "Changes" cases in the future.

mlir/include/mlir/Dialect/VectorOps/VectorOps.h
74

I don't think VectorChangesLoweringPatterns is appropriate, this is really all the patterns that come together to lower VectorContract.
I would either keep the original name of call it polulateVectorContractAndDependentLoweringPatterns.
In the future I expect we'll have more "key" vector ops that will be lowered through other patterns and "Change" wouldn't work either.

aartbik updated this revision to Diff 250037.Mar 12 2020, 1:24 PM
aartbik marked 2 inline comments as done.

addressed comments

aartbik added inline comments.Mar 12 2020, 1:27 PM
mlir/include/mlir/Dialect/VectorOps/VectorOps.h
74

Fair enough. I will just keep the name and flag as it was. A slight misnomer, but it will make merging and updating subsequent CLs a lot easier!

Thanks!

This revision was automatically updated to reflect the committed changes.