[libc-commits] [PATCH] D129916: [libc] Add float type and flag for nearest_integer to enable SSE4.2.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/__support/FPUtil/nearest_integer.h:31
 // are rounded to the nearest integer, tie-to-even.
+static inline float nearest_integer(float x) {
+  if (x < 0x1p24 && x > -0x1p24) {
----------------
sivachandra wrote:
> Can you explain where this overload is to be used? I am asking because I see couple of problems here:
> 
> 1. The constants that you used are of `double` type. So, the RHS expression on line 33 will get evaluated as a double expression and will be equivalent to ` r = x` for a large range of `x` values.
> 2. Even if you add a suffix of `f` to the constants, the behavior is still dependent on the value of `FLT_EVAL_METHOD`: https://en.cppreference.com/w/c/types/limits/FLT_EVAL_METHOD. Perhaps this aspect is not of a major concern on platforms like x86_64 and aarch64 anyway. 
> 
> Going by #1 at least, I am surprised that this fallback implementation is serving as a drop-in-replacement to the x86_64 and aarch64 specializations. If a drop-in-replacement is in fact the intention, then the testing strategy should be this:
> 
> 1. Unit test this fallback implementation separately - testing with the user of this fallback will amount to a kind of integration test.
> 2. Unit test the users with either the fallback or the preferred path. There is not much gain in unit testing the users for both the preferred path and the fallback path.
> 3. If testing of the plumbing of the fallback/preferred path is desired, then testing for that should be done with explicit intent.
> 
> One more point that strikes me here is the comment you have above, "... in case of a tie, might pick a random one among 2 closest integers when the rounding mode is not FE_TONEAREST." If the users' algorithms are really tolerant to such "randomness", then perhaps we should give this fallback a name which captures that randomness aspect. Also, if there is only one user of this fallback, then may be this fallback should live with that user and not as utility?
Thanks for spotting the data type issue!

About the intention of the change, this is to be used for improving `exp*f` functions as experimenting in https://reviews.llvm.org/D130008.  The main thing that these functions are used to to replace the idiom for rounding to nearest integer that we've been using:
```
int k = static_cast<int>(x < 0 ? x - 0.5f : x + 0.5f);
float kf = static_cast<float>(k);
```
This by itself also has all the problems that you mentioned in the comment, such as `FLT_EVAL_METHOD`, possible different answers for different rounding modes, etc.  Maybe I shouldn't use the word `random`.  It well-defined, it's just too wordy to describe its behavior explicitly in all cases.

About testing, this function is similar to `fputil::multiply_add` and `fputil::polyeval` that might give slightly different answers with/without special instructions, and that the fallback path is never taken on aarch64.  So it's suitable for a flag control in the same way:
- The end goal for each math function is that the final outputs are identical regardless of which path `nearest_integer` takes, so technically, we should require both preferred path and fallback path to be tested, unless explicitly stated otherwise.

So to answer to your questions: this function is kind of similar to `polyeval / multiply_add`, and will be used by several functions, hence being shared here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129916