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

add __forceinline to std::unreachable instead of inline #3055

Merged
merged 1 commit into from
Aug 27, 2022

Conversation

fsb4000
Copy link
Contributor

@fsb4000 fsb4000 commented Aug 25, 2022

Co-authored-by: Alexander Bessonov [email protected]

From #3051

And we already use __forceinline there:

_NODISCARD __forceinline uint64_t __ryu_umul128(const uint64_t __a, const uint64_t __b, uint64_t* const __productHi) {

@fsb4000 fsb4000 requested a review from a team as a code owner August 25, 2022 16:09
@strega-nil-ms
Copy link
Contributor

This looks extremely reasonable to me, personally - we know that MSVC does not consider [[noreturn]] functions for inlining unless they're marked with __forceinline. I would like to see what the opinions of the other maintainers are, though.

Copy link
Contributor

@strega-nil-ms strega-nil-ms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the rest of the team thinks this is reasonable, this is obviously the correct way to implement this.

@StephanTLavavej StephanTLavavej added the performance Must go faster label Aug 25, 2022
@StephanTLavavej
Copy link
Member

I agree with @strega-nil-ms, and @AlexBAV's codegen example https://godbolt.org/z/a9d33hc5b shows that this makes a difference for /O2. In __ryu_umul128, the use of __forceinline was a compiler workaround (marked TRANSITION), but here the compiler's behavior regarding [[noreturn]] is reasonable and so __forceinline is also a reasonable perma-workaround. (The Ryu/charconv usage proves that __forceinline shouldn't cause other issues due to its non-Standardness.)

@StephanTLavavej StephanTLavavej self-assigned this Aug 26, 2022
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej merged commit c23bbd8 into microsoft:main Aug 27, 2022
@StephanTLavavej
Copy link
Member

Thanks for improving the STL's performance to previously unreachable levels! 📈 🚀 😹

@fsb4000 fsb4000 deleted the force_inline_unreachable branch August 27, 2022 01:42
@AlexBAV
Copy link
Contributor

AlexBAV commented Aug 27, 2022

Thanks for improving the STL's performance to previously unreachable levels! 📈 🚀 😹

You're always welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants