Re: [lmi] [PATCH] Better support for non-x87 platforms

[Vadim Zeitlin]

On Wed, 4 Jan 2017 13:45:47 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2017-01-03 15:14, Vadim Zeitlin wrote:
GC> > 
GC> >  As promised, here are the changes which allow to set rounding mode when
GC> > not using x87 and so make the "round" and "round_to" tests pass in 64 bit
GC> > builds: https://github.com/vadz/lmi/pull/49
GC> > 
GC> >  I left a few comments there concerning some perhaps slightly less obvious
GC> > points, but globally the changes are pretty straightforward: we keep using
GC> > the same code as now, with as few changes as possible, when using x87 and
GC> > use the standard C++11 functions otherwise. I've checked that all tests 
now
GC> > pass under Linux in x86-64 build and that they still pass in the standard
GC> > 32 bit MinGW build. I also tested the interactive floating point
GC> > environment test in the GUI in the latter (it wouldn't do anything, and so
GC> > doesn't even exist, in the former).
GC> 
GC> But doesn't the menuitem still exist, because 'menus.xrc' is unchanged?

 Oops, yes, you're right, of course, it does.

GC> It still specifies:
GC>     <object class="wxMenuItem" name="test_floating_point_environment">
GC>         <label>Test _floating point environment</label>
GC>     </object>
GC> and what is the effect of selecting that menuitem in an x86_64 build?

 Nothing happens, which is not very user-friendly.

 But removing this item would require more code, as it can't be done at XRC
level (at least without major contortions), so maybe it would be better to
finally keep the event handler and just display "Floating point environment
tests not relevant for this platform" in the status bar? Or perhaps just
move this test into the unattended GUI test (and guard it with LMI_X87
there)? I know we planned to do it for all the "Test" menu items, but we
could do it one by one instead of doing it for all of them at once,
starting by this one, what do you think?

GC> I was at first surprised that you don't use __STDC_IEC_559__ anywhere,
GC> but apparently that macro is for C only and is neither mentioned in
GC> the C++ standard nor incorporated therein by reference:
GC>   
http://stackoverflow.com/questions/23802294/why-does-the-c-standard-not-mention-stdc-iec-559
GC> so I'll expunge all mention of it and of the FENV_ACCESS pragma.

 Yes, I didn't see any use in testing for this. We use only C++11
facilities. As for FENV_ACCESS pragma... I don't even know what to say,
things are really bad here, I had no idea there were such glaring
omissions/inconsistencies in the C++ ISO standard. But, as usual,
everything concerning floating point is more complicated than it seems. My
understanding is that, in theory, using any function defined in <cfenv>
without FENV_ACCESS pragma is undefined behaviour and, at the same time,
using this pragma in C++ code is undefined behaviour on its own. In
practice things are slightly better even though still very far from ideal:
using <cfenv> works, but requires careful use of "volatile" to prevent
unwanted optimizations, while the pragma is not supported by any of the
major compilers anyhow.


GC> Let me address some of the comments at
GC>   https://github.com/vadz/lmi/pull/49
GC> here:
GC> 
GC> * fenv_lmi.cpp
GC> | Dropping support for Borland could simplify the preprocessor checks
GC> | in this file a bit, and I think it should be done, but I decided
GC> | against doing it in this patch to keep the changes well separate.
GC> 
GC> Given this:
GC>   https://en.wikipedia.org/wiki/C%2B%2BBuilder#10_Seattle
GC>   "updates the C++ compiler suite to CLANG 3.3"
GC> it's safe to assume we'll never support any borland compiler again

 Thanks, but what exactly does this mean in practice? Should I remove these
particular __BORLANDC__ checks right now, i.e. update the PR with a commit
doing it, or leave them be and then remove all of the remaining ones at
once in a separate commit?


GC> | The only real change to the existing code is the addition of an
GC> | extra \n here. It simplifies the code of this function slightly
GC> | and I think an extra blank line makes the text of the warning more
GC> | readable, but please let me know if you disagree.
GC> 
GC> Actually, I found this the toughest section to review: the other changes
GC> are mostly mechanical, but this one is more complicated. At first, it's
GC> pretty shocking that fenv_is_valid(), formerly a lightweight function
GC> that simply compared two 16-bit words, now constructs a std::string and
GC> passes it to a function that constructs a std::ostringstream.

 I admit I didn't bother benchmarking this, but I have a lot of trouble
seeing how could creating 2 default-initialized objects have a noticeable
performance impact.

GC> I suppose you're doing that to avoid repeating the tests that <cfenv>
GC> requires,

 Yes, especially as it's not unlikely that they could change later, e.g. if
C++20 adds support for FP precision.

GC> though we could just as well test this in fenv_is_valid():
GC>   fenv_rounding() != fe_tonearest || 0 != fetestexcept(FE_ALL_EXCEPT)
GC> and call the reporting function if that fails. But what really bothers
GC> me is that we have to go to such trouble to test so few bits. I thought
GC> of doing something like this:
GC> 
GC>     // Set the rounding bits and interrupt mask the way we want,
GC>     // then save them somewhere:
GC>     std::fenv_t desired_settings;
GC>     std::fegetenv(&desired_settings);
GC>     // and later compare the whole environment:
GC>     std::fenv_t current_settings;
GC>     std::fegetenv(&current_settings);
GC>     assert(desired_settings == current_settings);
GC> 
GC> but type fenv_t is so opaque that it's not even equality-comparable.

 Not only this, but I think the current error messages are potentially more
useful than just saying that the fenv_t objects differ -- without being
able to say anything about how do they differ exactly.

GC> [...defining fenv_precision() iff LMI_X87 is defined...]
GC> 
GC> | Another alternative could be to provide these functions but always
GC> | return the fixed fe_dblprec value from the accessor and throw from
GC> | the setter.
GC> 
GC> I suspect that alternative is preferable. Otherwise, we have to write
GC> LMI_X87 conditionals each time these functions are called. But first
GC> I'd better look into 'math_functors_test.cpp', because I'm not sure
GC> how this code:
GC> 
GC> void sample_results()
GC> {
GC>     fenv_initialize();
GC>     fenv_precision(fe_ldblprec);
GC> 
GC> can even compile with x86_64.

 You're right again, it doesn't, I've somehow forgot to update it even
though I did find it and thought about doing it. Moreover, this is, in
fact, the example which more or less made me choose the current approach
because otherwise, i.e. if the call to fenv_precision(fe_ldblprec) threw,
this code would continue to compile, but stop working, which would, IMHO,
be worse. And while currently we do need to add "#ifdef LMI_X87" check
here (should I do this or are you already working with this patch and
prefer to do it yourself?), we would have to add a try/catch with the
alternative approach and this would be even worse, I think.

GC> * skeleton.cpp
GC> 
GC> | +#if defined LMI_X87
GC>    void Skeleton::UponTestFloatingPointEnvironment(wxCommandEvent&)
GC> | It didn't seem useful to neither keep a test doing nothing nor
GC> | creating a (necessarily) completely different and artificial
GC> | version of this test for non-x87.
GC> 
GC> I think we had concluded that we should keep Skeleton::UponTimer(),
GC> and make it test for changes in the SSE as well as the x87 environment.

 But we did keep UponTimer() unchanged and it does test for the changes to
the SSE environment when LMI_X87 is not defined because it uses
fenv_is_valid() which does this now. UponTestFloatingPointEnvironment() is
something quite different.


 To summarize, there are several changes that [may] need to be done:

1. The "Test floating point environment" menu item needs to be removed or
   the handler for it reestablished or this test should be moved into the
   GUI test completely.

2. fenv_is_valid() could be optimized if we want to avoid constructing
   std::string/ostringstream unnecessarily.

3. __BORLANDC__ checks need to be removed -- but it's not fully clear if
   it should be done now or later.

4. Compilation of math_functors_test in 64 bits needs to be fixed, which
   could be done either by making fenv_precision() always available (but
   this would still require modifying math_functors_test.cpp, just in a
   different way) or by just adding a preprocessor check.


 I'd like to fix all of these items immediately (i.e. today), but I'm not
sure what are your preferences for (1) and (4) and whether my changes
aren't going to conflict with yours. Please let me know which of these
fixes, if any, should I add to the PR.

 Thanks in advance,
VZ