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

Sequential checked_add and saturating_add of constants don't get combined together #52203

Closed
scottmcm opened this issue Jul 10, 2018 · 4 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. WG-llvm Working group: LLVM backend code generation

Comments

@scottmcm
Copy link
Member

pub fn test_saturating(a: u32) -> u32 {
    a.saturating_add(7).saturating_add(13)
}

gets compiled to

playground::test_saturating:
	add	edi, 7
	mov	eax, -1
	cmovb	edi, eax
	add	edi, 13
	cmovae	eax, edi
	ret

but ideally it would get compiled to a.saturating_add(20), aka

playground::test_saturating:
	add	edi, 20
	mov	eax, -1
	cmovae	eax, edi
	ret

Ditto 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).

@oli-obk oli-obk added the WG-llvm Working group: LLVM backend code generation label Jul 10, 2018
@hellow554
Copy link
Contributor

I ask myself if this should be fixed in LLVM directly, rather than rust, so two consecutive saturatings/checked adds should be merged?

@scottmcm
Copy link
Member Author

scottmcm commented Jul 10, 2018

@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...

@scottmcm scottmcm reopened this Jul 10, 2018
@scottmcm scottmcm added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Jul 10, 2018
@scottmcm
Copy link
Member Author

scottmcm commented Jul 16, 2018

Confirmed that LLVM can't do this kind of fold in C++ either. Filed an LLVM bug:

@scottmcm
Copy link
Member Author

scottmcm commented Oct 25, 2018

Might get fixed for saturating, or at least easier to fix, if #55286 happens.

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
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. WG-llvm Working group: LLVM backend code generation
Projects
None yet
Development

No branches or pull requests

3 participants