Re: [lmi] New clang errors fixes

[Greg Chicares]

On 2017-09-27 11:55, Vadim Zeitlin wrote:
> On Wed, 27 Sep 2017 00:04:32 +0000 Greg Chicares <address@hidden> wrote:
> 
> GC> On 2017-09-26 21:10, Vadim Zeitlin wrote:
> GC> > On Tue, 26 Sep 2017 17:02:58 +0000 Greg Chicares <address@hidden> wrote:
> GC> 
> GC> [...bourne_cast.hpp rejected by clang...]
> GC> 
> GC> > GC> If it can't be static and it can't be constexpr, then is this:
> GC> > GC> +    From const limit = std::ldexp(From(1), to_traits::digits);
> GC> > GC> the best we can do to express the intention and help the compiler 
> optimize?
> GC> > 
> GC> >  I don't know if it's the best, but I wonder if this is really 
> noticeably
> GC> > worse than the best. I might be forgetting this as well, but have you
> GC> > already run the benchmarks for this variant and did it show that it's
> GC> > really worth it to optimize this call to ldexp() away?
> GC> 
> GC> Benchmarks using i686-w64-mingw32-gcc version 6.3.0 :

[...they suggest that there's no difference among the several candidate
implementations considered...]

>  So, seeing that there is no advantage whatsoever in using "constexpr" from
> performance point of view, why do we still need lmi::ldexp()? Wouldn't the
> simplest, and hence the best, change be to just replace "constexpr" with
> "const" and keep using std::ldexp()? I feel like I'm missing something here
> because I just don't understand how did the benchmark results above lead to
> the subsequent commits...
They didn't. The '(gdb) disassemble' output motivated the last commit:
it showed that the iterative constexpr lmi::ldexp() had ideal performance.
Given that strong proof of optimality, the benchmarks (which I ran later
anyway) didn't affect this choice. When demonstrable perfection lies in
the palm of your hand, why not just close your fingers and grasp it?

More subjectively, while I sympathize with the Committee in their struggle
to balance performance with C99 consistency (setting 'errno' or whatever),
in this situation a constexpr ldexp() is obviously safe and desirable.
It's reasonable to want to calculate a value such as 2^7 as a compile-time
constant; they've decided that std::ldexp() is not the way to do that; so
it's natural to fill the gap they chose to leave in the standard library.

OTOH, for all I know, gcc and other compilers, now and forever, might
optimize the 's/constexpr/const/' alternative just as well, so that
introducing the complexity of lmi::ldexp() gains only the theoretical
advantage of a zero-runtime performance guarantee, yet achieves no
actual practical benefit...and any gain is less than we can measure
anyway, at least for this particular version of gcc. If there's no
measurable difference in performance, then favoring simplicity is a
pretty good idea.

I'm going to change it the way you suggest, not just because it makes
the code simpler, but also because it doesn't require me to go back and
read (or, worse, revise) the documentation I wrote. I seem to say that
 - the argument to ldexp() is always within proper bounds, and
 - if it's not within proper bounds, it does the right thing
but if those two statements are consistent then one is redundant--yet
today I can't say which, so reimplementing ldexp() may have introduced
a particle of uncertainty that I can remove by calling std::ldexp().

BTW, I thought of a just replacing this with simple inline code like:
-    constexpr From limit = lmi::ldexp(From(1), to_traits::digits);
+    constexpr From limit = From(1ULL << to_traits::digits);
but:

/opt/lmi/src/lmi/bourn_cast.hpp:197:38: error: left shift count >= width of 
type [-Werror=shift-count-overflow]
     constexpr From limit = From(1ULL << to_traits::digits);
                                 ~~~~~^~~~~~~~~~~~
/opt/lmi/src/lmi/bourn_cast.hpp:197:38: error: right operand of shift 
expression ‘(1ull << 64)’ is >= than the precision of the left operand 
[-fpermissive]