Page MenuHomePhabricator

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

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



This patch introduces the vector.mask operation and the MaskableOpInterface
as described in
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.
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
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
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!

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
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.

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
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


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


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


Please add a comment that this will trigger verification failure.


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
Moving forward with this patch. Thanks!

rriddle added inline comments.Oct 5 2022, 4:02 PM
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
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
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
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.


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.