[Bug libstdc++/96958] New: Long Double in Hash Table policy forces soft-float calculations

[Bug libstdc++/96958] New: Long Double in Hash Table policy forces soft-float calculations

[jgreenhalgh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

            Bug ID: 96958
           Summary: Long Double in Hash Table policy forces soft-float
                    calculations
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jgreenhalgh at gcc dot gnu.org
  Target Milestone: ---

It was pointed out that some forks of GCC (
https://github.com/FEX-Emu/gcc/commit/8a2b7389f50a50a4e26ec98101d47fb1fc1c1bcd
) reduce the hashtable policy implementation from a long double to a double.
Doing this reduces it from a soft-float calculation to hardware floating-point.

Reading the discussion on libstdc++ from when this code was introduced the
intention was to provide massive amounts of forwards compatibility for Very Big
hash tables. We're taking quite an efficiency hit for that future proofing.

[Bug libstdc++/96958] Long Double in Hash Table policy forces soft-float calculations

[jgreenhalgh at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

--- Comment #1 from James Greenhalgh <jgreenhalgh at gcc dot gnu.org> ---
Asleep at the wheel today, I had intended to link to the
https://gcc.gnu.org/pipermail/libstdc++/2011-September/036420.html original
discussion rather than leave it as a tedious exercise for the reader.

[Bug libstdc++/96958] Long Double in Hash Table policy forces soft-float calculations

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2020-09-07
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

[Bug libstdc++/96958] Long Double in Hash Table policy forces soft-float calculations

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
IMHO _any_ FP calculation in that spot is unwanted (but _M_max_load_factor is a
FP value?)

[Bug libstdc++/96958] Long Double in Hash Table policy forces soft-float calculations

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Yes, the public API for the load factor is defined in terms of float. We use a
higher precision type internally though.

[Bug libstdc++/96958] Long Double in Hash Table policy forces soft-float calculations

[houdek.ryan@fex-emu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

Ryan Houdek <houdek.ryan@fex-emu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |houdek.ryan@fex-emu.org

--- Comment #4 from Ryan Houdek <houdek.ryan@fex-emu.org> ---
Hello. Original creator of this fork here.
It would be nice to at least remove the long double requirement here, I
personally don't have much stock in the internal implementation details other
than that.
I believe other STL compliant hashable implementations end up doing something
using integers for their bucket calculations. So I guess it isn't strictly
necessary that it needs to even be double there.

In my particular case the long double implementation falls down a particularly
slow soft float path for me, so this has real performance implications. More so
than just the regular AArch64 soft float path.

Looking forward to this getting rectified.

[Bug libstdc++/96958] Long Double in Hash Table policy forces soft-float calculations

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:a1343e5c74093124d7fbce6052d838f47a8eeb20

commit r11-4581-ga1343e5c74093124d7fbce6052d838f47a8eeb20
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Fri Oct 30 15:14:33 2020 +0000

    libstdc++: Use double for unordered container load factors [PR 96958]

    These calculations were changed to use long double nearly ten years ago
    in order to get more precision than float:
    https://gcc.gnu.org/pipermail/libstdc++/2011-September/036420.html

    However, double should be sufficient, whlie being potentially faster
    than long double, and not requiring soft FP calculations for targets
    without native long double support.

    libstdc++-v3/ChangeLog:

            PR libstdc++/96958
            * include/bits/hashtable_policy.h (_Prime_rehash_policy)
            (_Power2_rehash_policy): Use double instead of long double.

[Bug libstdc++/96958] Long Double in Hash Table policy forces soft-float calculations

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.0

--- Comment #6 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I've just realised there's a second place I need to replace long double ...
which explains why the commit above didn't fix the test failures I was seeing
on POWER9 when mixing ibm128 and ieee128 long double formats.

I probably should have looked at this commit sooner:

(In reply to James Greenhalgh from comment #0)
> It was pointed out that some forks of GCC (
> https://github.com/FEX-Emu/gcc/commit/
> 8a2b7389f50a50a4e26ec98101d47fb1fc1c1bcd ) reduce the hashtable policy
> implementation from a long double to a double. Doing this reduces it from a
> soft-float calculation to hardware floating-point.

N.B. the commit msg in that fork talks about Oracle making this change. I don't
think it has anything to do with Oracle at all, except that one of the main
libstdc++ contributors happens to work for them. But he didn't change it
because of anything Oracle was doing or wanted, it was just normal libstdc++
maintenance.

[Bug libstdc++/96958] Long Double in Hash Table policy forces soft-float calculations

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:943cc2a1b70f2d755b4fed97b1c4b49234d92899

commit r11-4585-g943cc2a1b70f2d755b4fed97b1c4b49234d92899
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Sat Oct 31 00:52:57 2020 +0000

    libstdc++: Use double for unordered container load factors [PR 96958]

    My previous commit for this PR changed the types from long double to
    double, but didn't change the uses of __builtin_ceill and
    __builtin_floorl. It also failed to change the non-inline functions in
    src/c++11/hashtable_c++0x.cc. This should fix it properly now.

    libstdc++-v3/ChangeLog:

            PR libstdc++/96958
            * include/bits/hashtable_policy.h (_Prime_rehash_policy)
            (_Power2_rehash_policy): Use ceil and floor instead of ceill and
            floorl.
            * src/c++11/hashtable_c++0x.cc (_Prime_rehash_policy): Likewise.
            Use double instead of long double.

[Bug libstdc++/96958] Long Double in Hash Table policy forces soft-float calculations

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Should be properly fixed now.

[Bug libstdc++/96958] Long Double in Hash Table policy forces soft-float calculations

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
N.B. the calls to __builtin_ceill and __builtin_floorl also need to be changed
to avoid implicit conversions to long double.

[Bug libstdc++/96958] Long Double in Hash Table policy forces soft-float calculations

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96958

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
There are a few other places doing unnecessary long double arithmetic, e.g.
r11-4588-60d9f254876a00260992b2f37639ef4d82d9db8f