Page MenuHomePhabricator

[mlir] Split std.splat into tensor.splat and vector.splat
ClosedPublic

Authored by rriddle on Jan 25 2022, 3:52 PM.

Details

Summary

This is part of the larger effort to split the standard dialect. This will also allow for pruning some
additional dependencies on Standard (done in a followup).

Diff Detail

Event Timeline

rriddle created this revision.Jan 25 2022, 3:52 PM
rriddle requested review of this revision.Jan 25 2022, 3:52 PM

Was this part of an RFC already?

Was this part of an RFC already?

Can't remember, let me see. (Haha if so, just getting rid of things as I see them in the codebase)

Was this part of an RFC already?

Can't remember, let me see. (Haha if so, just getting rid of things as I see them in the codebase)

Couldn't find one when searching on discourse, it was left as a TODO when the tensor dialect was originally split. I'd like to just split it now given that the standard dialect has ~12 ops left.

bondhugula requested changes to this revision.Jan 25 2022, 4:25 PM
bondhugula added a subscriber: bondhugula.
bondhugula added inline comments.
mlir/lib/Dialect/Vector/VectorOps.cpp
4278–4291 ↗(On Diff #403060)

This is just duplicating the entire folding hook. Move this method to lib/IR/BuiltinTypeInterfaces.cpp and reuse from both places?

This revision now requires changes to proceed.Jan 25 2022, 4:25 PM
rriddle requested review of this revision.Jan 25 2022, 4:33 PM
rriddle updated this revision to Diff 403077.
rriddle edited the summary of this revision. (Show Details)
rriddle marked an inline comment as done.
rriddle added inline comments.Jan 25 2022, 4:33 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
4278–4291 ↗(On Diff #403060)

Doesn't feel like enough code to warrant sharing (mostly just unnecessary asserts that are already invariant to this method), cleaned up the code a bit.

mehdi_amini added inline comments.Jan 25 2022, 4:40 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
2479 ↗(On Diff #403077)

We lost the get prefix? Aren't all dialects generating both forms by now?

rriddle added inline comments.Jan 25 2022, 4:41 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
2479 ↗(On Diff #403077)

Yeah, I was also a bit surprised. Not sure what the status of the prefix flipping is, but I can look into flipping it myself.

The vector dialect has vector.broadcast op that is strictly a superset of splat. Personally I found it annoying to have to support both so I would prefer not having vector.splat at all.
@nicolasvasilache do you have any opinion? Any reason to have both?

bondhugula added inline comments.Jan 25 2022, 4:46 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
4278–4291 ↗(On Diff #403060)

Isn't the splat on a vector type functionally identical to a splat on a tensor type? Is there a need for two different splat op versions on shaped pure value types? (memref type vs tensor type isn't the same as vector type vs tensor type) The folding hook can get longer and is already missing many trivial optimizations here, which could in the future use both vector dialect ops and tensor dialect ops.

I feel a split should be motivated by the need for two different op versions as opposed to simply trimming the standard dialect. (For eg. the folding hooks for tensor.dim and memref.dim have diverged and specialized and that's suitable there, but I'm not sure that's the case here.)

rriddle added inline comments.Jan 25 2022, 4:50 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
4278–4291 ↗(On Diff #403060)

From a layering perspective right now it makes no sense to keep them unified. The functionality built on top of both is also so small right now that it brings more burden than benefit for keeping them unified. I'd rather separate and layer them properly in their own respective dialects (and help achieve a valuable goal of killing the standard dialect), and then determine what can be unified as things are actually built out.

As mentioned below as well, there is also a sort of awkward duplication with std.splat and vector.broadcast.

The vector dialect has vector.broadcast op that is strictly a superset of splat. Personally I found it annoying to have to support both so I would prefer not having vector.splat at all.
@nicolasvasilache do you have any opinion? Any reason to have both?

Yeah, it's kind of awkward. I've noticed that vector.broadcast lowers to splat for some of its cases, for what I would assume should be for behavior that vector.broadcast already supports. It'd be nice to clean that up.

mehdi_amini added inline comments.Jan 25 2022, 5:06 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
4278–4291 ↗(On Diff #403060)

We can take it the other way around: if the standard dialect didn't exist and someone needed this operation today, how would we add it?

The vector dialect has vector.broadcast op that is strictly a superset of splat. Personally I found it annoying to have to support both so I would prefer not having vector.splat at all.

A splat is on an "elemental" type while a broadcast is on an "elemental" or a "tensor" type and thus a superset as you say. It is trivial to convert a vector.splat to a vector.broadcast if you don't want to support both for the purposes of a transformation. Ultimately, it's a splat that most commonly corresponds to hardware intrinsics since those are defined on repeating elements -- while a broadcast is a higher-order abstraction. You can always canonicalize a vector.broadcast to a vector.splat if the operand is a scalar. I am a strong -1 on removing splat on vectors -- in fact, I feel vector.broadcast could drop scalar operand support and instead be exclusively for n-d vector operands.

jpienaar added inline comments.
mlir/lib/Dialect/Vector/VectorOps.cpp
2479 ↗(On Diff #403077)

I thought I flipped vector too ... Mmm. I may have forgotten to send it out. There is the procedure for flipping and the clang-tooling based rewrite if needed (even if I abuse clang-tidy a bit for that)

bondhugula added inline comments.Jan 25 2022, 5:28 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
4278–4291 ↗(On Diff #403060)

As mentioned below as well, there is also a sort of awkward duplication with std.splat and vector.broadcast.

That overlap is unrelated to this issue and mostly from an oversight when vector.broadcast was added: in fact, it's awkward that vector.broadcast right now broadcasts either a scalar or an n-d vector -- two very different types. Ideally, vector.broadcast should only support *vector typed* operands while splat supports scalar operands. The fact that the latter scenario is often the common case with direct hardware support (or close to that) warrants retaining such an operation -- this is aligned with the purposes of vector types and vector dialect as opposed to tensors. (Not that it's important now, in fact, when I introduced the splat op, the intent was to mainly use it only for vector types.)

The vector dialect has vector.broadcast op that is strictly a superset of splat. Personally I found it annoying to have to support both so I would prefer not having vector.splat at all.
@nicolasvasilache do you have any opinion? Any reason to have both?

Yes, dropping the antiquated splat, I don't see value in keeping it.

nicolasvasilache requested changes to this revision.Jan 25 2022, 11:52 PM
This revision now requires changes to proceed.Jan 25 2022, 11:52 PM
The vector dialect has vector.broadcast op that is strictly a superset of splat. Personally I found it annoying to have to support both so I would prefer not having vector.splat at all.
@nicolasvasilache do you have any opinion? Any reason to have both?

Yes, dropping the antiquated splat, I don't see value in keeping it.

I'm happy to kill it, but it begs the question of which comes first. I have a slight preference for pushing this commit first for a couple of reasons.
Partially because it is a necessary refactoring and has some dependencies stacked on top of it. Removing splat requires untangling some interconnected things, which are a bit easier to manage when all of the code is in one dialect. Lastly, because there is some contention from Uday about having splat (that I don't want to bog down this necessary commit too much with discussing).

The vector dialect has vector.broadcast op that is strictly a superset of splat. Personally I found it annoying to have to support both so I would prefer not having vector.splat at all.
@nicolasvasilache do you have any opinion? Any reason to have both?

Yes, dropping the antiquated splat, I don't see value in keeping it.

I'm happy to kill it, but it begs the question of which comes first. I have a slight preference for pushing this commit first for a couple of reasons.
Partially because it is a necessary refactoring and has some dependencies stacked on top of it. Removing splat requires untangling some interconnected things, which are a bit easier to manage when all of the code is in one dialect. Lastly, because there is some contention from Uday about having splat (that I don't want to bog down this necessary commit too much with discussing).

As you know I am a fan of faster iteration once we know where we are going so I'm happily giving you green light on this.

This revision now requires review to proceed.Jan 26 2022, 12:46 AM
rriddle updated this revision to Diff 403360.Jan 26 2022, 11:43 AM
rriddle updated this revision to Diff 404704.Jan 31 2022, 1:29 PM

Ping. Is there something inherently blocking here? This puts the world in an objectively better state than what we are now, and is kind of blocking the effort to kill the standard dialect.

bondhugula accepted this revision.EditedJan 31 2022, 5:49 PM

LGTM for larger reasons although it's unfortunate we've created an unnecessary duplicate. There is no reason for two ops doing the ditto -- both VectorType and TensorType are both "value" shaped types and "splat" does the *same* thing on both.

Yes, dropping the antiquated splat, I don't see value in keeping it.

The argument to remove vector.splat because vector.broadcast can handle either a scalar operand type or a vector operand type makes no sense to me. In fact, the scalar operand type on vector.broadcast is the one to kill I feel. Again, it's very likely that whoever added that support on vector.broadcast overlooked splat: which is also why the latter is less used. Repasting from comment above:

in fact, it's awkward that vector.broadcast right now broadcasts either a scalar or an n-d vector -- two very different types. Ideally, vector.broadcast should only support *vector typed* operands while splat supports scalar operands. The fact that the latter scenario is often the common case with direct hardware support (or close to that) warrants retaining such an operation -- this is aligned with the purposes of vector types and vector dialect as opposed to tensors. (Not that it's important now, in fact, when I introduced the splat op, the intent was to mainly use it only for vector types.)
This revision is now accepted and ready to land.Jan 31 2022, 5:49 PM

Is it difficult to match a vector.broadcast that is a splat? Is there code that would need to be matching both splat and broadcast when it could handle just broadcast generically?
To me these are the important questions when considering the value of having two operations instead of one.
(I don't know the answer for vector.splat vs vector.broadcast)

rriddle updated this revision to Diff 405031.Feb 1 2022, 12:03 PM
This revision was automatically updated to reflect the committed changes.