Page MenuHomePhabricator

[mlir][Vector] Introduce 'vector.mask' operation and MaskableOpInterface
ClosedPublic

Authored by dcaballe on Sep 29 2022, 11:49 PM.

Details

Summary

This patch introduces the vector.mask operation and the MaskableOpInterface
as described in https://discourse.llvm.org/t/rfc-vector-masking-representation-in-mlir/64964.
The vector.mask operation is used to predicate the execution of operations
implementing the MaskableOpInterface. This interface will be implemented by maskable
operations and provides information about its masking constraints and semantics.

For now, only vector transfer and reduction ops implement the MaskableOpInterface
for illustration and testing purposes.

Diff Detail

Event Timeline

dcaballe created this revision.Sep 29 2022, 11:49 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
dcaballe requested review of this revision.Sep 29 2022, 11:49 PM
rriddle requested changes to this revision.Sep 30 2022, 12:00 AM
rriddle added inline comments.
mlir/lib/IR/AsmPrinter.cpp
81–84 ↗(On Diff #464151)

This looks very broken (e.g. doesn't actually go through the printer), I'd rather strongly avoid this.

This revision now requires changes to proceed.Sep 30 2022, 12:00 AM
rriddle added inline comments.Sep 30 2022, 12:01 AM
mlir/include/mlir/IR/OperationSupport.h
792 ↗(On Diff #464151)

I'm not really on board with this API, I'd rather not include this.

dcaballe added inline comments.Sep 30 2022, 10:57 AM
mlir/include/mlir/IR/OperationSupport.h
792 ↗(On Diff #464151)

Could you please elaborate? Isn't it aligned with the rest of the API? I'm happy to try a better approach. Any suggestions? Thanks!

mlir/lib/IR/AsmPrinter.cpp
81–84 ↗(On Diff #464151)

Thanks for the feedback, River. I was hoping to get feedback on how to implement this properly. Printing an operation without its results avoids printing redundant information for vector.mask ops and reduces its verbosity. Would you have any suggestion? Thanks!

rriddle added inline comments.Sep 30 2022, 11:07 AM
mlir/include/mlir/IR/OperationSupport.h
792 ↗(On Diff #464151)

The rest of the flags are not about selectively eliding parts of an operation (except debug info, but that's not a great example to follow), this API doesn't really fit. In general, the API additions here aren't something that we should be exposing to users.

mlir/lib/IR/AsmPrinter.cpp
81–84 ↗(On Diff #464151)

spirv::SpecConstantOperationOp is an example of something that does close to what you want for the vector op.

rriddle added inline comments.Sep 30 2022, 11:18 AM
mlir/lib/IR/AsmPrinter.cpp
81–84 ↗(On Diff #464151)

Right now, the types of parsers like you want here go through the parse/printGenericOperation hooks on OpAsmParser. If we did decide to go further and allow printing the custom operation form via those hooks, I imagine we would have parse/printOperation on there. I'd rather try to avoid that for now, given that these types of operations are generally rare (in definition I mean, not in how often they get used).

nicolasvasilache accepted this revision.Oct 5 2022, 12:22 AM

I am fine with this once the parsing / printing issues are addressed to @rriddle 's satisfaction

mlir/include/mlir/Dialect/Vector/IR/VectorOps.td
2169

can we use a type that is clearer wrt what is an operand and what is returned vector<8xi1> -> vector<8xi32>

2173

the return type seems incorrect, you prob. want vector<16xf32> -> tensor<?xf32>

mlir/lib/Dialect/Vector/IR/VectorOps.cpp
4973

Please add a comment that this will trigger verification failure.

mlir/test/Dialect/Vector/ops.mlir
845

let's have a test on tensor too plz, returning a tensor here seems necessary for proper composition

dcaballe updated this revision to Diff 465585.Oct 5 2022, 3:59 PM
dcaballe marked 9 inline comments as done.

Rebase + address comments

River's comments were addressed in https://reviews.llvm.org/D135006.
Moving forward with this patch. Thanks!

rriddle added inline comments.Oct 5 2022, 4:02 PM
mlir/include/mlir/Interfaces/MaskingInterfaces.td
21 ↗(On Diff #465585)

Why is this in Interfaces/ and not in a directory of the vector dialect? We shouldn't be putting dialect specific stuff in Interfaces/.

rriddle requested changes to this revision.Oct 5 2022, 4:03 PM
This revision now requires changes to proceed.Oct 5 2022, 4:03 PM
dcaballe added inline comments.Oct 5 2022, 4:08 PM
mlir/include/mlir/Interfaces/MaskingInterfaces.td
21 ↗(On Diff #465585)

That would create a dependency between the Vector dialect and any other dialect that may contain a maskable operation, which is mostly any dialect in MLIR (arith, math, affine, etc.).

rriddle added inline comments.Oct 5 2022, 4:10 PM
mlir/include/mlir/Interfaces/MaskingInterfaces.td
21 ↗(On Diff #465585)

No it wouldn't. You just do the same thing you do here, i.e. the interface library just shouldn't depend on VectorDialect. Directory placement has no affect on dependencies. If the interface is conceptually tied to a dialect, it should be packaged near that dialect.

dcaballe added inline comments.Oct 5 2022, 4:15 PM
mlir/include/mlir/Interfaces/MaskingInterfaces.td
21 ↗(On Diff #465585)

Ok, that makes sense. Let me give it a try. Thanks!

dcaballe updated this revision to Diff 465995.Oct 7 2022, 1:12 AM

Move masking interfaces to Dialect/Vector/Interfaces

I changed the location of the interfaces, as requested, and addressed all the feedback. I'll move forward with this if no more comments.

Thanks!
Diego

This revision was not accepted when it landed; it landed in state Needs Review.Oct 10 2022, 2:44 PM
This revision was automatically updated to reflect the committed changes.