Page MenuHomePhabricator

[flang] Fix setting mxcsr on MSVC
Needs ReviewPublic

Authored by isuruf on Apr 9 2020, 11:25 AM.

Diff Detail

Event Timeline

isuruf created this revision.Apr 9 2020, 11:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2020, 11:25 AM
isuruf added a project: Restricted Project.
ro added a subscriber: ro.Apr 15 2020, 7:48 AM

I wonder if something like this patch should be applied more broadly or even in general. I'm seeing the same issue (lack of __mxcsr in fenv_t)
on Solaris/x86. When looking around, I found that that non-standard member does exist in glibc, macOS, and FreeBSD, so they could continue
to use the existing code.

However, AFAICS <xmmintrin.h> is available with all compilers that can be used to build llvm: clang, gcc, and msvc, so one could just remove
the references to __mxcsr and use the _mm_getcsr/_mm_setcsr code instead.

Thoughts?

I agree that we can use it for all platforms. @sscalpone, what do you think?

Yes, _mm_setcsr seems portable across x86_64 systems. It's fine. Is there an already-existing llvm API that is similar? (I don't know.)

isuruf updated this revision to Diff 257770.Apr 15 2020, 11:19 AM

Use _mm_getcsr/_mm_setcsr in all x86_64

isuruf updated this revision to Diff 257773.Apr 15 2020, 11:21 AM

Remove MSVC mentions

This change doesn't preserve the meaning of currentFenv_. I don't know if currentFenv_ (or originalFenv_) are still useful.

Thanks @sscalpone for the review. Looks like originalFenv_ is still used for restoring the original state. Shall I change the type of originalFenv_ to be unsigned int on x86_64? (Or maybe a union?)
From what I understand originalFenv_ is used for changing and restoring the __mxcsr value (or its equivalents on aarch64).

Looks like originalFenv_ is still used for restoring the original state. Shall I change the type of originalFenv_ to be unsigned int on x86_64? (Or maybe a union?)
From what I understand originalFenv_ is used for changing and restoring the __mxcsr value (or its equivalents on aarch64).

In host.cpp, originalFenv_ is more than just the __mxcsr, it also backs-up the whole fp environment (original rounding mode, exceptions that were raised,...) before calling the run-time to fold. Interrupts are also disabled between the related feholdexcept / fesetenv (we do not want the compiler to crash if the Fortran code being folded is wrong and triggers a division by zero or such), so it should be kept.
currentFenv_ however was only about manipulating the __mxcsr (or __fpcr /...), so you can remove it where you do not need it anymore.

ro added a comment.Apr 16 2020, 3:14 AM

I just tried a 2-stage build on amd64-pc-solaris2.11, which failed:

FAILED: tools/flang/unittests/Evaluate/CMakeFiles/FortranEvaluateTesting.dir/fp-testing.cpp.o
[...]
In file included from /vol/llvm/src/llvm-project/local/flang/unittests/Evaluate/fp-testing.cpp:1:
/vol/llvm/src/llvm-project/local/flang/unittests/Evaluate/fp-testing.h:22:10: error: private field 'currentFenv_' is not used [-Werror,-Wunused-private-field]
  fenv_t currentFenv_;
         ^
1 error generated.
isuruf updated this revision to Diff 258037.Apr 16 2020, 6:28 AM

Add a new field originalMxcsr and restore it along with originalFenv

isuruf updated this revision to Diff 258038.Apr 16 2020, 6:34 AM

Refactor

DavidTruby resigned from this revision.Apr 22 2020, 6:45 AM
ro added a comment.May 5 2020, 1:49 AM

Given that this fixes the flang build on at least Windows and Solaris/x86, it would be good if someone could review this.

FWIW, I`ve successfully built and tested with this patch on amd64-pc-solaris2.11