[libcxx-commits] [PATCH] D114805: [NFC][libc++] Recognize int128 as an ABI affecting property

Mon Mar 7 10:14:44 PST 2022

ldionne added inline comments.

================
Comment at: libcxx/CMakeLists.txt:315
+check_cxx_symbol_exists(__SIZEOF_INT128__ "" LIBCXX_HAVE_INT128)
+cmake_dependent_option(LIBCXX_USE_INT128   "Use __int128 in the library" ON
+"LIBCXX_HAVE_INT128" OFF)
----------------
daltenty wrote:
> ldionne wrote:
> > I don't think we should allow this to be set explicitly during CMake configuration. If it's an ABI affecting property of the compiler, I believe we should just detect it and set/unset it unconditionally.
> > 
> > I can't imagine building libc++ with one compiler and using it with a compiler so different that it would differ in whether it supports `int128`. For example, compiling libc++ with a version of Clang and using the headers with a different version of Clang clearly works, however I wouldn't dare ship a library compiled with e.g. GCC to my users. Not saying it won't work, but it seems like living on the edge -- enough that I don't think it's worth supporting. WDYT?
> The only problem I see with taking the our direction from the build compiler is it doesn't even require something as esoteric as the example of mixing compilers you mentioned. If a compiler transitions in support for the type in a new release, you'd have no way to easily move up the build compiler while still selecting compatibility with the existing ABI.
>  
> For example, on AIX  clang's support for `__int128` only recently landed in https://reviews.llvm.org/D111078 for 64-bit, so clang-13 (and thus Open XL 17.1) don't support it, but clang-14 will.
> 
> That said, since the interfaces involving __int128 are relatively new and limited to C++ 20 we might still have some wiggle room about which ABI to choose on AIX, but it still seems better to give the configuration choice in case we aren't the only ones with this problem going forward.
Okay, I guess this makes sense. Could you also update the `AIX.cmake` cache accordingly? And also we should be running the `check-cxx-abilist` target on AIX (I assume we aren't right now). I think this patch would make more sense if we did.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D114805/new/

https://reviews.llvm.org/D114805