Page MenuHomePhabricator

[mlir][vector] Add missing support for contract of integer lowering.
ClosedPublic

Authored by ThomasRaoux on Feb 10 2021, 4:01 PM.

Details

Summary

Some of the lowering of vector.contract didn't support integer case. Since reduction of integer cannot accumulate we always break up the reduction op, it should be merged by a separate canonicalization if possible.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Feb 10 2021, 4:01 PM
ThomasRaoux requested review of this revision.Feb 10 2021, 4:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2021, 4:01 PM
aartbik added inline comments.Feb 12 2021, 3:23 PM
mlir/lib/Dialect/Vector/VectorTransforms.cpp
2032

Minor, but ignorable suggestion: since we repeat this, how about making a helper somewhere that is called createAdd and createMul and pass in the isInt?

mlir/test/Dialect/Vector/vector-contract-transforms.mlir
21–23

As you state in the description, we always have this breakup now, which is a bit unfortunate (the merged version tends to generate better code). Do you feel special cases codegen is too much of a maintenance burden? Can we at least get a commitment to get the follow up canonicalization in place

nicolasvasilache accepted this revision.Feb 13 2021, 1:00 AM
nicolasvasilache added inline comments.
mlir/test/Dialect/Vector/vector-contract-transforms.mlir
21–23

For cases that reduce to FMA, I have one that I need to flush into core somewhere.
For cases like integer etc it'll prob be HW_specific.

This revision is now accepted and ready to land.Feb 13 2021, 1:00 AM
bondhugula added inline comments.
mlir/lib/Dialect/Vector/VectorTransforms.cpp
2014

Nit: You can move this to further below - to just outside the for loop.

Address feedback.

ThomasRaoux added inline comments.Feb 15 2021, 8:49 PM
mlir/lib/Dialect/Vector/VectorTransforms.cpp
2014

Done.

2032

I was also thinking about it. I followed your suggestion.

mlir/test/Dialect/Vector/vector-contract-transforms.mlir
21–23

@nicolasvasilache pushed this canonicalization so this shouldn't be a concern!