Skip to content

Conversation

@smanna12
Copy link

@smanna12 smanna12 commented Jul 22, 2021

The previous codes only work for complex header file by PR's:
#1316
#1728.

The _Bit_cast function in xutility header file does not allow 80 bits wide long double. The Intel C++ compiler uses /Qlong-double option to set the long double type to 80 bits wide.

This patch generalizes the 80 bit long double check in xutility header file using same logic from #1316.

Copy link
Contributor

@celonymire celonymire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably needs tests.

@smanna12 smanna12 changed the title Accept the 80-bit long double in <xutility> for Clang Accept the 80-bit long double in <xutility> Jul 26, 2021
smanna12 added 2 commits July 26, 2021 12:19
Signed-off-by: Soumi Manna <soumi.manna@intel.com>
@smanna12
Copy link
Author

smanna12 commented Jul 26, 2021

This probably needs tests.

Thanks @sam20908 for looking into the patch and the reviews.
The fix is same as #1316 to generalize the 80 bit long double check in xutility header file.
I do not see any test there, so no tests have been added in this patch.

@smanna12 smanna12 requested a review from CaseyCarter July 26, 2021 20:07
@smanna12 smanna12 marked this pull request as ready for review July 26, 2021 20:08
@smanna12 smanna12 requested a review from a team as a code owner July 26, 2021 20:08
Copy link
Contributor

@celonymire celonymire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just spotted one weird thing :)

