[libc-commits] [PATCH] D152280: [libc] Add platform independent floating point rounding mode checks.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/FPUtil/FEnvImpl.h:130
+// Quick free standing get rounding mode based on the above observations.
+LIBC_INLINE int quick_get_round() {
+  static volatile float x = 0x1.0p-24f;
----------------
lntue wrote:
> sivachandra wrote:
> > lntue wrote:
> > > michaelrj wrote:
> > > > 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.
> > > 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?
> > 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.
> 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.
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D152280/new/

https://reviews.llvm.org/D152280