Define TEST_HAS_NO_INT128 accordingly.
Details
Details
- Reviewers
Mordante • Quuxplusone - Group Reviewers
Restricted Project - Commits
- rG8d58cb62da0f: [libcxx][test] Let the library indicate support for int128
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
This change probably is good in itself, but the situation of __int128_t in clang-cl environments is actually more complex than that.
Clang-cl actually does support code generation for the __int128_t type, but it doesn't have access to the division helper routines provided by compiler-rt builtins normally.
Regarding libc++ (which is only tangential, as I take it your commit is about testing STL with libc++'s testsuite) - in https://reviews.llvm.org/D91139 I suggested explicitly disabling use of __int128_t in libc++ due to this (as it can't be used as-is), but it was decided the way forward was to provide those helpers, somehow, e.g. the comment https://reviews.llvm.org/D91139#2429595. But there hasn't been any other specific progress on that front since, and the clang-cl CI configurations build with _LIBCPP_HAS_NO_INT128 explicitly defined: https://github.com/llvm/llvm-project/blob/d271fc04d5b97b12e6b797c6067d3c96a8d7470e/libcxx/utils/ci/run-buildbot#L99-L113
| libcxx/test/support/test_macros.h | ||
|---|---|---|
| 369 | Do we support any compilers that predefine both __SIZEOF_INT128__ and _MSC_VER, and yet don't support __int128? I bet the answer is "no." If the answer is "no," then you should remove || defined(TEST_COMPILER_MSVC) from this line, leaving only #if defined(_LIBCPP_HAS_NO_INT128) || defined(_MSVC_STL_VERSION) which sounds about right to me: it says "If this is libc++ with no int128, or if this is MSVC STL which never has int128, then..." What C++ compiler we're using shouldn't matter at all, because we should just trust the library (libc++ or MSVC STL) to relay accurately the situation re int128 support. I notice that we're implicitly assuming that libstdc++ always supports int128, which I'm sure is technically wrong; but we can leave that to the libstdc++ people to sort out. :) Someone +1 my logic and then I'll approve this. ;) | |
| libcxx/test/support/test_macros.h | ||
|---|---|---|
| 369 | Yes, I agree this falls out of the definition of _LIBCPP_HAS_NO_INT128. Fixing. | |
Do we support any compilers that predefine both __SIZEOF_INT128__ and _MSC_VER, and yet don't support __int128? I bet the answer is "no." If the answer is "no," then you should remove || defined(TEST_COMPILER_MSVC) from this line, leaving only
which sounds about right to me: it says "If this is libc++ with no int128, or if this is MSVC STL which never has int128, then..." What C++ compiler we're using shouldn't matter at all, because we should just trust the library (libc++ or MSVC STL) to relay accurately the situation re int128 support.
I notice that we're implicitly assuming that libstdc++ always supports int128, which I'm sure is technically wrong; but we can leave that to the libstdc++ people to sort out. :)
Someone +1 my logic and then I'll approve this. ;)