Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider using new LLVM intrinsic for i32::saturating_add etc. #55286

Closed
hanna-kruppe opened this issue Oct 23, 2018 · 9 comments
Closed

Consider using new LLVM intrinsic for i32::saturating_add etc. #55286

hanna-kruppe opened this issue Oct 23, 2018 · 9 comments
Assignees
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@hanna-kruppe
Copy link
Contributor

As featured in LLVM Weekly, https://reviews.llvm.org/rL344629 introduces @llvm.sadd.sat for saturating signed integer addition. Not sure whether it's as transparent to the optimizer yet as open-coding it based on checked_add (as we currently do), but we might want to try it when we update to an LLVM version that has that intrinsic available.

@varkor varkor added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Oct 23, 2018
@nikic
Copy link
Contributor

nikic commented Nov 8, 2018

Right now add.sat is completely opaque. I've submitted https://reviews.llvm.org/D54237 for ConstantFolding and InstCombine support, which should be enough to properly handle #52203.

@nikic
Copy link
Contributor

nikic commented Nov 28, 2018

All the optimizations have landed, so we can give this a try after the next LLVM update.

@scottmcm
Copy link
Member

Looks like a sufficiently-new LLVM has landed in nightly, and the folding logic is working: https://play.rust-lang.org/?version=nightly&mode=release&edition=2018&gist=75e0b8364fc0d815edc1d091840fca47

@nikic
Copy link
Contributor

nikic commented Jan 28, 2019

Work on saturating add/sub intrinsics is still ongoing (turns out that getting new intrinsics to optimize and codegen well is a pretty big rabbit hole), but I believe at this point they're good enough that we should give them a try in Rust. I'll submit a PR later this week, if no one beats me to it.

@jonas-schievink jonas-schievink added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 28, 2019
@nikic nikic self-assigned this Jan 29, 2019
bors added a commit that referenced this issue Jan 31, 2019
Use LLVM intrinsics for saturating add/sub

Use the `[su](add|sub).sat` LLVM intrinsics, if we're compiling against LLVM 8, as they should optimize and codegen better than IR based on `[su](add|sub).with.overlow`.

For the fallback for LLVM < 8 I'm using the same expansion that target lowering in LLVM uses, which is not the same as Rust currently uses (in particular due to the use of selects rather than branches).

Fixes #55286.
Fixes #52203.
Fixes #44500.

r? @nagisa
@scottmcm
Copy link
Member

scottmcm commented Jul 6, 2019

It's awesome seeing how much better things like Chain::size_hint are with saturating, since it now figures out sometimes that it can't overflow and removes the saturating-ness 🙂

If you have available time, consider having LLVM fold uadd.sat(a, b) == 0 into or(a, b) == 0.

@nikic
Copy link
Contributor

nikic commented Jul 6, 2019

@scottmcm Do you have an example where the uadd.sat(a, b) == 0 pattern turns up in practice?

@scottmcm
Copy link
Member

scottmcm commented Jul 8, 2019

@nikic The place I ran into it was #35428 (comment), trying to figure out whether a Chain is empty based on its size_hint. Simple godbolt demo: https://rust.godbolt.org/z/gKKzIu

(Something other than or could be fine too; I just picked that because LLVM folds x == 0 && y == 0 into (x | y) == 0, so thought it seemed reasonable.)

But I acknowledge you're busy and it's not urgent.

@nikic
Copy link
Contributor

nikic commented Oct 20, 2019

@scottmcm I've opened https://reviews.llvm.org/D69224 to add this fold.

@scottmcm
Copy link
Member

That's awesome, @nikic; Thanks!

I particularly like the other forms of it you did; I hadn't even thought about usub.sat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants