Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accept 80-bit long double in <complex> #1316

Merged
merged 2 commits into from
Sep 25, 2020

Conversation

simmse
Copy link
Contributor

@simmse simmse commented Sep 24, 2020

The new _Bit_cast function does not allow 80 bits wide long double. The Intel C++ compiler has an option to set the long double type to 80 bits wide (e.g. /Qlong-double). Coordinating with S.T. Lavavej, this modification uses the pre-processor when compiling with Intel C++, to revert the _Ctraits template class' _Isnan and _Isinf function bodies. The single _LDtest line will be used, as in the previous complex include file implementation. If the new pre-processor logic is activated, the function bodies use the _LDtest function and compares the output to either _NANCODE or _INFCODE respectively. The pre-processor logic is such that three conditions have to be met to use the _LDtest function:

  1. The __INTEL_COMPILER macro has to be defined
  2. The __LONG_DOUBLE_SIZE__ macro has to be defined
  3. The __LONG_DOUBLE_SIZE__ has to equal 80

The above two macros are defined when using the Intel C++ Compiler.

Without the modification, the Intel C++ Compiler with /Qlong-double enabled, produces the following output messages from a single declaration of a complex variable, using the 80 bit long double (e.g. complex<long double> sum;)

1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\include\complex(226): error : no instance of function template "std::_Bit_cast" matches the argument list
1> argument types are: (long double)
1> const auto _Uint = _Bit_cast<uint64_t>(_Left);

1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\include\complex(235): error : no instance of function template "std::_Bit_cast" matches the argument list
1> argument types are: (long double)
1> const auto _Uint = _Bit_cast<uint64_t>(_Left);

…he Intel C++ compiler has an option to set the long double type to 80 bits wide (e.g. /Qlong-double). Coordinating with S.T. Lavavej, this modification uses the pre-processor when compiling with Intel C++, to revert the _Ctraits<long double> template class' _Isnan and _Isinf function bodies, to the single _LDtest line used in the previous complex include file implementation. If the new pre-processor logic is activated, the function bodies use the _LDtest function and compares the output to either _NANCODE or _INFCODE respectively. The pre-processor logic is such that three conditions have to be met to use the _LDtest function:

1) The __INTEL_COMPILER macro has to be defined
2) The __LONG_DOUBLE_SIZE__ macro has to be defined
3) The __LONG_DOUBLE_SIZE__ has to equal 80

The above two macros are defined when using the Intel C++ Compiler.

Without the modification, the Intel C++ Compiler with /Qlong-double enabled, produces the following output messages from a single declaration of a complex variable using the 80 bit long double (e.g. complex<long double> sum;)

1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\include\complex(205): error : no instance of function template "std::_Bit_cast" matches the argument list
1>            argument types are: (long double)
1>          const auto _Uint = _Bit_cast<uint64_t>(_Left);

1>C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.27.29110\include\complex(214): error : no instance of function template "std::_Bit_cast" matches the argument list
1>            argument types are: (long double)
1>          const auto _Uint = _Bit_cast<uint64_t>(_Left);
@simmse simmse requested a review from a team as a code owner September 24, 2020 15:14
@AlexGuteniev
Copy link
Contributor

Is it supposed to be supported in the whole STL implementation?

@simmse
Copy link
Contributor Author

simmse commented Sep 24, 2020

Is it supposed to be supported in the whole STL implementation?

Is the question whether 80 bits wide long double should be supported in all of the STL implementation? If so, for the Intel C++ compiler, using the supplied STL from Visual Studio, the short answer is yes. The modification successfully compiles a Visual Studio production solution with Intel C++, 19.1, Update two (also known as Intel Parallel Studio XE 2020 Update 2) using the /Qlong-double option. Note that my employer has commercially used the Intel C++ compiler and the /Qlong-double option since 2003.

When debug stepping into std::isnan or std::isinf, the _fpclassify function is used (e.g. internal to the STL). The _fpclassify body currently uses the _LDtest function. Since _LDtest correctly operates on 80 bits wide long double and compares to the existing macros representing the floating point state (e.g. nan, infinite, normal, sub-normal, zero), Intel C++ projects continue to compile and run without modifying the production code base.

@AlexGuteniev
Copy link
Contributor

I know it could be problematic in different area. atomic<long double> would have padding bits...

@BillyONeal
Copy link
Member

Is the question whether 80 bits wide long double should be supported in all of the STL implementation? If so, for the Intel C++ compiler, using the supplied STL from Visual Studio, the short answer is yes.

The short answer is no. We do not test with, nor design for, 80 bit long double. There are parts that might happen to work but the assumption that double == long double is pervasive.

@simmse
Copy link
Contributor Author

simmse commented Sep 24, 2020

