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

Always inline arithmetic operators #62095

Conversation

Mark-Simulacrum
Copy link
Member

In release mode, this has no effect, as they're inlined anyway, but in
debug mode LLVM otherwise lowers these to a call instruction -- much
more expensive than the inlined version.

Fixes #12279

If we decide not to land this I think we should close #12279 as won't fix.

r? @alexcrichton

In release mode, this has no effect, as they're inlined anyway, but in
debug mode LLVM otherwise lowers these to a call instruction -- much
more expensive than the inlined version.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 24, 2019
@Mark-Simulacrum
Copy link
Member Author

@bors try

I want to try to run perf to make sure this doesn't regress compilation time

@bors
Copy link
Contributor

bors commented Jun 24, 2019

⌛ Trying commit 363b20e with merge e300b3d1d935612cbd734b3683c726874af46838...

@bors
Copy link
Contributor

bors commented Jun 24, 2019

☀️ Try build successful - checks-travis
Build commit: e300b3d1d935612cbd734b3683c726874af46838

@Mark-Simulacrum
Copy link
Member Author

@rust-timer build e300b3d1d935612cbd734b3683c726874af46838

@rust-timer
Copy link
Collaborator

Success: Queued e300b3d1d935612cbd734b3683c726874af46838 with parent 166c49d, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit e300b3d1d935612cbd734b3683c726874af46838, comparison URL.

@Mark-Simulacrum
Copy link
Member Author

Looks like no compile time changes (as hoped for).

@jonas-schievink
Copy link
Contributor

IIUC this change only affects code that is generic over these operators, and there might not be much of that in the benchmarks. Perhaps the num crates would be a better benchmark?

@Mark-Simulacrum
Copy link
Member Author

Perhaps. If @alexcrichton decides this is reasonable I'll collect perf before landing probably

@alexcrichton
Copy link
Member

Can you add a comment somewhere indicating why they're inline(always) and then comment each location pointing to that comment? Additionally is the purpose here to improve the runtime performance of debug binaries? If so what sort of speedups is this yielding?

@Mark-Simulacrum
Copy link
Member Author

I am unable to measure performance benefits, and since that's the main goal here I'm going to close this PR -- and the relevant issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

builtin arithmetic ops should be inlined when used in a generic function
6 participants