This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Vector] Make minor identity permutation map optional in transfer op printing and parsing
ClosedPublic

Authored by nicolasvasilache on May 17 2020, 3:33 PM.

Details

Summary

This revision makes the use of vector transfer operatons more idiomatic by
allowing to omit and inferring the permutation_map.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2020, 3:33 PM
ftynse requested changes to this revision.May 18 2020, 5:53 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/Vector/VectorOps.td
866

You don't need a class unless it's parameterized. Try

def Vector_TransferOpUtils {
  code transferDeclaration = [{...}];
}

def Vector_TransferReadOp : ...{
  let extraClassDeclaration = Vector_TransferOpUtils.transferdeclaration # [{...}];
}

without classes or inheritance.

877

Typo: tte

mlir/lib/Dialect/Vector/VectorOps.cpp
1331

Can you just put this into the declaration of the function in .td ?

1337

Should we also check that it is a minor identity of the correct size, i.e. has the expected number of equal dimensions? Or is it impossible for valid ops?

1371

Please add tests for user-visible error messages

1462

parser.emitError(..) is convertible to ParseResult::failure() IIRC

mlir/test/Conversion/AffineToStandard/lower-affine-to-vector.mlir
3

Please drop the capture for the map if it is no longer necessary. Here and below, maybe in other files too.

This revision now requires changes to proceed.May 18 2020, 5:53 AM
nicolasvasilache marked 7 inline comments as done.

Address review.

nicolasvasilache marked an inline comment as done.May 18 2020, 8:35 AM
nicolasvasilache added inline comments.
mlir/lib/Dialect/Vector/VectorOps.cpp
1337

since it's inferred we can just construct and test.

ftynse accepted this revision.May 18 2020, 8:36 AM
This revision is now accepted and ready to land.May 18 2020, 8:36 AM
This revision was automatically updated to reflect the committed changes.

I was thinking about doing the same! Thanks for doing this, Nicolas!