-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Sequential checked_add and saturating_add of constants don't get combined together #52203
Comments
I ask myself if this should be fixed in LLVM directly, rather than rust, so two consecutive saturatings/checked adds should be merged? |
@hellow554 Yes, it'll probably need LLVM changes unless there's some way we could emit it differently that would work better -- this would be rather complicated to do in MIR, AFAICT. Edit: oops, palm-clicked trying to reply on my phone... |
Confirmed that LLVM can't do this kind of fold in C++ either. Filed an LLVM bug:
|
Might get fixed for saturating, or at least easier to fix, if #55286 happens. |
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
gets compiled to
but ideally it would get compiled to
a.saturating_add(20)
, akaDitto for checked_add.
Playground repro: https://play.rust-lang.org/?gist=eeea31ea85491e38bdd6d456b9f6e47f&version=nightly&mode=release&edition=2015
Found looking at step_by, which would like
.next(); .nth(step)
to collapse nicely for ranges (#52065).The text was updated successfully, but these errors were encountered: