[PATCH] D46863: [X86] Use __builtin_convertvector to implement some of the packed integer to packed float conversion intrinsics.
DavidKreitzer added a comment. This looks good to me, Craig. I am not worried about the constant folding issue, as I think constant folding these conversion intrinsics (assuming round-to-nearest) is a perfectly valid optimization in the absence of FENV_ACCESS. (FWIW, we don't do this constant folding in icc, but that is only because we have never gotten around to implementing it.) I am also not worried about the spurious 'inexact' exceptions that the changes to the mask intrinsics will cause. Repository: rC Clang https://reviews.llvm.org/D46863 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47029: [X86] Remove some preprocessor feature checks from intrinsic headers
DavidKreitzer added a comment. Looks right to me, Craig. https://reviews.llvm.org/D47029 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D46863: [X86] Use __builtin_convertvector to implement some of the packed integer to packed float conversion intrinsics.
DavidKreitzer accepted this revision. DavidKreitzer added a comment. This revision is now accepted and ready to land. I had actually done that as part of my initial review, so LGTM. Repository: rC Clang https://reviews.llvm.org/D46863 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47174: [X86] Move 128-bit f16c intrinsics to __emmintrin_f16c.h include from emmintrin.h. Move 256-bit f16c intrinsics back to f16cintrin.h
DavidKreitzer added a comment. A bit of history: In icc, the f16<=>f32 conversion intrinsics are a bit of an anomaly in that they can be implemented using either native code or emulation code based on the target architecture switch. See https://godbolt.org/g/bQy7xY (thanks, Craig, for the example code). The emulation code lives in the Intel Math Library. The reason icc chose to declare the scalar & 128-bit versions of the intrinsics in emmintrin.h rather than a header file that more closely corresponds to the f16c feature is that emmintrin.h contains the minimum necessary to use the emulation code, i.e. the declaration of the __m128i type. Given that clang doesn't support the lowering of these intrinsics to emulation code, I don't see much benefit including them in emmintrin.h. It would make more sense to just put everything in f16cintrin.h and include that from immintrin.h. In brief, I like your changes in immintrin.h. I would move the code from _emmintrin_f16c.h into f16cintrin.h. And I would remove the include from emmintrin.h. I think that would be consistent with gcc as well. We can let the emulation behavior of these intrinsics remain an icc-specific anomaly. Repository: rL LLVM https://reviews.llvm.org/D47174 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47182: [X86] Move all Intel defined intrinsic includes into immintrin.h
DavidKreitzer added a comment. I agree with the changes in x86intrin.h and immintrin.h. For the others, I question whether we ought to recommend inclusion of x86intrin.h or immintrin.h. The distinction as I understand it is that immintrin.h is used for Intel-specific intrinsics while x86intrin.h covers all intrinsics for x86-based architectures. Repository: rC Clang https://reviews.llvm.org/D47182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47182: [X86] Move all Intel defined intrinsic includes into immintrin.h
DavidKreitzer added a comment. Hi Craig, just one comment on the details. Everything else looks good. Comment at: lib/Headers/x86intrin.h:47 - -#if !defined(_MSC_VER) || __has_feature(modules) || defined(__POPCNT__) -#include I see that you are removing this popcntintrin.h inclusion without adding it to immintrin.h. Is that because you are relying on the inclusion from smmintrin.h? If so, won't that cause a problem on Windows with -mpopcnt for targets that don't include smmintrin.h? https://reviews.llvm.org/D47182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47182: [X86] Move all Intel defined intrinsic includes into immintrin.h
DavidKreitzer accepted this revision. DavidKreitzer added a comment. This revision is now accepted and ready to land. Thanks, Craig. LGTM. https://reviews.llvm.org/D47182 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
