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

Solutions for ClangCL intrin0.h header for _STL_INTRIN_HEADER macro #4282

Closed

Conversation

MaxEW707
Copy link

@MaxEW707 MaxEW707 commented Dec 21, 2023

Background Reading

See this MSVC STL PR from last year for historical context: #3285

See this Clang bug report that spawned this work: llvm/llvm-project#53520
See this Clang PR for context on the changes we would like to make to clang to support intrin0.h: llvm/llvm-project#75711

Problem

As you are well aware intrin0.h is a header shipped with MSVC that stores the minimal set of intrinsics for the MSVC STL to reduce header include time.

immintrin.h shipped with ClangCL only includes the simd intrinsics if they are enabled at compile time. This prevents the ability to do runtime simd detection with ClangCL.
This was done to reduce the header include times from _STL_INTRIN_HEADER using intrin.h which ends up including immintrin.h.

To solve this I am adding a similar intrinsic header to ClangCL to mimic intrin0.h for MSVC STL.

Solution

At noted above we already have a PR up that provides the minimal set of intrinsics that the MSVC STL requires.

We would like to come to some agreement about how to do the feature check so we can use this new minimal intrin header when it becomes available. We can be flexible and we want our solution to work for you. Not force our solution onto you.

We can't use __has_include(<intrin0.h>) because MSVC ships its own copy of intrin0.h and thus can't distinguish it from ours.

We can't use __has_include_next(<intrin0.h>) inside yvals_core.h to determine if ClangCL is providing its own header since clang will rightfully warn that it will be found by absolute path, "#include_next in file found relative to primary source file or found by absolute path; will search from start of include path [-Winclude-next-absolute-path]".

We can do a version check for when this header will be introduced.

The proposed solution that I am thinking is the way to go is for clang to have its own custom name for the header such as intrin_msvc_stl.h and then we can easily do a __has_include check.

I don't intend for this PR to get merged before we merge the clang PR linked above. Would appreciate which avenue you guys would like us to go down to support this feature.
Thanks :).

@MaxEW707 MaxEW707 requested a review from a team as a code owner December 21, 2023 22:19
@github-actions github-actions bot added this to Initial Review in Code Reviews Dec 21, 2023
stl/inc/yvals_core.h Show resolved Hide resolved
@@ -2034,8 +2034,14 @@ compiler option, or define _ALLOW_RTCc_IN_STL to suppress this error.
#endif // ^^^ !defined(__cpp_noexcept_function_type) ^^^

#ifdef __clang__
#if __clang_major__ >= 18
Copy link
Member

Choose a reason for hiding this comment

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

I have a mild preference for detection via __clang_major__ over using a distinct header name. We don't support compilers older than most-recently-shipped-with-VS, so this will let us replace the whole mess with #include <intrin0.h> in a release cycle or two.

We'll see if other maintainers have different opinions. (STL is notably on vacation for another week.)

Copy link
Author

@MaxEW707 MaxEW707 Jan 14, 2024

Choose a reason for hiding this comment

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

llvm/llvm-project@f39e4aa for reference.

Quick update. I added an interposing yvals_core.h header to my PR which ensures that when we introduce this change we do not reduce throughput for users upgrading clang but still referring to an older version of MSVC STL.

If that solution is accepted and when that PR gets merged and shipped with some release of clang you should be able to just replace _STL_INTRIN_HEADER with <intrin0.h> once MSVC STL targets that release of clang with the intrin0.h header.

@CaseyCarter CaseyCarter added the enhancement Something can be improved label Jan 4, 2024
@CaseyCarter
Copy link
Member

Thanks for the extensive writeup with links to the source material, I needed a refresher and I suspect other maintainers are less familiar with the history of clang-cl and intrin.h. I'm glad someone finally found the time and motivation to make a clang-specific intrin0.h.

@StephanTLavavej
Copy link
Member

We talked about this at the weekly maintainer meeting - thanks for working on improving the throughput situation here! Your LLVM PR appears to be progressing nicely and it looks like there are no changes needed in microsoft/STL right now. @CaseyCarter mentioned that we should be able to close this PR now - does that sound right to you?

@StephanTLavavej StephanTLavavej removed their assignment Jan 24, 2024
@MaxEW707
Copy link
Author

@CaseyCarter mentioned that we should be able to close this PR now - does that sound right to you?

Correct feel free to close this PR since it appears our direction on Clang is satisfactory for you guys. I can open another PR if required in the future.

@StephanTLavavej
Copy link
Member

Excellent, thank you! 😻

Code Reviews automation moved this from Initial Review to Done Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
throughput Must compile faster
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants