Page MenuHomePhabricator

Add some MS aliases for existing intrinsics
ClosedPublic

Authored by agutowski on Sep 7 2016, 8:15 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

agutowski updated this revision to Diff 70642.Sep 7 2016, 8:15 PM
agutowski retitled this revision from to Add some MS aliases for existing intrinsics.
agutowski updated this object.
agutowski added a subscriber: cfe-commits.
rnk added a reviewer: rsmith.Sep 8 2016, 10:22 AM
rnk edited edge metadata.

+Richard for ideas on how to navigate the intel intrinsics

include/clang/Basic/BuiltinsX86.def
304 ↗(On Diff #70642)

Part of the idea behind the Intel *mmintrin.h headers is that they define symbols outside of the implementor's namespace. Users should be able to write code that uses these _mm_* names if they don't include those headers. Having this builtin always be available on x86 makes that impossible.

That said, I can see that winnt.h uses these directly, rather than including xmmintrin.h, so we need them to be builtins in MSVC mode, and it's annoying to have them sometimes be builtins and sometimes be functions.

majnemer added inline comments.Sep 8 2016, 10:28 AM
include/clang/Basic/BuiltinsX86.def
304 ↗(On Diff #70642)

We could have the header file use a pragma which turns the _mm intrinsics on along the same lines as #pragma intrinsic. For MSVC compat, we could treat them as-if they were enabled by some similar mechanism.

rsmith added inline comments.Sep 8 2016, 11:18 AM
include/clang/Basic/BuiltinsX86.def
304 ↗(On Diff #70642)

Names starting with an underscore are reserved to the implementation for use in the global namespace. Also, we declare these lazily, on-demand, only if name lookup would otherwise fail, so (outside of odd SFINAE cases in C++) this shouldn't affect the meaning of any valid programs. While I find it distasteful to model these as builtins, the only significant problem I see here is that we'll accept non-portable code that forgets to include the right header if we do so.

Perhaps we could model this somewhat similarly to LIBBUILTIN, so that we allow use of the builtin without a declaration, but produce a diagnostic about the missing #include when we do so?

rnk added inline comments.Sep 8 2016, 2:35 PM
include/clang/Basic/BuiltinsX86.def
304 ↗(On Diff #70642)

+1 for doing something like LIBBUILTIN

agutowski updated this revision to Diff 70923.Sep 9 2016, 3:51 PM
agutowski edited edge metadata.

Changed way of handling Intel intrinsics
Removed constant folding

agutowski updated this revision to Diff 70926.Sep 9 2016, 4:31 PM

Separated Intel intrinsics tests

rnk added inline comments.Sep 13 2016, 11:32 AM
test/Sema/implicit-intel-builtin-decl.c
4 ↗(On Diff #70926)

Can you add a C++ test? I think you'll get "undeclared identifier '_mm_getcsr'" instead of this helpful diagnostic.

agutowski added inline comments.Sep 13 2016, 11:43 AM
test/Sema/implicit-intel-builtin-decl.c
4 ↗(On Diff #70926)

This diagnostic works also in C++, although that's not the case with _byteswap functions - if they are LIBBUILTINs, the C++ diagnostic doesn't work, and LANGBUILTINs doesn't have header dependencies. I can create another thing like TARGET_HEADER_BUILTIN, but I don't know if that's a good idea.

agutowski updated this revision to Diff 71212.Sep 13 2016, 11:48 AM

Add test for implicitly declared intel intrinsic diagnostic in C++

rnk accepted this revision.Sep 13 2016, 11:51 AM
rnk edited edge metadata.

lgtm after merging the test back.

test/Sema/implicit-intel-builtin-decl.c
4 ↗(On Diff #70926)

OK, it seems fine as is.

Given that there are no diagnostic differences, I would suggest copy-pasting the RUN line at the top of this file and adding -x c++ to avoid duplicating the whole test.

This revision is now accepted and ready to land.Sep 13 2016, 11:51 AM
agutowski updated this revision to Diff 71213.Sep 13 2016, 11:56 AM
agutowski edited edge metadata.

Merge C and C++ tests

This revision was automatically updated to reflect the committed changes.
agutowski reopened this revision.Sep 13 2016, 4:35 PM

xmmintrin.h and emmintrin.h broke compilation when included inside extern "C++" block

This revision is now accepted and ready to land.Sep 13 2016, 4:35 PM
agutowski updated this revision to Diff 71271.Sep 13 2016, 4:55 PM

Add extern "C" to Intel intrinsics declarations

rnk added a comment.Sep 13 2016, 8:17 PM

lgtm We could define a macro that conditionally expands to extern "C", but then we'd have to undef it, and that seems like no good.

alexshap added inline comments.
include/clang/Basic/Builtins.h
142 ↗(On Diff #71271)

should the doxygen comment start with /// here ?

agutowski updated this revision to Diff 71404.Sep 14 2016, 11:28 AM

Fix doxygen comment

agutowski marked an inline comment as done.Sep 14 2016, 11:31 AM
This revision was automatically updated to reflect the committed changes.