You’re right, GCC has these in a header as a #define. This patch didn’t interfere with that and was causing some other failures for us as a result. If its OK, I’d prefer to revert ONLY the enable on Linux mode at the moment, the long-int fix WAS valuable as well.
From: James Y Knight [mailto:jykni...@google.com] Sent: Thursday, March 7, 2019 3:52 PM To: Keane, Erich <erich.ke...@intel.com> Cc: cfe-commits <cfe-commits@lists.llvm.org> Subject: Re: r355322 - Enable _rotl, _lrotl, _rotr, _lrotr on all platforms. This patch breaks some code which is (conditionally) defining functions of these names on certain platforms. Now, it's true that it shouldn't be doing that, and if the claim in the commit message about GCC was true, I'd just say "don't do that". But, the commit message is wrong. GCC does _not_ define these as builtins, it defines them in ia32intrin.h header (publicly via x86intrin.h) . IMO, this commit should be reverted, and functions defined in the header, instead. Perhaps similar to what 2c8f9c2c23e0cafd7b85a7aec969c949349f747c<https://github.com/llvm/llvm-project/commit/2c8f9c2c23e0cafd7b85a7aec969c949349f747c> did, although I note that was reverted in b62c5bc64dee3642884c31b8208c07a6f74b81fd<https://github.com/llvm/llvm-project/commit/b62c5bc64dee3642884c31b8208c07a6f74b81fd>, reportedly due to breaking mingw. (I'd further note that even MSVC doesn't enable these builtins by default! It implements a `#pragma intrinsic(NAME)` mechanism which you need to use in order to enable them (and #include <windows.h> will do so). But clang doesn't really implement that pragma, and instead enables all the MSVC-builtins unconditionally.) On Mon, Mar 4, 2019 at 1:46 PM Erich Keane via cfe-commits <cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote: Author: erichkeane Date: Mon Mar 4 10:47:21 2019 New Revision: 355322 URL: http://llvm.org/viewvc/llvm-project?rev=355322&view=rev Log: Enable _rotl, _lrotl, _rotr, _lrotr on all platforms. The above builtins are currently implemented for MSVC mode, however GCC also implements these. This patch enables them for all platforms. Additionally, this corrects the type for these builtins to always be 'long int' to match the specification in the Intel Intrinsics Guide. Change-Id: Ida34be98078709584ef5136c8761783435ec02b1 Added: cfe/trunk/test/CodeGen/rot-intrinsics.c (with props) Modified: cfe/trunk/include/clang/Basic/Builtins.def cfe/trunk/test/CodeGen/ms-intrinsics-rotations.c
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
