This is an archive of the discontinued LLVM Phabricator instance.

[SCCP] Use range info to prove AddInst has NUW flag.
ClosedPublic

Authored by fhahn on Jan 23 2023, 10:47 AM.

Details

Summary

This patch updates SCCP to use the value ranges of AddInst operands to
try to prove the AddInst does not overflow in the unsigned sense and
adds the NUW flag. The reasoning is done with
makeGuaranteedNoWrapRegion (thanks @nikic for point it out!).

Follow-ups will include adding NSW and extension to more
OverflowingBinaryOperators.

Diff Detail

Event Timeline

fhahn created this revision.Jan 23 2023, 10:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 10:47 AM
fhahn requested review of this revision.Jan 23 2023, 10:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 10:47 AM
nikic added inline comments.Jan 24 2023, 3:48 AM
llvm/lib/Transforms/Utils/SCCPSolver.cpp
180

Why is this needed?

190

There is an SCCPInstVisitor::getConstantRange() helper -- probably that should be moved to SCCPSolver and reused here?

196

Should be RangeB. Doesn't make a difference for add, but would be wrong when extending to sub.

fhahn updated this revision to Diff 491749.Jan 24 2023, 6:17 AM

Address comments, thanks!

fhahn marked 3 inline comments as done.Jan 24 2023, 6:19 AM
fhahn added inline comments.
llvm/lib/Transforms/Utils/SCCPSolver.cpp
180

It's not needed in the latest version, originally this was to exclude vector constants. Removed, thanks!

190

probably that should be moved to SCCPSolver and reused here?

I just made it a standalone static function

196

Flipped Ranges, thanks!

nikic accepted this revision.Jan 24 2023, 9:05 AM

LGTM apart from the one note.

llvm/lib/Transforms/Utils/SCCPSolver.cpp
191

This should be guarded under if (!Inst.hasNoUnsignedWrap()). Otherwise we will report an unnecessary change.

This revision is now accepted and ready to land.Jan 24 2023, 9:05 AM
This revision was landed with ongoing or failed builds.Jan 24 2023, 12:53 PM
This revision was automatically updated to reflect the committed changes.
fhahn marked 3 inline comments as done.
fhahn marked an inline comment as done.Jan 24 2023, 12:55 PM
fhahn added inline comments.
llvm/lib/Transforms/Utils/SCCPSolver.cpp
191

Thanks, changed to if (!Inst.hasNoUnsignedWrap() &&NUWRange.contains(RangeA)). Could also have a separate check guarding makeGuaranteedNoWrapRegion, which would increase nesting though and hopefully existing compilers should be smart enough to delay the computation :)

nikic added inline comments.Jan 24 2023, 1:17 PM
llvm/lib/Transforms/Utils/SCCPSolver.cpp
191

Existing compilers almost certainly can't delay that computation (it's not LLVM_READONLY, and I'm not sure clang would sink the call even then).

dyung added a subscriber: dyung.Jan 24 2023, 9:24 PM

@fhahn this change was causing many of our internal tests to hit an assertion failure which I bisected back to your change. I've filed issue #60280 with a repro. Can you take a look?

I've reverted the change so that it doesn't go into the release branch if you don't have a chance to fix it first.

@fhahn this change was causing many of our internal tests to hit an assertion failure which I bisected back to your change. I've filed issue #60280 with a repro. Can you take a look?

I've reverted the change so that it doesn't go into the release branch if you don't have a chance to fix it first.

Thanks for the revert! I also ran into this, and bisected the issue back to this commit.

fhahn marked 2 inline comments as done.Jan 25 2023, 10:10 AM

Thanks for the reports and revert! Should be fixed in the recommitted version.

llvm/lib/Transforms/Utils/SCCPSolver.cpp
191

Fair point, I was assuming it at least to get inlined, but then realized that it is not even defined in the header....

Updated in the recommitted version.