On Sun, Feb 19, 2012 at 9:20 PM, Aaron Ballman <[email protected]> wrote: > On Sun, Feb 19, 2012 at 9:02 PM, Chandler Carruth <[email protected]> > wrote: >> On Sun, Feb 19, 2012 at 6:48 PM, Aaron Ballman <[email protected]> >> wrote: >>> >>> On Sun, Feb 19, 2012 at 7:41 PM, Chandler Carruth <[email protected]> >>> wrote: >>> > On Sun, Feb 19, 2012 at 5:35 PM, Aaron Ballman <[email protected]> >>> > wrote: >>> >> >>> >> On Sun, Feb 19, 2012 at 5:18 PM, Chandler Carruth >>> >> <[email protected]> >>> >> wrote: >>> >> > On Sun, Feb 19, 2012 at 3:09 PM, Aaron Ballman >>> >> > <[email protected]> >>> >> > wrote: >>> >> >> >>> >> >> On Sun, Feb 19, 2012 at 4:45 PM, Eli Friedman >>> >> >> <[email protected]> >>> >> >> wrote: >>> >> >> > On Sun, Feb 19, 2012 at 10:28 AM, Aaron Ballman >>> >> >> > <[email protected]> >>> >> >> > wrote: >>> >> >> >> On Windows, the default search paths for includes would put the >>> >> >> >> CMake-found headers (<build dir>\lib\clang\3.1\include) before >>> >> >> >> the >>> >> >> >> OS >>> >> >> >> headers. For the most part, this is fine. However, there are >>> >> >> >> some >>> >> >> >> header files that share names with the system headers, which are >>> >> >> >> incorrect. For instance, immintrin.h is completely different >>> >> >> >> from >>> >> >> >> the >>> >> >> >> SDK include. This causes miscompilations in surprising places >>> >> >> >> because >>> >> >> >> a system header will attempt to include one of these duplicates, >>> >> >> >> and >>> >> >> >> find the clang header instead of the other system header. >>> >> >> >> >>> >> >> >> A simple test case of: #include <map> when building from MSVC >>> >> >> >> demonstrates the issue. >>> >> >> >> >>> >> >> >> A user can work around this behavior by specifying -nobuiltininc >>> >> >> >> on >>> >> >> >> the command line, but that seems rather obtuse for such normal >>> >> >> >> use >>> >> >> >> cases. >>> >> >> >> >>> >> >> >> I've fixed this by changing the order that the headers are >>> >> >> >> included >>> >> >> >> from the toolchain. The SDK headers get added *before* the clang >>> >> >> >> headers, and this resolves the issue. >>> >> >> >> >>> >> >> >> This patch does cause one regression test to fail >>> >> >> >> (Modules\compiler_builtins.m). This is because the platform SDK >>> >> >> >> intrinsics are differently structured from the ones that come >>> >> >> >> with >>> >> >> >> clang. The OS APIs expect the SDK intrinsics, but our tests seem >>> >> >> >> to >>> >> >> >> require our own intrinsics. I'm open to suggestions. >>> >> >> > >>> >> >> > If on Windows, we specifically need the OS version of immintrin.h, >>> >> >> > we >>> >> >> > should modify our version to forward to it; completely ignoring >>> >> >> > the >>> >> >> > builtin headers is a bad idea (for example, >>> >> >> > http://llvm.org/bugs/show_bug.cgi?id=11918 is a miscompile caused >>> >> >> > by >>> >> >> > pulling in the wrong version of stdarg.h). >>> >> >> >>> >> >> The patch doesn't completely ignore the builtin headers though -- it >>> >> >> simply prefers the OS headers over the builtin headers. >>> >> > >>> >> > >>> >> > That doesn't work in the general case though -- look at all the >>> >> > places >>> >> > in >>> >> > our builtin headers where we specifically forward *to* the system >>> >> > headers. >>> >> > If the builtin doesn't come first, lots of things go wrong here. >>> >> > >>> >> > We should make our builtins always come first, and always be correct >>> >> > when >>> >> > they come first. >>> >> > >>> >> >> That being >>> >> >> said, I am not against forwarding our version. What would be a good >>> >> >> approach to it? >>> >> > >>> >> > >>> >> > Look at the other builtin headers that use '__has_include_next' and >>> >> > 'include_next' to fold together a Clang builtin header and a system >>> >> > builtin >>> >> > header. >>> >> > >>> >> > I suspect you will have to essentially tack on the system >>> >> > 'immintrin.h' >>> >> > header to the end of the builtin, as we do expect #include >>> >> > <immintrin.h> >>> >> > to >>> >> > pull in the Clang builtin bits... Essentially, this doesn't seem like >>> >> > it >>> >> > should be either-or, it should be both. >>> >> >>> >> So how should I handle the duplicate declarations? For instance: >>> >> >>> >> // mmintrin.h from MSVC >>> >> typedef union __declspec(intrin_type) _CRT_ALIGN(8) __m64 >>> >> { >>> >> unsigned __int64 m64_u64; >>> >> float m64_f32[2]; >>> >> __int8 m64_i8[8]; >>> >> __int16 m64_i16[4]; >>> >> __int32 m64_i32[2]; >>> >> __int64 m64_i64; >>> >> unsigned __int8 m64_u8[8]; >>> >> unsigned __int16 m64_u16[4]; >>> >> unsigned __int32 m64_u32[2]; >>> >> } __m64; >>> >> >>> >> void _m_empty(void); >>> >> #define _mm_empty _m_empty >>> >> >>> >> // mmintrin.h from clang >>> >> typedef long long __m64 __attribute__((__vector_size__(8))); >>> >> >>> >> static __inline__ void __attribute__((__always_inline__, __nodebug__)) >>> >> _mm_empty(void) >>> >> { >>> >> __builtin_ia32_emms(); >>> >> } >>> >> #define _m_empty _mm_empty >>> >> >>> >> It seems to me that trying to keep *both* headers is going to be >>> >> rather challenging given that both make the same typedefs in >>> >> incompatible ways, the declarations/macros are opposite one another, >>> >> etc. >>> > >>> > >>> > Hold on -- the MS one is trying to provide the same intel intrinsics? >>> > Now I >>> > think that the original search order and header file is correct -- we >>> > don't >>> > want the MS version of this header as it is likely to use MS extensions >>> > Clang doesn't support (or for which Clang has better models). Maybe what >>> > we >>> > really need is to understand what the clang immintrin.h *lacks* in order >>> > for >>> > MS code to use it? >>> >>> Yes, MS has the same intrinsics, but with differently-structured >>> header files. The MS immintrin.h has all of the AVX intrinsics and >>> includes only wmmintrin.h, which includes nmmintrin.h for SSE4.2, etc >>> The clang immintrin.h checks various defines (__AVX__, __AES__, etc) >>> before including all of the files. >> >> >> 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. > > I don't believe that's the issue at all. > > The problem *only* crops up where our declarations are macros instead > of functions. In every case (all 18), macro expansion is what screws > up. Microsoft's headers don't use any macro implementations of the > intrinsics -- only extern functions. Everywhere we use an inline > function compiles properly. > >> 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. > > Yes, and I think we're doing alright there.
After an IRC discussion, Chandler and I seem to be on the same page now. I'm not going to push for this patch, but will write up some prose about what I think needs to be done. Thanks (all)! ~Aaron _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
