Page MenuHomePhabricator

[mlir][Linalg] NFC - Refactor vector.broadcast op verification logic and make it available as a precondition in Linalg vectorization.
ClosedPublic

Authored by nicolasvasilache on Oct 11 2021, 9:29 AM.

Diff Detail

Event Timeline

nicolasvasilache requested review of this revision.Oct 11 2021, 9:29 AM

Thanks, Nicolas! It looks good. Only some tests are missing.

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

Are these formatting changes accidental? Please make sure they follow the LLVM style.

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

Can parse the plural (scalars, vectors) here properly, but maybe it's just me :)
Maybe: Broadcast scalar to vector of the same element type?

1369

Could we please add some tests for these errors to invalid.mlir? I think it's important to get them covered.

ThomasRaoux added inline comments.Oct 11 2021, 9:37 PM
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
159–161

what case is it supposed to handle besides the case where vecType.getShape() == shape?

pifon2a accepted this revision.Oct 12 2021, 12:57 AM

LGTM, once the comments are addressed.

This revision is now accepted and ready to land.Oct 12 2021, 12:57 AM
nicolasvasilache marked 4 inline comments as done.

Address review.

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
159–161

You're right that broadcasting to the same shape is a noop that should just fold.
Added a trivial folder instead which triggered another simplification.

791

yeah for some reason cider doe this for me .. sigh.
Thanks!

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

I think that is correct :) updated to your suggestion

1369

The first 2 are already there.
Upon further inspection that scary "unknown" is just a "source type not a vector" issue.
Added support + test for that.

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

actually no, this is what clang-format does for me automatically; I always try to format things.

This revision was landed with ongoing or failed builds.Oct 12 2021, 4:35 AM
This revision was automatically updated to reflect the committed changes.