Re: [lmi] MinGW-w64 anomaly?

[Greg Chicares]

On 2016-12-25 16:10, Vadim Zeitlin wrote:
> On Sun, 25 Dec 2016 12:17:26 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2016-12-21 14:23, Vadim Zeitlin wrote:

[...x86_64: 'round_to_test.cpp' compiles, but 126 tests fail now...]

> GC> > [...] The change in the commit
> GC> > 54d250300e367b33af5719c40087e5536640fa1f which really broke it was to
> GC> > remove
> GC> > 
> GC> >         template<typename RealType>
> GC> >         inline RealType perform_rint(RealType r)
> GC> >         {
> GC> >             __asm__ ("frndint" : "=t" (r) : "0" (r));
> GC> >             return r;
> GC> >         }
> GC> > 
> GC> > and replace the calls to it with std::rint(). The old version worked
> GC> > because it used x87 instruction which was governed by the x87 rounding
> GC> > mode. The new version doesn't work because it uses the SSE instruction
> GC> > (some _mm_cvtsd_xxx I think), but the rounding mode is not set for SSE
> GC> > (this would require modifying MXCSR register instead of the x87 control
> GC> > word).
> GC> 
> GC> Now I'm confused. There's some sort of breakage here--with all x86_64
> GC> builds, IIRC. But what is actually broken? Is it that lmi fails to
> GC> compile? Or that some unit-test program fails to compile? Or only that
> GC> some unit test that formerly passed now doesn't?
> 
>  It's the latter, as I wrote before:
> 
>       round_to_test.cpp compiles just fine, but fails with 126 errors
>       under 64 bit Linux, which were not present before
> 
> (see http://lists.nongnu.org/archive/html/lmi/2016-12/msg00053.html)

Thanks, now I understand.

> GC> It sounds like it's the last of those--in which case maybe there is
> GC> no real problem. I.e., if we have some code that's supposed to behave
> GC> in a certain way when the rounding direction is set (in the x87 control
> GC> word or in some MMX register, as appropriate), and we test that code by
> GC> manipulating the x87 control word only, then the test is invalid for
> GC> non-87 code, right?

Now, AIUI, we have

(1) an implementation of round_to<>() in 'round_to.hpp', which formerly
    used its own cover function for rint(), which was implemented as the
    x87 FRNDINT instruction for x86_64...and that was replaced by calling
    std::rint() directly, which uses SSE instructions for x86_64; and

(2) a unit-test suite for round_to<>() in 'round_to_test.cpp', which
    sets the rounding direction with the x87 FSTCW instruction only,
    for i386 as well as for x86_64.

>  Well, yes, but we still want (all) test(s) to pass, ideally. Of course, we
> could just disable the test instead of fixing it and knowing (now, I didn't
> know this when I wrote the original bug report) that we don't actually ever
> change the rounding mode in the main program, maybe it's acceptable. But I
> think it would still be better to fix the code changing the rounding mode
> as previously discussed and let the test continue to pass.

In and of itself, (1) was a pure improvement, which I would prefer not
to revert. But it requires a change to (2) that has not been made, and
today we may not even be certain what that change should eventually be.
For the time being, does the following patch work on x86_64, and if so,
should I apply it?

--------8<--------8<--------8<--------8<--------8<--------8<--------8<--------
diff --git a/round_to_test.cpp b/round_to_test.cpp
index e013709..a4e0d4a 100644
--- a/round_to_test.cpp
+++ b/round_to_test.cpp
@@ -33,6 +33,13 @@
 #include <ostream>
 
 #if defined LMI_IEC_559
+    // The 'LMI_IEC_559' macro is never defined by any source file or
+    // by any makefile, so this conditional block is not reached today.
+    // For the macro's original rationale, see:
+    //   http://lists.nongnu.org/archive/html/lmi/2008-06/msg00034.html
+    // The description that follows is outdated anyway, because C++11
+    // now provides <cfenv>.
+    //
     // In case the C++ compiler offers C99 fesetround(), assume that
     // it defines __STDC_IEC_559__, but doesn't support
     //   #pragma STDC FENV_ACCESS ON
@@ -44,7 +51,24 @@
         ,fe_upward     = FE_UPWARD
         ,fe_towardzero = FE_TOWARDZERO
         };
+#elif defined LMI_X86_64
+    // Probably this conditional should distinguish SSE from x87,
+    // rather than 64- from 32-bit x64. For the nonce, this prevents
+    // certain unit-test failures noted here:
+    //   http://lists.nongnu.org/archive/html/lmi/2016-12/msg00053.html
+#   include <cfenv>
+    // If defining this enum here is an error, then remove it:
+    enum e_ieee754_rounding
+        {fe_tonearest  = FE_TONEAREST
+        ,fe_downward   = FE_DOWNWARD
+        ,fe_upward     = FE_UPWARD
+        ,fe_towardzero = FE_TOWARDZERO
+        };
 #elif defined LMI_X86
+    // It seems at first that the conditional here should be LMI_X86_32
+    // because x86_64 is treated above. However, perhaps the actual
+    // distinction is between SSE above and x87 here.
+    //
     // "fenv_lmi_x86.hpp" provides the necessary values.
 #else  // No known way to set rounding style.
 #   error No known way to set rounding style.
@@ -102,6 +126,11 @@ void set_hardware_rounding_mode(e_ieee754_rounding mode, 
bool synchronize)
 {
 #if defined LMI_IEC_559
     fesetround(mode);
+#elif defined LMI_X86_64
+    // See comments above on the <cfenv> series of conditionals.
+    // For the nonce, set both i87 and SSE rounding modes here.
+    fesetround(mode);
+    fenv_rounding(mode);
 #elif defined LMI_X86
     fenv_rounding(mode);
 #else // No known way to set hardware rounding mode.

-------->8-------->8-------->8-------->8-------->8-------->8-------->8--------

Of course, that's only a temporary fix, and I'm not even sure it works
for x86_64. (I should routinely build for GNU/Linux x86_64 here, but I
don't do that today, and can't set that up this vacation week.) It
raises the interesting question whether the x87 and SSE rounding flags
should ever be permitted to conflict; apparently the hardware makers
say "yes" (because valid use cases can be imagined), but perhaps we
should add "but not for lmi unit tests". If the patch above works for
x86_64, then probably we should set both sets of flags consistently
for i386 as well.