I know it could be problematic in different area. atomic would have padding bits...

Note that I am not employed by Intel. It is not known how the Intel C++ team verifies functionality. That being said, if there is a problem with the atomic<long double>, it has not been found with the Intel C++, 19.1, Update 2 compiler in a production solution. The Intel C++ compiler integrates with Visual Studio and uses the files supplied from installing/updating Visual Studio.

@BillyONeal
Copy link
Member

To clarify: The Intel compiler supports /Qlong-double, Visual C++, and by extension, our standard library implementation, does not. (In fact we don't technically support ICC at all though it tends to work)

@StephanTLavavej StephanTLavavej added the bug Something isn't working label Sep 24, 2020
@StephanTLavavej
Copy link
Member

I met with @simmse earlier this week - he explained his company's reason for using 80-bit long double, and I explained MSVC's level of support for ICC (i.e. it is neither supported nor tested by Microsoft; unofficially, when asked, we'll consider making minimal changes to avoid breaking it egregiously for trivial reasons). I also explained why MSVC cannot support 80-bit long double in general.

Since this was triggered by recent changes to <complex>, and because the workaround is extremely targeted, doesn't affect other compilers, and simply conditionally restores the code we were previously shipping, @MahmoudGSaleh and I agree that accepting this PR is reasonable.

To summarize:

  • This doesn't change our position regarding ICC support.
  • 80-bit long double remains especially unsupported by MSVC; if we break it again, we might not fix it again, and we certainly won't extend changes to things that never worked.
  • The "vNext" ABI-breaking release may eradicate the _LDtest support functions, at which point we may need to revisit this - if the workaround is complicated, we might not preserve it.

For a similar workaround, note that we have:

STL/stl/inc/yvals_core.h

Lines 377 to 378 in 7210dd7

#elif defined(__CUDACC__) || defined(__INTEL_COMPILER)
#define _HAS_CONDITIONAL_EXPLICIT 0 // TRANSITION, CUDA/ICC

That workaround was added for CUDA, and activated for ICC because it was easy; at that time, we stated that when we can remove the workaround for CUDA, we will not retain it for ICC alone.

@BillyONeal
Copy link
Member

Since this was triggered by recent changes to <complex>, and because the workaround is extremely targeted, doesn't affect other compilers, and simply conditionally restores the code we were previously shipping, @MahmoudGSaleh and I agree that accepting this PR is reasonable.

Right, to clarify I've only been answering the general "does the whole STL support 80 bit long double" question, I'm not making any value judgements on this PR.

Copy link
Member

@StephanTLavavej StephanTLavavej left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. I pushed a couple of stylistic changes:

  • We conventionally don't parenthesize equality tests in preprocessor conditions (I am assuming that ICC's preprocessor will be fine with this), and
  • We conventionally comment #else and #endif directives when more than a single line each is controlled; I used these comments to clarify that this is a special exception and not comprehensive support for 80-bit long double.

@StephanTLavavej StephanTLavavej self-assigned this Sep 25, 2020
@simmse
Copy link
Contributor Author

simmse commented Sep 25, 2020

I have tested the commented modifications and the non parenthesized equality test with the Intel 19.1, Update Two compiler with and without the /Qlong-double, and the warning level at three and four. It compiles without warnings or errors. All combinations of Release, Debug, x86, and x64 successfully run a short console application using one complex <long double> variable too.

@StephanTLavavej StephanTLavavej merged commit c70b7a8 into microsoft:master Sep 25, 2020
@StephanTLavavej
Copy link
Member

Thanks again for the patch, and congratulations on your first microsoft/STL commit! 🎉 😺 As I mentioned, this change will flow into VS 2019 16.9 Preview 2; I'll begin the process of getting approval to port it to the VS 2019 16.8 Preview 4 branch (which is Microsoft-internal; nothing to do on GitHub).

CaseyCarter added a commit to CaseyCarter/STL that referenced this pull request Oct 8, 2020
c70b7a8 corresponds to microsoft#1316 "Accept 80-bit `long double` in `<complex>`," which was ported into a late 16.8 preview. This will almost certainly be our last commit for 16.8.

Not that not every preceding commit is in 16.8. This is fine: we need all shipping commits to be covered by the cgmanifest, it's ok to also cover some commits that won't ship until the next release.
CaseyCarter added a commit that referenced this pull request Oct 9, 2020
c70b7a8 corresponds to #1316 "Accept 80-bit `long double` in `<complex>`," which was ported into a late 16.8 preview. This will almost certainly be our last commit for 16.8.

Not that not every preceding commit is in 16.8. This is fine: we need all shipping commits to be covered by the cgmanifest, it's ok to also cover some commits that won't ship until the next release.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants