This is an archive of the discontinued LLVM Phabricator instance.

[mlir][linalg] Fix generic reduction vectorization
ClosedPublic

Authored by ThomasRaoux on Oct 12 2021, 11:30 AM.

Details

Summary

We shouldn't broadcast the original value when doing reduction. Instead
we compute the reduction and then combine it with the original value.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Oct 12 2021, 11:30 AM
ThomasRaoux requested review of this revision.Oct 12 2021, 11:30 AM
nicolasvasilache accepted this revision.Oct 12 2021, 1:28 PM

LGTM so putting a stamp, but please be sure to really refactor the code I point out, with good names to be super easy to come back to in the future.

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
264

the new code above and below this line feel quite dry.
Can you refactor in helper functions with proper names so it becomes clearer what is happening?
I have to squint a lot to get it and I know I'll have more trouble in the future.

This revision is now accepted and ready to land.Oct 12 2021, 1:28 PM

Review feedback

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
264

Done.

This revision was landed with ongoing or failed builds.Oct 12 2021, 3:47 PM
This revision was automatically updated to reflect the committed changes.
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
189

typo: two

208

This is highly non-trivial, please add a comment with a detailed example with all the steps involved.

217

This is adding a new read + broadcast that didn't exist before, right?
OTOH, you have already read this value into some other broadcasted form.
It seems like you should get that initial broadcasted value and extract from it?

247

typo: match

mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
268

This should be return vectorizeBinaryReductionOp(reduceOp) where this code gets hoisted.
In the future this would become a VectorOpInterface for ops that are vectorizable and we would just check / use that interface to build.

ThomasRaoux added inline comments.Oct 13 2021, 7:25 AM
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
217

Correct, the problem is that the value we get at this stage is already loaded + broadcast and added (for a reduction add) to the input so there is nothing I can't extra bak the original value.
Maybe a better solution is to emit the reduction beforehand when we generate the vector operations?

ThomasRaoux added inline comments.Oct 13 2021, 8:18 AM
mlir/lib/Dialect/Linalg/Transforms/Vectorization.cpp
268

I'm not sure I understand. Do you mean the whole creation of multiDimReduce + the extra op to combine with the initial value should go into vectorizeBinaryReductionOp or just the code creating the last instruction?