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

[Greg Chicares]

On 2017-01-05 13:03, Vadim Zeitlin wrote:
> On Thu, 5 Jan 2017 03:41:12 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> [See commit 571924bbc8f29c3002e28e4ee3f1c3f2c3269d22.]
> 
>  Sorry, but this still doesn't compile under non-x87 platforms because of
> the
> 
> #       error Unknown compiler--cannot detect SSE. Consider contributing 
> support.
> 
> line in config.hpp which is triggered in this case.

Thanks for reporting this defect.

>  Looking at the diff between my original changes and current master:
[...snip diff...]
> I'm not sure why was this changed. Moving these checks (much) further down
> doesn't look really good to me neither because IMHO it would make sense to
> define LMI_X87 as near as possible from the place where LMI_X86 is defined
> because both of them will likely need to be updated together if support for
> a new platform is added

Here's my rationale, in two parts:

(1) Some features can be detected by well-known and widely-used
macros, e.g.:
  // Reference: http://predef.sourceforge.net/prearch.html
  #if defined __x86_64 || defined __x86_64__ || defined __amd64 || defined 
__amd64__ || defined _M_X64
but there doesn't seem to be any such idiom for x87, so we need to
develop our own. For gcc, we have __SSE_MATH__, and you've documented
why that's preferable to __SSE__; but what if some other compiler
defines a macro with the same name but different semantics? The same
question arises for the msvc macros. I thought it would be safer to
test these macros only for the particular compilers that are known to
use them.

(BTW, is there any way to detect x87 with clang? Apparently we'd need
  http://stackoverflow.com/questions/22581350/clang-5-1-fpmath
| for Clang to accept -mfpmath=387, I had to also pass -mno-sse
to select it, but does that define a feature macro? and, even if
there's supposed to be a macro, is it correct in light of this:
  https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=632472
?)

(2) I too didn't like moving LMI_X87 away from LMI_X86. I did that
only to make it follow LMI_MSC, on which it depends, and for which
the inline documentation references this:

http://boost.cvs.sf.net/boost/boost/boost/config.hpp?annotate=1.1
//   Many other "compilers define _MSC_VER. Thus BOOST_MSVC."
...
//  Must remain the last #elif since some other vendors (Metrowerks, for
//  example) also #define _MSC_VER

I'm not necessarily adamant about (1), but it doesn't seem ideal
to let clang (e.g.) default silently to SSE.

Upon reconsideration, (2) isn't really very convincing. It's
probably good enough for this purpose just to test _MSC_VER (and
move the code up with LMI_X86). Counter-rationale: msvc has killed
every non-free compiler that would define _MSC_VER (metrowerks,
e.g., mentioned in the boost documentation above, seems to have
abandoned its x86 tools a decade ago); and free (as in software)
compilers probably never defined _MSC_VER.

> but the main problem is, of course, this "#error"
> line which, IMO, should be simply removed. Alternatively, if we really want
> to keep it, we should rewrite the conditions as
> 
>       #if defined __GNUC__
>               #if !defined __SSE_MATH__
>                       #define LMI_X87
>               #endif
>       #elif defined LMI_MSC
>               #if defined _M_IX86_FP && _M_IX86_FP == 0
>                       #define LMI_X87
>               #endif
>       #else
>               #error Unknown compiler.
>       #endif

Now I see. I had it written out that way, and then thought that
combining the main and subordinate conditionals wouldn't affect
the meaning...because I gave insufficient consideration to what
the "#else" referred to.

I just pushed a change that looks very much like your rewrite
above, and moves the LMI_X86 and LMI_X87 tests into propinquity.