<div class="gmail_quote">On Sun, Feb 19, 2012 at 7:02 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">On Sun, Feb 19, 2012 at 8:48 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br>
> On Sun, Feb 19, 2012 at 7:41 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:<br>
>> On Sun, Feb 19, 2012 at 5:35 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>> wrote:<br>
>>><br>
>>> On Sun, Feb 19, 2012 at 5:18 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>><br>
>>> wrote:<br>
>>> > On Sun, Feb 19, 2012 at 3:09 PM, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>>> > wrote:<br>
>>> >><br>
>>> >> On Sun, Feb 19, 2012 at 4:45 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>><br>
>>> >> wrote:<br>
>>> >> > On Sun, Feb 19, 2012 at 10:28 AM, Aaron Ballman<br>
>>> >> > <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>><br>
>>> >> > wrote:<br>
>>> >> >> On Windows, the default search paths for includes would put the<br>
>>> >> >> CMake-found headers (<build dir>\lib\clang\3.1\include) before the<br>
>>> >> >> OS<br>
>>> >> >> headers. For the most part, this is fine. However, there are some<br>
>>> >> >> header files that share names with the system headers, which are<br>
>>> >> >> incorrect. For instance, immintrin.h is completely different from<br>
>>> >> >> the<br>
>>> >> >> SDK include. This causes miscompilations in surprising places<br>
>>> >> >> because<br>
>>> >> >> a system header will attempt to include one of these duplicates, and<br>
>>> >> >> find the clang header instead of the other system header.<br>
>>> >> >><br>
>>> >> >> A simple test case of: #include <map> when building from MSVC<br>
>>> >> >> demonstrates the issue.<br>
>>> >> >><br>
>>> >> >> A user can work around this behavior by specifying -nobuiltininc on<br>
>>> >> >> the command line, but that seems rather obtuse for such normal use<br>
>>> >> >> cases.<br>
>>> >> >><br>
>>> >> >> I've fixed this by changing the order that the headers are included<br>
>>> >> >> from the toolchain. The SDK headers get added *before* the clang<br>
>>> >> >> headers, and this resolves the issue.<br>
>>> >> >><br>
>>> >> >> This patch does cause one regression test to fail<br>
>>> >> >> (Modules\compiler_builtins.m). This is because the platform SDK<br>
>>> >> >> intrinsics are differently structured from the ones that come with<br>
>>> >> >> clang. The OS APIs expect the SDK intrinsics, but our tests seem to<br>
>>> >> >> require our own intrinsics. I'm open to suggestions.<br>
>>> >> ><br>
>>> >> > If on Windows, we specifically need the OS version of immintrin.h, we<br>
>>> >> > should modify our version to forward to it; completely ignoring the<br>
>>> >> > builtin headers is a bad idea (for example,<br>
>>> >> > <a href="http://llvm.org/bugs/show_bug.cgi?id=11918" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=11918</a> is a miscompile caused by<br>
>>> >> > pulling in the wrong version of stdarg.h).<br>
>>> >><br>
>>> >> The patch doesn't completely ignore the builtin headers though -- it<br>
>>> >> simply prefers the OS headers over the builtin headers.<br>
>>> ><br>
>>> ><br>
>>> > That doesn't work in the general case though -- look at all the places<br>
>>> > in<br>
>>> > our builtin headers where we specifically forward *to* the system<br>
>>> > headers.<br>
>>> > If the builtin doesn't come first, lots of things go wrong here.<br>
>>> ><br>
>>> > We should make our builtins always come first, and always be correct<br>
>>> > when<br>
>>> > they come first.<br>
>>> ><br>
>>> >> That being<br>
>>> >> said, I am not against forwarding our version. What would be a good<br>
>>> >> approach to it?<br>
>>> ><br>
>>> ><br>
>>> > Look at the other builtin headers that use '__has_include_next' and<br>
>>> > 'include_next' to fold together a Clang builtin header and a system<br>
>>> > builtin<br>
>>> > header.<br>
>>> ><br>
>>> > I suspect you will have to essentially tack on the system 'immintrin.h'<br>
>>> > header to the end of the builtin, as we do expect #include <immintrin.h><br>
>>> > to<br>
>>> > pull in the Clang builtin bits... Essentially, this doesn't seem like it<br>
>>> > should be either-or, it should be both.<br>
>>><br>
>>> So how should I handle the duplicate declarations? For instance:<br>
>>><br>
>>> // mmintrin.h from MSVC<br>
>>> typedef union __declspec(intrin_type) _CRT_ALIGN(8) __m64<br>
>>> {<br>
>>> unsigned __int64 m64_u64;<br>
>>> float m64_f32[2];<br>
>>> __int8 m64_i8[8];<br>
>>> __int16 m64_i16[4];<br>
>>> __int32 m64_i32[2];<br>
>>> __int64 m64_i64;<br>
>>> unsigned __int8 m64_u8[8];<br>
>>> unsigned __int16 m64_u16[4];<br>
>>> unsigned __int32 m64_u32[2];<br>
>>> } __m64;<br>
>>><br>
>>> void _m_empty(void);<br>
>>> #define _mm_empty _m_empty<br>
>>><br>
>>> // mmintrin.h from clang<br>
>>> typedef long long __m64 __attribute__((__vector_size__(8)));<br>
>>><br>
>>> static __inline__ void __attribute__((__always_inline__, __nodebug__))<br>
>>> _mm_empty(void)<br>
>>> {<br>
>>> __builtin_ia32_emms();<br>
>>> }<br>
>>> #define _m_empty _mm_empty<br>
>>><br>
>>> It seems to me that trying to keep *both* headers is going to be<br>
>>> rather challenging given that both make the same typedefs in<br>
>>> incompatible ways, the declarations/macros are opposite one another,<br>
>>> etc.<br>
>><br>
>><br>
>> Hold on -- the MS one is trying to provide the same intel intrinsics? Now I<br>
>> think that the original search order and header file is correct -- we don't<br>
>> want the MS version of this header as it is likely to use MS extensions<br>
>> Clang doesn't support (or for which Clang has better models). Maybe what we<br>
>> really need is to understand what the clang immintrin.h *lacks* in order for<br>
>> MS code to use it?<br>
><br>
> Yes, MS has the same intrinsics, but with differently-structured<br>
> header files. The MS immintrin.h has all of the AVX intrinsics and<br>
> includes only wmmintrin.h, which includes nmmintrin.h for SSE4.2, etc<br>
> The clang immintrin.h checks various defines (__AVX__, __AES__, etc)<br>
> before including all of the files.<br>
><br>
> One thing which does get us further in parsing is to force define the<br>
> includes from clang's intrinsics:<br>
><br>
> #define __AVX__<br>
> #define __AES__<br>
> #define __SSE3__<br>
> #define __SSSE3__<br>
> #define __SSE4_1__<br>
> #define __SSE4_2__<br>
><br>
> There are still compile errors, however, because of the way<br>
> Microsoft's intrin.h header file works. It has some macro hacks to<br>
> basically erase some declarations. For instance:<br>
><br>
> #define __MACHINE(X) X;<br>
> #define __MACHINEX86X_IA64 __MACHINE<br>
> ...<br>
> #define __MACHINEZ(X) /* NOTHING */<br>
> ...<br>
> #if !(defined(_M_IX86) || defined(_M_IA64))<br>
> #undef __MACHINEX86X_IA64<br>
> #define __MACHINEX86X_IA64 __MACHINEZ<br>
> #endif<br>
> ...<br>
> __MACHINEX86X_IA64(__m64 _m_pshufw(__m64,int))<br>
><br>
> // Microsoft headers<br>
> extern __m64 _m_pshufw(__m64, int);<br>
> #define _mm_shuffle_pi16 _m_pshufw<br>
><br>
> // Clang headers<br>
> #define _mm_shuffle_pi16(a, n) __extension__ ({ \<br>
> __m64 __a = (a); \<br>
> (__m64)__builtin_ia32_pshufw((__v4hi)__a, (n)); })<br>
> #define _m_pshufw _mm_shuffle_pi16<br>
><br>
> This causes compile errors with clang like this:<br>
><br>
> error: expected unqualified-id<br>
> __MACHINEX86X_IA64(__m64 _m_pshufw(__m64,int))<br>
><br>
> Because this expands out into:<br>
><br>
> __m64 __extension__ ({ __m64 __a = (__m64);<br>
> (__m64)__builtin_ia32_pshufw((__v4hi)__a, (int)); });<br>
<br>
</div></div>I wonder if it would make sense to find all of the macro declarations<br>
in the intrinsics and turn them into something like this:<br>
<br>
<br>
#if defined(_MSC_VER)<br>
<div class="im">static __inline__ void __attribute__((__always_inline__, __nodebug__))<br>
</div>_mm_shuffle_pi16(__m64 a, int n) {<br>
return __buildin_ia32_pshufw((__v4hi)a, n);<br>
}<br>
#else<br>
<div class="im">#define _mm_shuffle_pi16(a, n) __extension__ ({ \<br>
__m64 __a = (a); \<br>
(__m64)__builtin_ia32_pshufw((__v4hi)__a, (n)); })<br>
</div>#endif<br>
<br>
There appear to only be a dozen or so cases of this. But I'm not<br>
certain how much customization we'd like in these header files,<br>
either.</blockquote><div><br></div><div>No, we shouldn't need two versions. See my previous email, I don't think this is the root cause of the problem, I think there is just some other interfaces that MS requires these headers to provide which we don't yet provide.</div>
</div>