Skip to content

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

Merged
StephanTLavavej merged 1 commit into
microsoft:mainfrom
fsb4000:force_inline_unreachable
Aug 27, 2022
Merged

add __forceinline to std::unreachable instead of inline#3055
StephanTLavavej merged 1 commit into
microsoft:mainfrom
fsb4000:force_inline_unreachable

Conversation

@fsb4000
Copy link
Copy Markdown
Contributor

@fsb4000 fsb4000 commented Aug 25, 2022

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

From #3051

And we already use __forceinline there:

STL/stl/inc/xcharconv_ryu.h

Line 166 in febb643

_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
Copy Markdown
Contributor

strega-nil-ms commented Aug 25, 2022

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.

strega-nil-ms
strega-nil-ms approved these changes Aug 25, 2022
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

StephanTLavavej commented Aug 25, 2022

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
Copy Markdown
Member

StephanTLavavej commented Aug 26, 2022

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
Copy Markdown
Member

StephanTLavavej commented Aug 27, 2022

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
Copy Markdown
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

Reviewers

@StephanTLavavej StephanTLavavej StephanTLavavej approved these changes

+2 more reviewers

@CaseyCarter CaseyCarter CaseyCarter approved these changes

@strega-nil-ms strega-nil-ms strega-nil-ms approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@StephanTLavavej StephanTLavavej

Labels

performance Must go faster

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@fsb4000 @strega-nil-ms @StephanTLavavej @AlexBAV @CaseyCarter