This is an archive of the discontinued LLVM Phabricator instance.

[mlir][Vector] Add a folder for vector.broadcast
ClosedPublic

Authored by hanchung on Sep 15 2020, 8:31 AM.

Details

Summary

Fold the operation if the source is a scalar constant or splat constant.

Update transform-patterns-matmul-to-vector.mlir because the broadcast ops are folded in the conversion.

Diff Detail

Event Timeline

hanchung created this revision.Sep 15 2020, 8:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2020, 8:31 AM
hanchung requested review of this revision.Sep 15 2020, 8:31 AM
hanchung edited the summary of this revision. (Show Details)Sep 15 2020, 9:25 AM
aartbik added inline comments.Sep 15 2020, 1:10 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
933

is this test really required, what vector types would fail?

934

style: single statement if has no { }

939

prevOp sounds like it is a predecessor, but it is the defining op

940

these cases work for the simple tests you made, but I am not convinced this works for all the duplicate/stretch semantics? did you give that some thought?

aartbik added inline comments.Sep 15 2020, 1:24 PM
mlir/lib/Dialect/Vector/VectorOps.cpp
933

I realized this comment was a bit obscure, I meant to say that for vector types you need proper duplicate/stretch semantics and you avoid that right now for this case, but not the other. Perhaps your intention was to only deal with scalar sources for both cases?

hanchung updated this revision to Diff 292137.Sep 16 2020, 1:30 AM

Remove the propagatin of vector.broadcast

hanchung updated this revision to Diff 292141.Sep 16 2020, 1:46 AM
hanchung marked 4 inline comments as done.

Handle splat constants.

hanchung marked an inline comment as done.Sep 16 2020, 1:47 AM
hanchung added inline comments.
mlir/lib/Dialect/Vector/VectorOps.cpp
933

My intention is to deal with broadcasting constant scalar, so I check if this is a scalar in the first condition.

Now I fixed it to accept both scalar and splat element attrs (like vectors).

940

Actually I didn't think about this part much. I just came out of this idea when adding the folder on constant scalars. I feel that we could fold it according to the op definition -- the source operand is duplicated to match the given rank and sizes in the result vector. If we can broadcast in one step why two steps.

However, I think you are might right, there could be more complicated cases, eg, each step could have different meanings. Let's hold this off and revisit it when there are more examples?

hanchung edited the summary of this revision. (Show Details)Sep 16 2020, 1:48 AM
rriddle added inline comments.Sep 16 2020, 2:06 AM
mlir/lib/Dialect/Vector/VectorOps.cpp
933

Please do not check for constants like this, operands contains the attribute values for constant operands for this operation or null if the operand isn't constant.

aartbik added inline comments.Sep 16 2020, 2:23 AM
mlir/lib/Dialect/Vector/VectorOps.cpp
940

Oh, you are probably right, I just needed some convincing and given the lack of tests I was not sure if you had considered all cases ;-) Either way works for me, but what you have now is a nice pure constant folder.

mlir/test/Dialect/Vector/canonicalize.mlir
393

can you please make these tests a bit more specific that the constant is indeed returned
something like

CHECK: [[A:.*]] = constant ....
CHECK-NOT: ...
CHECK: return %[[A]]

rriddle added inline comments.Sep 16 2020, 3:00 AM
mlir/lib/Dialect/Vector/VectorOps.cpp
933

For additional clarification as to why:

  • This API is used in a wide variety of circumstances, and constants may be passed in to operands when the actual operand of this operation is not a ConstantOp(or equivalent).
  • By hard coding ConstantOp you break composability with another constant operation.
hanchung updated this revision to Diff 292179.Sep 16 2020, 4:15 AM
hanchung marked an inline comment as done.

Add more checks on tests and handle constants properly.

hanchung marked 4 inline comments as done.Sep 16 2020, 4:16 AM
hanchung added inline comments.
mlir/lib/Dialect/Vector/VectorOps.cpp
933

Thanks for pointing this out.

aartbik added inline comments.Sep 16 2020, 4:37 AM
mlir/lib/Dialect/Vector/VectorOps.cpp
935

nit: can't we use getVectorType() directly here, since this is a broadcast

I would also call it vectorType rather than shapedType (or perhaps resultType if you prefer), since thta makes it a bit more clear

hanchung updated this revision to Diff 292212.Sep 16 2020, 7:27 AM
hanchung marked 2 inline comments as done.

Use getVectorType() instead

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

Oh yes, it makes more sense to me too. Thanks!

aartbik accepted this revision.Sep 16 2020, 8:39 AM

Good to go if River has no more feedback on this. Please make sure to run the integration tests as well prior to submitting.
And thanks for the optimization!

Also, please feel free to follow-up with the broadcast-broadcast canonicalization. It was not my intention to discourage that, just to make sure you thought and tested through all cases.

This revision is now accepted and ready to land.Sep 16 2020, 8:39 AM
This revision was automatically updated to reflect the committed changes.