[Win/X86] Make _m_prefetch[w] builtins to avoid winnt.h conflicts #115099
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-x86 Author: Reid Kleckner (rnk) ChangesThis is similar in spirit to previous changes to make _mm_mfence builtins to avoid conflicts with winnt.h and other MSVC ecosystem headers that pre-declare compiler intrinsics as extern "C" symbols. This should fix issue #87515. Full diff: https://github.com/llvm/llvm-project/pull/115099.diff 3 Files Affected:
diff --git a/clang/include/clang/Basic/BuiltinsX86.def b/clang/include/clang/Basic/BuiltinsX86.def
index c93ea27f164e34..c45bb2a2a17431 100644
--- a/clang/include/clang/Basic/BuiltinsX86.def
+++ b/clang/include/clang/Basic/BuiltinsX86.def
@@ -31,10 +31,13 @@
// All MMX instructions will be generated via builtins. Any MMX vector
// types (<1 x i64>, <2 x i32>, etc.) that aren't used by these builtins will be
// expanded by the back-end.
+//
// FIXME: _mm_prefetch must be a built-in because it takes a compile-time constant
// argument and our prior approach of using a #define to the current built-in
// doesn't work in the presence of re-declaration of _mm_prefetch for windows.
-TARGET_BUILTIN(_mm_prefetch, "vcC*i", "nc", "mmx")
+TARGET_HEADER_BUILTIN(_mm_prefetch, "vcC*i", "nc", IMMINTRIN_H, ALL_LANGUAGES, "mmx")
+TARGET_HEADER_BUILTIN(_m_prefetch, "vv*", "nc", INTRIN_H, ALL_LANGUAGES, "")
+TARGET_HEADER_BUILTIN(_m_prefetchw, "vvDC*", "nc", INTRIN_H, ALL_LANGUAGES, "")
// SSE intrinsics.
diff --git a/clang/lib/CodeGen/CGBuiltin.cpp b/clang/lib/CodeGen/CGBuiltin.cpp
index 82770a75af23e4..5db354d8fab4e3 100644
--- a/clang/lib/CodeGen/CGBuiltin.cpp
+++ b/clang/lib/CodeGen/CGBuiltin.cpp
@@ -14774,6 +14774,16 @@ Value *CodeGenFunction::EmitX86BuiltinExpr(unsigned BuiltinID,
Function *F = CGM.getIntrinsic(Intrinsic::prefetch, Address->getType());
return Builder.CreateCall(F, {Address, RW, Locality, Data});
}
+ case X86::BI_m_prefetch:
+ case X86::BI_m_prefetchw: {
+ Value *Address = Ops[0];
+ // The 'w' suffix implies write.
+ Value *RW = ConstantInt::get(Int32Ty, BuiltinID == X86::BI_m_prefetchw ? 1 : 0);
+ Value *Locality = ConstantInt::get(Int32Ty, 0x3);
+ Value *Data = ConstantInt::get(Int32Ty, 1);
+ Function *F = CGM.getIntrinsic(Intrinsic::prefetch, Address->getType());
+ return Builder.CreateCall(F, {Address, RW, Locality, Data});
+ }
case X86::BI_mm_clflush: {
return Builder.CreateCall(CGM.getIntrinsic(Intrinsic::x86_sse2_clflush),
Ops[0]);
diff --git a/clang/lib/Headers/prfchwintrin.h b/clang/lib/Headers/prfchwintrin.h
index eaea5f3cf8febf..8ec55d7073716f 100644
--- a/clang/lib/Headers/prfchwintrin.h
+++ b/clang/lib/Headers/prfchwintrin.h
@@ -14,6 +14,10 @@
#ifndef __PRFCHWINTRIN_H
#define __PRFCHWINTRIN_H
+#if defined(__cplusplus)
+extern "C" {
+#endif
+
/// Loads a memory sequence containing the specified memory address into
/// all data cache levels.
///
@@ -26,11 +30,7 @@
///
/// \param __P
/// A pointer specifying the memory address to be prefetched.
-static __inline__ void __attribute__((__always_inline__, __nodebug__))
-_m_prefetch(void *__P)
-{
- __builtin_prefetch (__P, 0, 3 /* _MM_HINT_T0 */);
-}
+void _m_prefetch(void *__P);
/// Loads a memory sequence containing the specified memory address into
/// the L1 data cache and sets the cache-coherency state to modified.
@@ -48,13 +48,10 @@ _m_prefetch(void *__P)
///
/// \param __P
/// A pointer specifying the memory address to be prefetched.
-static __inline__ void __attribute__((__always_inline__, __nodebug__))
-_m_prefetchw(volatile const void *__P)
-{
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wcast-qual"
- __builtin_prefetch ((const void*)__P, 1, 3 /* _MM_HINT_T0 */);
-#pragma clang diagnostic pop
-}
+void _m_prefetchw(volatile const void *__P);
+
+#if defined(__cplusplus)
+} // extern "C"
+#endif
#endif /* __PRFCHWINTRIN_H */
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
RKSimon
reviewed
Nov 6, 2024
| TARGET_BUILTIN(_mm_prefetch, "vcC*i", "nc", "mmx") | ||
| TARGET_HEADER_BUILTIN(_mm_prefetch, "vcC*i", "nc", IMMINTRIN_H, ALL_LANGUAGES, "mmx") | ||
| TARGET_HEADER_BUILTIN(_m_prefetch, "vv*", "nc", INTRIN_H, ALL_LANGUAGES, "") | ||
| TARGET_HEADER_BUILTIN(_m_prefetchw, "vvDC*", "nc", INTRIN_H, ALL_LANGUAGES, "") |
There was a problem hiding this comment.
Should we tag these with MMX as well?
There was a problem hiding this comment.
Yes, we should, good catch
|
Maybe a stupid question, but couldn't we make the function simply |
|
So, this is interesting, because the decls already don't conflict, normally. They only conflict if the x86intrin.h is included within a It comes down to, effectively this. Note, run the examples with static void _m_prefetchw() {} // from prfchwintrin.h
extern "C" { void _m_prefetchw(); } // from winnt.hNotably, surrounding the first in extern "C++" still doesn't trigger an error: extern "C++" { static void _m_prefetchw() {} }
extern "C" { void _m_prefetchw(); }even though it would've if the declaration didn't say "static": extern "C++" { void _m_prefetchw() {} }
extern "C" { void _m_prefetchw(); } // errorWhich I guess means Clang decides language-linkage doesn't really matter for internal linkage functions? I haven't traced where in the code that happens, but it seems vaguely sensible. Yet, reversing the order: extern "C" { void _m_prefetchw(); }
extern "C++" { static void _m_prefetchw() {} }does throw an "error: declaration of '_m_prefetchw' has a different language linkage" on the second line.. If language-linkage doesn't matter for static functions (seems sensible), I think we probably also shouldn't throw an error for that last case. And, if we did stop throwing that error, the incompatibility here disappears, and this PR is unnecessary. |
|
(...and maybe we could also get rid of the similar hacks we did for _mm_mfence/etc before? The commit message for 727ab8a didn't say anything about the rationale, but if it's the same as this, then perhaps so?) |
|
We can adjust the rules around language linkage if we like, but the main reason we implement builtins this way is to support the MSVC intrinsic model, which is to declare extern "C" functions and mark them with winnt.h provides macros that do stuff like this without including our intrinsic headers, so anything they mention this way has to get implemented as a compiler built-in, otherwise users experience surprising linker errors like "_m[m]_prefetch symbol not defined". The Clang project policy is to be MSVC-compatible enough to compile the system headers. Reimplementing the entire Intel intrinsic API as builtins is out of scope. Any non-system, user code using this mechanism to call Intel vector intrinsics should be updated to include immintrin.h instead. As a side benefit, this is also good for compile time, since the immintrin.h header is a giant umbrella header that's very bad for compile time. See also intrin0.h, which the MSVC STL uses as a compile time optimization. |
rnk
commented
Jan 30, 2025
| TARGET_BUILTIN(_mm_prefetch, "vcC*i", "nc", "mmx") | ||
| TARGET_HEADER_BUILTIN(_mm_prefetch, "vcC*i", "nc", IMMINTRIN_H, ALL_LANGUAGES, "mmx") | ||
| TARGET_HEADER_BUILTIN(_m_prefetch, "vv*", "nc", INTRIN_H, ALL_LANGUAGES, "") | ||
| TARGET_HEADER_BUILTIN(_m_prefetchw, "vvDC*", "nc", INTRIN_H, ALL_LANGUAGES, "") |
There was a problem hiding this comment.
Yes, we should, good catch
| ((sel) >> 2) & 1, (sel) & 0x3)) | ||
| #endif | ||
| /// | ||
| /// _mm_prefetch is implemented as a "library builtin" directly in Clang, |
There was a problem hiding this comment.
How does this interact with doxygen (https://clang.llvm.org/doxygen/xmmintrin_8h.html#a938d3f37a8561a80cecbac4f7b55898f)?
There was a problem hiding this comment.
Probably not well. I can put this inside an #if 0 block or some other macro construct so that Doxygen sees it but it never interacts with user code, but I don't have a working doxygen install to confirm if it will work.
There was a problem hiding this comment.
Can we use /// \fn _mm_prefetch(const void *a, const int sel)?
phoebewang
reviewed
Jan 31, 2025
|
|
||
| let Features = "mmx", Header = "intrin.h", Attributes = [NoThrow, Const] in { | ||
| def _m_prefetch : X86LibBuiltin<"void(void *)">; | ||
| def _m_prefetchw : X86LibBuiltin<"void(void volatile const *)">; |
There was a problem hiding this comment.
prefetchw should map to feature prfchw?
There was a problem hiding this comment.
Yes, it should! I looked at the Intel intrinsic documentation, and it said these intrinsics were part of the deprecated 3dnow ISA extension, and I wasn't sure what to. However, I took the time to check the Intel ISA manual and I updated this feature set and the _mm_prefetch feature check to "sse", since that seems to be the correct feature. PTAL, since I've expanded scope a bit.
There was a problem hiding this comment.
The whole thing is sort of confusing...
AMD originally implemented 3dnow including prefetch and prefetchw instructions. Intel then implemented SSE with different prefetch instructions... but didn't include one with a write hint. Later, they implemented prefetchw, and added a corresponding CPUID bit.
Modern LLVM never generates "prefetch"; _m_prefetch is actually lowered to the SSE prefetcht0.
_mm_prefetch(x, _MM_HINT_ET0) generates different instructions depending on the command-line: if the target only supports SSE, it generates prefetcht0. If it supports prefetchw (-mprfchw), it generates prefetchw.
I guess given that behavior, this feature mapping is probably fine?
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/17265 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/30/builds/15169 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/200/builds/2995 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/123/builds/12987 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/14019 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/11704 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/19027 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/16198 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/181/builds/12972 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/2/builds/16328 Here is the relevant piece of the build log for the reference |
aeubanks
added a commit
that referenced
this pull request
Feb 3, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/7575 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/174/builds/12484 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/46/builds/11529 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/12182 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/51/builds/10271 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/89/builds/15828 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/21790 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/17817 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/18711 Here is the relevant piece of the build log for the reference |
|
The build is still broken (e.g. https://lab.llvm.org/buildbot/#/builders/63/builds/3861). I'll back it out. |
Thanks, I was just about to report the same issue as well; this seems to conflict with winnt.h, at least in older versions of WinSDK (10.0.18362.0 in my case too). I don't have more data to go on, other than what's in that build log: |
phoebewang
reviewed
Feb 4, 2025
| @@ -138,6 +142,12 @@ let Attributes = [Const, NoThrow, RequiredVectorWidth<256>], Features = "avx" in | |||
| } | |||
| } | |||
|
|
|||
| // PRFCHW | |||
| let Features = "prfchw", Header = "x86intrin.h", Attributes = [NoThrow, Const] in { | |||
| def _m_prefetch : X86LibBuiltin<"void(void *)">; | |||
There was a problem hiding this comment.
Only _m_prefetchw requires "prfchw". _m_prefetch can be put together with _mm_prefetch.
There was a problem hiding this comment.
PTAL at the new patch: #138360
At least based on the header structure before this, they were both in prftchwintrin.h , so I haven't made this change.
Thank you for doing that. I missed all notifications because they got lost in the storm of failures due to the mismatch between my local build and what was on github. |
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
…vm#115099) This is similar in spirit to previous changes to make _mm_mfence builtins to avoid conflicts with winnt.h and other MSVC ecosystem headers that pre-declare compiler intrinsics as extern "C" symbols. Also update the feature flag for _mm_prefetch to sse, which is more accurate than mmx. This should fix issue llvm#87515.
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
…icts (llvm#115099)" This broke the build, see buildbot comments on the PR. This reverts commit ee92122 and follow-up 5dccfd9.
rnk
added a commit
to rnk/llvm-project
that referenced
this pull request
May 2, 2025
…licts (llvm#115099)" This reverts commit 83ff9d4. Don't change the builtin signature of _mm_prefetch this time.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is similar in spirit to previous changes to make _mm_mfence builtins to avoid conflicts with winnt.h and other MSVC ecosystem headers that pre-declare compiler intrinsics as extern "C" symbols.
This should fix issue #87515.