This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Linalg] Generalize Vectorization of Linalg contractions
ClosedPublic

Authored by nicolasvasilache on Jul 7 2020, 7:43 AM.

Details

Summary

This revision adds support for vectorizing named and generic contraction ops to vector.contract. Cases in which the memref is 0-D are special cased to emit std.load/std.store instead of vector.transfer. Relevant tests are added.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald Transcript
aartbik added inline comments.Jul 8 2020, 7:36 PM
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
91–92

can't we just

return success(isContraction(op))

at this point?

aartbik added inline comments.Jul 8 2020, 10:52 PM
mlir/test/Dialect/Linalg/transform-patterns-matmul-to-vector.mlir
3

Having several RUN command (and possible prefixes) makes sense if you want to run the *same* test and check it with different lowering strategies. However, here you seem to simply append new tests.

Wouldn't it make more sense to just start a new test file for this, to avoid running commands without any CHECKs. Or is the duplicate run intentional (if so, probably document that)?

ftynse accepted this revision.Jul 10 2020, 6:04 AM
ftynse added inline comments.
mlir/include/mlir/IR/Attributes.h
289

UnderlyingTy looks unused.

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
74

return success(/*condition here*/); ?

This revision is now accepted and ready to land.Jul 10 2020, 6:04 AM
nicolasvasilache marked 5 inline comments as done.Jul 10 2020, 7:20 AM
nicolasvasilache added inline comments.
mlir/test/Dialect/Linalg/transform-patterns-matmul-to-vector.mlir
3

I don't think we have particular best practices here.
I personally find it useful to "have more tests" and see that things run properly in even outside of the groomed positive tests.
But I can also see how this could be viewed as overhead and I am open to splitting this.
If we do the split, we should do it more systematically.
One could argue that the LLVM model should be the way (i.e. never have any test with split-input-files) but I would not be enthused by such a change.

nicolasvasilache marked an inline comment as done.

Format

This revision was automatically updated to reflect the committed changes.