Page MenuHomePhabricator

[mlir] add support for index type in vectors.
ClosedPublic

Authored by gysit on Apr 6 2021, 5:20 AM.

Details

Summary

The patch enables the use of index type in vectors. It is a prerequisite to support vectorization for indexed Linalg operations. This refactoring became possible due to the newly introduced data layout infrastructure. The data layout of a module defines the bitwidth of the index type needed to verify bitcasts and similar vector operations.

Diff Detail

Event Timeline

gysit created this revision.Apr 6 2021, 5:20 AM
gysit requested review of this revision.Apr 6 2021, 5:20 AM
gysit added inline comments.Apr 6 2021, 5:23 AM
mlir/lib/Dialect/Vector/VectorOps.cpp
2205

@ftynse is this the intended way to access the dataLayout?

nicolasvasilache accepted this revision.Apr 6 2021, 6:38 AM

Can we add one integration test to see that everything works end to end?
e.g. in the same dir as this test: https://github.com/llvm/llvm-project/blob/main/mlir/test/Integration/Dialect/Vector/CPU/test-extract-strided-slice.mlir
LGTM otherwise after @ftynse comments on the data layout usage.

This revision is now accepted and ready to land.Apr 6 2021, 6:38 AM

Very pleased to see this happen! This will also remove some artificial bail out in sparse compiler vectorization due to "index" as base type!

ftynse added inline comments.Apr 6 2021, 2:21 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
2205

I would suggest adding a helper to find the closest parent that can have a data layout (either a ModuleOp or an op implementing DataLayoutOpInterface, whatever comes first). This can live on the DataLayout itself as a static method, static DataLayout DataLayout::closest(Operation *). I didn't have a need for this yet so I haven't added untested code.

In a slightly longer term, we would want to have a DataLayoutAnalysis that caches the results, and have verifier do this analysis only once.

mlir/lib/IR/BuiltinAttributes.cpp
685

Please update the .md docs accordingly. We still have a rationale that explains why we can't have vector<index> and a spec that contradicts this.

gysit updated this revision to Diff 335806.Apr 7 2021, 7:39 AM

Addressing the following comments:

  • added a simple end-to-end integration test
  • updated the rational to reflect the index type support
  • introduced a method to get the closest module or data layout op
gysit marked 2 inline comments as done.Apr 7 2021, 7:42 AM
gysit added inline comments.
mlir/lib/Dialect/Vector/VectorOps.cpp
2205

There would be the option to use the closest method in the test-data-layout-query pass. Yet the code there also implements caching so I decided to not touch it.

nicolasvasilache accepted this revision.Apr 8 2021, 12:48 AM
gysit updated this revision to Diff 336026.Apr 8 2021, 1:09 AM
gysit marked an inline comment as done.
  • updated to reflect ongoing changes
This revision was landed with ongoing or failed builds.Apr 8 2021, 1:32 AM
This revision was automatically updated to reflect the committed changes.