This is an archive of the discontinued LLVM Phabricator instance.

[mlir][vector] Refactor Vector Unrolling and remove Tuple ops
ClosedPublic

Authored by ThomasRaoux on Jul 2 2021, 4:05 PM.

Details

Summary

Simplify vector unrolling pattern to be more aligned with rest of the
patterns and be closer to vector distribution.
The new implementation uses ExtractStridedSlice/InsertStridedSlice
instead of the Tuple ops. After this change the ops based on Tuple don't
have any more used so they can be removed.

This allows removing signifcant amount of dead code and will allow
extending the unrolling code going forward.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Jul 2 2021, 4:05 PM
ThomasRaoux requested review of this revision.Jul 2 2021, 4:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2021, 4:05 PM

Fix formatting

ftynse added a comment.Jul 5 2021, 1:37 AM

A couple of nits. Nice improvement otherwise.

mlir/lib/Dialect/Vector/VectorTransforms.cpp
176–198

Nit: we usually use backticks to delimit operand names, I've never seen pipes before in mlir codebase.

347

Please reserve space before pushing into the vector in a loop.

360

Nit: the second cast has no effect.

364

Ditto.

367

Please fix the linter warnings here and below.

425

Could we drop the explicit number of stack elements unless strictly necessary? The declarations on previous lines don't have it...

nicolasvasilache accepted this revision.Jul 5 2021, 3:44 AM

Very nice, it was always suspected that all the tuple stuff was unnecessary complexity and the tuple slices op etc were premature complexity.
Thanks for cleaning all this stuff up, the resulting amount of red is quite impressive..

Just added a refactoring request because spelling out lhs, rhs and acc seems quite nicer that an opaque semantics loop to me.

ShipIt!

mlir/lib/Dialect/Vector/VectorTransforms.cpp
194

There is a change in functionality here in that unrolling now only works with tile sizes that divide.
Can you add comments here and to the commit message to document this?

201–211

backticks instead of pipes plz.

214–220

pipe -> backticks

392

I find the logic here quite complex to follow, it definitely needs at least doc improvements (e.g. step 1. do x) etc and maybe a refactoring.

400

I find this loop more confusing that anything else.
Can we spell it out for lhs, rhs, acc instead ?
AFAICS, the amount of duplicated code will be very marginal for greatly improved readability.
You may want to add getLhsIndexingMap, etc helpers.

419

Same thing here: lhsMask, rhsMask, accMask and interleave with the unrolled code above.

This revision is now accepted and ready to land.Jul 5 2021, 3:44 AM
ThomasRaoux marked 6 inline comments as done.

Address review comments.

ThomasRaoux marked 5 inline comments as done.Jul 7 2021, 11:02 AM
ThomasRaoux added inline comments.
mlir/lib/Dialect/Vector/VectorTransforms.cpp
194

This code was already there (line 145 in the original version). I just moved it from the previous pattern. We never added support for tile that don't divide as far as I know.

392

Changed the logic as suggested in your other comments and added extra comments. Hopefully that makes it clearer.

400

Good point, making it explicit does make the logic simpler.

425

Correct, dropped it.

This revision was landed with ongoing or failed builds.Jul 7 2021, 11:11 AM
This revision was automatically updated to reflect the committed changes.
mlir/lib/Dialect/Vector/VectorTransforms.cpp
194

ah ok, I was confused by the fact that the insert_slices / extract_slices ops had that capability, didn't realize it was not used in unrolling.
That is a nice extra source of complexity you removed here.

400

Nice, thanks!

mlir/test/Integration/Dialect/Vector/CPU/test-contraction.mlir