Page MenuHomePhabricator

[mlir] [VectorOps] Merge VectorReduction/VectorReductionV2 into one Op
ClosedPublic

Authored by aartbik on Mar 5 2020, 10:17 AM.

Details

Summary

Paying off some technical debt in VectorOps, where I introduced a special
op for a fused accumulator into reduction to avoid some issues around
printing and parsing an optional accumulator. This CL merges the two
into one op again and does things the right way (still would be nice
to have "assemblyFormat" for optional operands though....).

Diff Detail

Event Timeline

aartbik created this revision.Mar 5 2020, 10:17 AM
aartbik edited the summary of this revision. (Show Details)
nicolasvasilache accepted this revision.Mar 5 2020, 11:19 AM

Great, thanks Aart!
I'll use this to ping again on the broadcast progressive lowering, was this something you wanted to offload as an intro task IIRC ?

mlir/lib/Dialect/VectorOps/VectorOps.cpp
98

Re assembly format, file a bug for @rriddle linking to this diff?
Another way, if variadic is more tricky to impl automatically (because of say combinatorial effects with variadic operands), could be to allow multiple declarative forms and a simple convention on optional arg ordering?

This revision is now accepted and ready to land.Mar 5 2020, 11:19 AM
aartbik updated this revision to Diff 248557.Mar 5 2020, 11:20 AM

fixed lint

rriddle added inline comments.Mar 5 2020, 11:35 AM
mlir/lib/Dialect/VectorOps/VectorOps.cpp
98
aartbik marked an inline comment as done.Mar 5 2020, 11:40 AM
aartbik added inline comments.
mlir/lib/Dialect/VectorOps/VectorOps.cpp
98

Ah, very interesting! The customer parsing/printing here has a minor advantage of printing less type information (since most of it is inferred), but that is good to keep in mind. Thanks.

rriddle added inline comments.Mar 5 2020, 11:43 AM
mlir/lib/Dialect/VectorOps/VectorOps.cpp
98

I think the only problem you will run into is with inferring the type of the optional operand.

rriddle added inline comments.Mar 5 2020, 11:46 AM
mlir/lib/Dialect/VectorOps/VectorOps.cpp
98

The assembly format can already infer types in a majority of cases. I don't think this explicit case is handled right now, though it should be easy to support. (i.e. inferring the type of an optional operand)

https://mlir.llvm.org/docs/OpDefinitions/#type-inferrence

This revision was automatically updated to reflect the committed changes.