Page MenuHomePhabricator

[InstCombine] Optimize always overflowing signed saturating add/sub
ClosedPublic

Authored by nikic on May 28 2019, 12:00 PM.

Details

Summary

Based on the overflow direction information added in D62463, we can now fold always overflowing signed saturating add/sub to signed min/max.

Diff Detail

Repository
rL LLVM

Event Timeline

nikic created this revision.May 28 2019, 12:00 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2019, 12:00 PM
lebedev.ri added inline comments.May 28 2019, 12:22 PM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2073–2074 ↗(On Diff #201738)

Can you use APSInt::getMinValue(BitWidth, !SI->isSigned()) instead?

2079–2080 ↗(On Diff #201738)

Same

nikic marked an inline comment as done.May 28 2019, 12:40 PM
nikic added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2073–2074 ↗(On Diff #201738)

It works, but I'm wondering whether it's okay to use APSInt for this purpose. It looks like APSInt is used heavily by clang, but the only uses in the LLVM middle end are for interfacing with the convertToInteger() APFloat API.

lebedev.ri added inline comments.May 28 2019, 1:12 PM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2073–2074 ↗(On Diff #201738)

If it's not ok to use it for this purpose then i, too, would like to know about that :)

nikic updated this revision to Diff 201950.May 29 2019, 9:02 AM

Use APSInt to construct signed/unsigned min/max.

craig.topper added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2074 ↗(On Diff #201950)

What's really happening on this line? APSInt::getMinValue returns an APSInt object which is larger than an APInt object. Does this create an APSInt and then copy the APInt part of that out into a separate APInt? Or does the APInt portion get moved?

lebedev.ri accepted this revision.May 29 2019, 9:15 AM

LGTM, thank you!
(There seems to be a lot of constant folds missing)

This revision is now accepted and ready to land.May 29 2019, 9:15 AM
nikic marked an inline comment as done.May 29 2019, 9:27 AM
nikic added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2074 ↗(On Diff #201950)

My C++ foo is pretty weak, so I stepped through this in gdb... It takes the APInt(APInt &&) move constructor.

lebedev.ri added inline comments.May 29 2019, 9:27 AM
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
2074 ↗(On Diff #201950)
This revision was automatically updated to reflect the committed changes.