<div class="gmail_quote">On Sun, Feb 19, 2012 at 6:48 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 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>
</div></div>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></blockquote><div><br></div><div>The question is, if those intrinsics are valid, why aren't the defines present? My suspicion is that we're either not correctly detecting the CPU variant targeted, we're setting our default CPU target too conservatively compared to what Microsoft's compiler does (in which case we should change the defaults under the MS compat modes), or we're checking the wrong spelling of defines in our immintrin.h file.</div>
<div><br></div><div>It shouldn't matter how the files are organized, we should ensure that the necessary defines are present to get the necessary headers. That does mean that the CPU needs to support executing those instructions though.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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></blockquote><div><br></div><div>If you can find a document that describes the expected macro interfaces exported by Microsoft's intrin.h, then we should be able to add those macros to our headers when compiling for that target.</div>
<div><br></div><div>Note that we need a document describing the interface, and we need someone who has not read the MS headers in any detail (which it seems you have, and now that I've read your email I have as well) to read just the documentation and implement the macros from scratch for inclusion and licensing as part of Clang's builtin header files.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Beyond that, I'm a bit more worried about the drastic differences<br>
between the type declarations. I know there is code out there which<br>
makes use of the union for __m64, for instance. So that code will<br>
break because the clang intrinsics do not use it (they use long long<br>
instead).<br></blockquote><div><br></div><div>This code is exceedingly misbehaved. These interfaces are defined by a spec from Intel. Code should be written according to that spec, and both Clang and Microsoft's headers should implement that spec.</div>
<div><br></div><div>If Microsoft has some alternate spec we should additionally support, then the samething holds as above with the macros: someone can extend Clang's headers to support the MS spec when targeting MS code. They'll need to follow the rules I mentioned above where they have *only* read documentation of the spec.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">But perhaps that can be worked around by conditionally compiling in<br>
the declarations used by the Microsoft headers?<br></blockquote><div><br></div><div>The goal should be for Clang's builtin headers to support the desired interface to these builtin headers without relying on the MS headers. The reason is that the MS headers use compiler extensions specific to MS. We don't necessarily want to implement the spec using those extensions in Clang, and the header file provides an abstraction mechanism for that purpose.</div>
</div>