This is an archive of the discontinued LLVM Phabricator instance.

[libc] Add platform independent floating point rounding mode checks.
ClosedPublic

Authored by lntue on Jun 6 2023, 8:56 AM.

Details

Summary

Many math functions need to check for floating point rounding modes to
return correct values. Currently most of them use the internal implementation
of fegetround, which is platform-dependent and blocking math functions to be
enabled on platforms with unimplemented fegetround. In this change, we add
platform independent rounding mode checks and switching math functions to use
them instead. https://github.com/llvm/llvm-project/issues/63016

Diff Detail

Event Timeline

lntue created this revision.Jun 6 2023, 8:56 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 6 2023, 8:56 AM
Herald added a subscriber: tschuett. · View Herald Transcript
lntue requested review of this revision.Jun 6 2023, 8:56 AM
michaelrj added inline comments.Jun 6 2023, 1:04 PM
libc/src/__support/FPUtil/FEnvImpl.h
130 ↗(On Diff #528892)

I like the idea, but I'd like to have a flag to use the old method as well for targets where we're using -ffastmath or other optimizations that might precompute this value unexpectedly.

lntue added inline comments.Jun 9 2023, 5:58 PM
libc/src/__support/FPUtil/FEnvImpl.h
130 ↗(On Diff #528892)

For math functions, I think it is ok be not correctly rounded in all rounding modes with -ffastmath, but ideally, string functions should not be affected by it. On the other hand, -ffast-math flags break the floating point standards in many ways, such as flush denormal numbers to 0, so maybe losing the last bit of accuracy in some rounding modes with -ffast-math is acceptable?

sivachandra added inline comments.Jun 9 2023, 11:30 PM
libc/src/__support/FPUtil/FEnvImpl.h
130 ↗(On Diff #528892)

What do you think of this:

  1. Eliminate get_round from FEnvImpl.h.
  2. Move these new functions to FPUtil/rounding_mode.h.
  3. Make rounding_mode.h::get_round the only implementation of get_round in the libc.
lntue added inline comments.Jun 10 2023, 12:03 AM
libc/src/__support/FPUtil/FEnvImpl.h
130 ↗(On Diff #528892)

I that would be great, but unfortunately I think fegetround still have to return the correct one from fesetround, even with -ffast-math and fno-rounding-math: https://godbolt.org/z/8ca6djYvh

So we still need the current get_round somewhere. Only the internal functions that are not expected to be correctly rounded with -ffast-math or -fno-rounding-mode should use this function.

sivachandra added inline comments.Jun 10 2023, 12:57 AM
libc/src/__support/FPUtil/FEnvImpl.h
130 ↗(On Diff #528892)

I do not know enough about -ffast-math and friends that I can agree or disagree with this. But, I think moving this to a separate header file is still good for distinction of what this FEnvImpl.h is for (it an implementation of a uniform interface to observe and manipulate the hardware FP env).

lntue updated this revision to Diff 530205.Jun 10 2023, 7:03 AM

Move the new functions to a separate header.

lntue marked an inline comment as done.Jun 10 2023, 7:04 AM
lntue added inline comments.
libc/src/__support/FPUtil/FEnvImpl.h
130 ↗(On Diff #528892)

I've moved these functions to rounding_mode.h

sivachandra accepted this revision.Jun 11 2023, 10:58 PM
This revision is now accepted and ready to land.Jun 11 2023, 10:58 PM