Re: [lmi] New clang errors fixes

[Greg Chicares]

On 2017-09-26 21:10, Vadim Zeitlin wrote:
> On Tue, 26 Sep 2017 17:02:58 +0000 Greg Chicares <address@hidden> wrote:

[...bourne_cast.hpp rejected by clang...]

> GC> If it can't be static and it can't be constexpr, then is this:
> GC> +    From const limit = std::ldexp(From(1), to_traits::digits);
> GC> the best we can do to express the intention and help the compiler 
> optimize?
> 
>  I don't know if it's the best, but I wonder if this is really noticeably
> worse than the best. I might be forgetting this as well, but have you
> already run the benchmarks for this variant and did it show that it's
> really worth it to optimize this call to ldexp() away?

Benchmarks using i686-w64-mingw32-gcc version 6.3.0 :

HEAD as of this moment:
    constexpr From limit = std::ldexp(From(1), to_traits::digits);

  static_cast<U>(S): 3.156e-004 s mean;        313 us least of 100 runs
   bourn_cast<U>(S): 3.199e-004 s mean;        313 us least of 100 runs
   bourn_cast<S>(U): 3.232e-004 s mean;        313 us least of 100 runs

  static_cast<D>(U): 6.322e-004 s mean;        626 us least of 100 runs
   bourn_cast<D>(U): 1.916e-003 s mean;       1879 us least of 100 runs

  static_cast<U>(D): 2.513e-003 s mean;       2435 us least of 100 runs
   bourn_cast<U>(D): 1.346e-002 s mean;      12893 us least of  75 runs
   bourn_cast<S>(D): 6.764e-003 s mean;       6703 us least of 100 runs

  static_cast<F>(D): 1.346e-003 s mean;       1287 us least of 100 runs
   bourn_cast<F>(D): 5.104e-003 s mean;       4678 us least of 100 runs
   bourn_cast<D>(F): 4.668e-003 s mean;       4595 us least of 100 runs

Only const, not static, not constexpr:
    From const limit = std::ldexp(From(1), to_traits::digits);

  static_cast<U>(S): 3.140e-004 s mean;        313 us least of 100 runs
   bourn_cast<U>(S): 3.179e-004 s mean;        313 us least of 100 runs
   bourn_cast<S>(U): 3.173e-004 s mean;        313 us least of 100 runs

  static_cast<D>(U): 6.325e-004 s mean;        626 us least of 100 runs
   bourn_cast<D>(U): 1.942e-003 s mean;       1879 us least of 100 runs

  static_cast<U>(D): 2.507e-003 s mean;       2433 us least of 100 runs
   bourn_cast<U>(D): 1.321e-002 s mean;      12874 us least of  76 runs
   bourn_cast<S>(D): 6.471e-003 s mean;       6284 us least of 100 runs

  static_cast<F>(D): 1.220e-003 s mean;       1207 us least of 100 runs
   bourn_cast<F>(D): 4.873e-003 s mean;       4386 us least of 100 runs
   bourn_cast<D>(F): 4.662e-003 s mean;       4595 us least of 100 runs

Iterative constexpr ldexp() replacement:

  static_cast<U>(S): 3.138e-004 s mean;        313 us least of 100 runs
   bourn_cast<U>(S): 3.134e-004 s mean;        313 us least of 100 runs
   bourn_cast<S>(U): 3.180e-004 s mean;        313 us least of 100 runs

  static_cast<D>(U): 6.301e-004 s mean;        626 us least of 100 runs
   bourn_cast<D>(U): 2.042e-003 s mean;       1879 us least of 100 runs

  static_cast<U>(D): 2.561e-003 s mean;       2427 us least of 100 runs
   bourn_cast<U>(D): 1.393e-002 s mean;      12913 us least of  72 runs
   bourn_cast<S>(D): 6.502e-003 s mean;       6284 us least of 100 runs

  static_cast<F>(D): 1.288e-003 s mean;       1207 us least of 100 runs
   bourn_cast<F>(D): 4.819e-003 s mean;       4386 us least of 100 runs
   bourn_cast<D>(F): 4.630e-003 s mean;       4595 us least of 100 runs

Purging everything except the floating-to-integral conversions, because
nothing else should be affected:

    constexpr From limit = std::ldexp(From(1), to_traits::digits);
  static_cast<U>(D): 2.513e-003 s mean;       2435 us least of 100 runs
   bourn_cast<U>(D): 1.346e-002 s mean;      12893 us least of  75 runs
   bourn_cast<S>(D): 6.764e-003 s mean;       6703 us least of 100 runs

    From const limit = std::ldexp(From(1), to_traits::digits);
  static_cast<U>(D): 2.561e-003 s mean;       2427 us least of 100 runs
   bourn_cast<U>(D): 1.393e-002 s mean;      12913 us least of  72 runs
   bourn_cast<S>(D): 6.502e-003 s mean;       6284 us least of 100 runs

    Iterative constexpr ldexp() replacement:
  static_cast<U>(D): 2.561e-003 s mean;       2427 us least of 100 runs
   bourn_cast<U>(D): 1.393e-002 s mean;      12913 us least of  72 runs
   bourn_cast<S>(D): 6.502e-003 s mean;       6284 us least of 100 runs

The only significant difference seems to be in the last timing in each
group, where HEAD appears to be on the order of ten percent slower than
the others. I'd guess that's transient noise.

> GC> Or should we write our own function, e.g.:
> GC> 
> GC> -    constexpr From limit = std::ldexp(From(1), to_traits::digits);
> GC> +    constexpr From limit = const_ldexp(From(1), to_traits::digits);
> GC> 
> GC> ? All we're trying to do is raise two to a representable power. Is there
> GC> a clean way to do that within the intersection of both compilers' 
> constexpr
> GC> rules (and perhaps msvc's too, if reasonable)? Iteration? Recursion? Table
> GC> lookup? Bitwise manipulation of an array of sizeof(long double) bits, with
> GC> reinterpret_cast<long double>?
> 
>  The last one is an especially interesting idea, but there is a slightly
> simpler way to write this function with C++17 (this wasn't valid code in
> C++14 and earlier, so your decision to switch to it was very premonitory):

Yes, a good decision in retrospect, but attributable only to chance.

[...C++ iterative constexpr (thank you) compiles to...]

> (gdb) disassemble
> Dump of assembler code for function main:
> => 0x0000555555554510 <+0>:     mov    eax,0x80
>    0x0000555555554515 <+5>:     ret
> End of assembler dump.

Great, that leaves little prospect for further optimization.

>  So recursion is not the one true way any more

I've barely begun reading TC++PL4 and it's already obsolescent.

>  The only problem with this approach is that MSVS 2015 that I still use
> doesn't support this yet. However MSVS 2017 does, so perhaps this will
> finally push me to update to it. But it seems a pity to write the function
> in a less clear recursive way IMHO.

Especially since the recursive way didn't pass the unit test,
although the reason why remains mysterious...

>  However I can't guarantee that this variant avoids the problem in the unit
> test you've encountered, as it seems like a compiler bug to me. But it
> could be worth at least trying it.

It does indeed avoid that problem.

> GC> As for std::auto_ptr<>,
[...another alternative...]
> would be to just replace auto_ptr with unique_ptr in the Boost headers we
> use. It's not like we're ever going to update them anyhow...

Let me sleep on it--I'll give you my answer in the morning. Meanwhile,
I'll commit the bourn_cast change, cuz one out of two ain't bad.