Page MenuHomePhabricator

Adding min(f/s/u) and max(f/s/u) cases for vector reduction
ClosedPublic

Authored by aslepko on Jun 24 2021, 3:50 PM.

Details

Summary

This PR adds missing AtomicRMWKind::min/max cases which we would like to use for min/max reduction loop vectorizations.

Diff Detail

Event Timeline

aslepko created this revision.Jun 24 2021, 3:50 PM
aslepko requested review of this revision.Jun 24 2021, 3:50 PM

Thanks for the generalization. Don't you want to put some new tests in test/Dialect/Affine/SuperVectorize just to make sure the new code is covered?

Hi @aartbik, thank you for your reply. And agree, it would be nice to add testing. I was not sure how to do that for min/max reductions without proper changes in the reduction recognizer (which we do in our local project). But I'd really appreciate any suggestions for what I can do here.
Thanks!

proper changes in the reduction recognizer (which we do in our local project).

Do you plan to follow up with these? If so, please make this a parent revision of such a follow up.
I am just trying to avoid checkin in uncovered code....

Hi @aartbik, my apologies this got dragged out. I was out for a while.
After internal discussion we decided to also add the code to recognize min/max reductions. With that we can add all test cases to cover the initial additions in this current commit.
Since it has been a while, should I do an entirely new commit, or just upload a second patch here?
Thanks.

Since it has been a while, should I do an entirely new commit, or just upload a second patch here?

If you rebase with main, let's try to continue on this patch, so we keep full history.
Thanks for adding!

aslepko updated this revision to Diff 369596.Mon, Aug 30, 6:32 PM

This update adds code to recognize min/max reductions. This was necessary to test part some of the functionality we had added initially.
Also, this update contains all the tests missing in the original commit.

Now that I know some testing part through recognition will follow, I am okay breaking up this revision as originally planned, i.e. adding the ops in a first small revision and then adding the recognition later.
That way you get the first revision in a bit quicker, since I think the recognition part may need some discussion.

mlir/lib/Analysis/AffineAnalysis.cpp
105 ↗(On Diff #369596)

please align parameters (clang-format will fix all those issues for you)

129 ↗(On Diff #369596)

L108 to here could use some comments, in particular some visual representation of what you are looking for

154 ↗(On Diff #369596)

IEEE floating-point comparisons as very tricky and we need to make sure we preserve the semantics when rewriting them into min/max . Have you made sure these rewriting preserve behaviors for e.g. NaN operands?

@aartbik, that sounds reasonable. Should I then basically go ahead and remove the tests+recognizer for a new/third patch and upload that patch. Then, make a new PR once this is merged for the recognition part? Thanks.

@aartbik, that sounds reasonable. Should I then basically go ahead and remove the tests+recognizer for a new/third patch and upload that patch. Then, make a new PR once this is merged for the recognition part? Thanks.

Yes please. That way we can get your first patch in quickly, and discuss some details on the second.

aslepko updated this revision to Diff 371126.Tue, Sep 7, 10:55 AM

As discussed, this update removes the min/max recognition and testing part from the prior update (which will be added through a separate, new PR).

aartbik accepted this revision.Tue, Sep 7, 11:31 AM
This revision is now accepted and ready to land.Tue, Sep 7, 11:31 AM

make sure you have green bot builds prior to submitting but lgtm

This revision was landed with ongoing or failed builds.Thu, Sep 9, 12:25 PM
This revision was automatically updated to reflect the committed changes.