@CaseyCarter CaseyCarter added the enhancement Something can be improved label Jul 26, 2021
@smanna12 smanna12 requested a review from CaseyCarter July 27, 2021 18:46
template <class _Ty, enable_if_t<is_floating_point_v<_Ty>, int> = 0>
_NODISCARD _CONSTEXPR_BIT_CAST bool _Is_finite(const _Ty _Xx) { // constexpr isfinite()
#if defined(__LDBL_DIG__) && __LDBL_DIG__ == 18
return _CSTD _LDtest(&_Xx) == _FINITE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe that _LDtest could properly handle 80-bit long double:

STL/stl/src/xldtest.cpp

Lines 10 to 12 in bd7adb4

_CRTIMP2_PURE short __CLRCALL_PURE_OR_CDECL _LDtest(long double* px) { // categorize *px -- 64-bit
return _Dtest(reinterpret_cast<double*>(px));
}

Copy link
Author

@smanna12 smanna12 Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_LDtest is handling long double:

STL/stl/inc/complex

Lines 542 to 557 in bd7adb4

static bool _Isinf(_Ty _Left) { // test for infinity
#if defined(__LDBL_DIG__) && __LDBL_DIG__ == 18
return _CSTD _LDtest(&_Left) == _INFCODE;
#else // ^^^ 80-bit long double (not supported by MSVC in general, see GH-1316) / 64-bit long double vvv
const auto _Uint = _Bit_cast<uint64_t>(_Left);
return (_Uint & 0x7fffffffffffffffU) == 0x7ff0000000000000U;
#endif // ^^^ 64-bit long double ^^^
}
static _CONSTEXPR20 bool _Isnan(_Ty _Left) {
#if defined(__LDBL_DIG__) && __LDBL_DIG__ == 18
return _CSTD _LDtest(&_Left) == _NANCODE;
#else // ^^^ 80-bit long double (not supported by MSVC in general, see GH-1316) / 64-bit long double vvv
const auto _Uint = _Bit_cast<uint64_t>(_Left);
return (_Uint & 0x7fffffffffffffffU) > 0x7ff0000000000000U;
#endif // ^^^ 64-bit long double ^^^

Copy link
Author

@smanna12 smanna12 Jul 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I work with Intel C++ compiler team. The Intel C++ compiler uses /Qlong-double option to set the long double type to 80 bits wide. The _Bit_cast function in <xutility> header file does not allow 80 bits wide long double.

Intel C++ Compiler with /Qlong-double enabled, produces the following compilation error messages when we compile <complex> header file with /std:c++latest option:

In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\iostream:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\istream:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\ostream:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\ios:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\xlocnum:12:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\cmath:26:
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\xutility(5918,24): error: no matching function for call to '_Bit_cast'
    const auto _Bits = _Bit_cast<_Uint_type>(_Xx);
                       ^~~~~~~~~~~~~~~~~~~~~
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\xutility(5967,12): note: in instantiation of function template specialization 'std::_Float_abs_bits<long double, 0>'
      requested here
    return _Float_abs_bits(_Xx) < _Traits::_Shifted_exponent_mask;
           ^
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\cmath(1307,31): note: in instantiation of function template specialization 'std::_Is_finite<long double, 0>' requested here
    const bool _T_is_finite = _Is_finite(_ArgT);
                              ^
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\cmath(1392,12): note: in instantiation of function template specialization 'std::_Common_lerp<long double>' requested here
    return _Common_lerp(_ArgA, _ArgB, _ArgT);
           ^
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\xutility(67,36): note: candidate template ignored: requirement 'conjunction_v<std::integral_constant<bool, false>,
      std::is_trivially_copyable<unsigned long long>, std::is_trivially_copyable<long double>>' was not satisfied [with _To = unsigned long long, _From = long double]
_NODISCARD _CONSTEXPR_BIT_CAST _To _Bit_cast(const _From& _Val) noexcept {
                                   ^

In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\iostream:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\istream:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\ostream:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\ios:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\xlocnum:12:
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\cmath(1308,25): error: no matching function for call to '_Is_finite'
    if (_T_is_finite && _Is_finite(_ArgA) && _Is_finite(_ArgB)) {
                        ^~~~~~~~~~
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\cmath(1392,12): note: in instantiation of function template specialization 'std::_Common_lerp<long double>' requested here
    return _Common_lerp(_ArgA, _ArgB, _ArgT);
           ^
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\xutility(5965,37): note: candidate template ignored: substitution failure [with _Ty = long double, $1 = 0]
_NODISCARD _CONSTEXPR_BIT_CAST bool _Is_finite(const _Ty _Xx) { // constexpr isfinite()
                                    ^

In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\iostream:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\istream:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\ostream:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\ios:11:
In file included from c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\xlocnum:12:
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\cmath(1308,46): error: no matching function for call to '_Is_finite'
    if (_T_is_finite && _Is_finite(_ArgA) && _Is_finite(_ArgB)) {
                                             ^~~~~~~~~~
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\xutility(5965,37): note: candidate template ignored: substitution failure [with _Ty = long double, $1 = 0]
_NODISCARD _CONSTEXPR_BIT_CAST bool _Is_finite(const _Ty _Xx) { // constexpr isfinite()
                                    ^
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\xutility(5941,12): error: no matching function for call to '_Float_abs_bits'
    return _Float_abs_bits(_Xx) > _Traits::_Shifted_exponent_mask;
           ^~~~~~~~~~~~~~~
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\cmath(1337,13): note: in instantiation of function template specialization 'std::_Is_nan<long double, 0>' requested here
        if (_Is_nan(_ArgA)) {
            ^
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\cmath(1392,12): note: in instantiation of function template specialization 'std::_Common_lerp<long double>' requested here
    return _Common_lerp(_ArgA, _ArgB, _ArgT);
           ^
c:/Program files/Microsoft Visual Studio/2022/Preview/VC/Tools/MSVC/14.29.30130/include\xutility(5915,37): note: candidate template ignored: substitution failure [with _Ty = long double, $1 = 0]
_NODISCARD _CONSTEXPR_BIT_CAST auto _Float_abs_bits(const _Ty& _Xx) {
                                  ^
4 errors generated

I think the problem is that _Bit_cast is unable to work on a long double (probably because sizeof(long double) > sizeof(unsigned long long)?).

I am not sure whether _Bit_cast needs to be fixed, or whether we should be avoiding trying to call it in the first place (generalizes the 80 bit long double check in xutility header file using same logic #1316)

It would be very helpful if we could get any input or assistance in figuring out the right path forward to fix the problem.
Tagging @CaseyCarter, @statementreply, @StephanTLavavej, @sam20908, @simmse for any suggestion. Thank you.

Adding @AaronBallman, @erichkeane for any more information.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there's a separate correctness issue here, because this codepath was being used for 32-bit float as well, and that won't be properly handled by _LDtest (regardless of whether it's the original 64-bit implementation or a replaced 80-bit implementation).

Signed-off-by: Soumi Manna <soumi.manna@intel.com>
@StephanTLavavej
Copy link
Member

The maintainer team talked about this at our weekly meeting, and we believe that this falls under our project's Non-Goals; 80-bit long double is essentially a different platform (it is certainly ABI-incompatible with MSVC), and as a result we can't devote any significant time towards maintaining it. In a couple of previous PRs, we were willing to make targeted changes to restore old code that was being changed, and to generalize the check for 80-bit long double, as this was quick to review and non-destabilizing. However, we can't keep adding machinery for 80-bit long double endlessly; that's essentially providing support which we have never claimed.

As always, you're welcome to maintain a fork of our repo and add support for 80-bit long double there; our concerns are simply with the resources needed to upstream such support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Something can be improved

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants