Page MenuHomePhabricator

[mlir][Vector] Pass for lowering of transfer_read/write
AbandonedPublic

Authored by sgrechanik on Mar 12 2021, 10:31 AM.

Details

Summary

This patch adds a pass that lowers transfer_read/write ops
to other vector ops using the recently added patterns.

Since this is the first non-testing pass for the vector dialect,
the patch also adds pass-related infrastructure files to the
vector dialect.

Diff Detail

Event Timeline

sgrechanik created this revision.Mar 12 2021, 10:31 AM
sgrechanik requested review of this revision.Mar 12 2021, 10:31 AM

Thanks for working on this, Sergei! Some comments.

mlir/include/mlir/Dialect/Vector/Passes.h
2

typo?

mlir/include/mlir/Dialect/Vector/Passes.td
16

Since documentation is automatically generated out of this and posted here: https://mlir.llvm.org/docs/Passes/
Could we elaborate a bit more about what this is doing and may add a couple of examples? I think that would be very useful for any external user trying to use this pass.

mlir/lib/Dialect/Vector/PassDetail.h
6 ↗(On Diff #330836)

Looking at similar files, shouldn't this file be under Dialect/Vector/Transforms?

mlir/lib/Dialect/Vector/Transforms/VectorTransforms.cpp
3333

Would it make more sense to move this pass to its own independent file? I don't think we should have multiple passes in the same file. This file seems a bit overloaded already.

sgrechanik marked 2 inline comments as done.
  • Moved the pass to a separate file.
  • Restructured the mlir/lib/Dialect/Vector directory.
  • Split the MLIRVector library into MLIRVector and MLIRVectorTransforms

Fixed formatting

mlir/include/mlir/Dialect/Vector/Passes.h
2

More like a tradition. I'll change it to match the corresponding line in e.g. Dialect/Linalg/Passes.h, which is more sane.

mlir/lib/Dialect/Vector/PassDetail.h
6 ↗(On Diff #330836)

Yes, other dialects use a more complex directory structure. I moved some files to match it and also separated the MLIRVectorTransforms library from MLIRVector, although I'm not sure about this since it may break things.

dcaballe accepted this revision.Mar 17 2021, 9:26 AM

Thanks, Sergei! LGTM. @nicolasvasilache, @aartbik, could we please have final approval from anybody from your side? Thanks!

This revision is now accepted and ready to land.Mar 17 2021, 9:26 AM
  • Fixed the clang-tidy warning

@nicolasvasilache @aartbik Kindly ping. This patch changes the directory structure quite a bit, so I'm not comfortable proceeding without a second opinion.

nicolasvasilache resigned from this revision.Jul 28 2021, 2:20 AM
nicolasvasilache added a subscriber: springerm.

FYI @springerm made nice improvements to the vector infra that supersede this

sgrechanik abandoned this revision.Aug 3 2021, 12:39 PM