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

[Siva Chandra via Phabricator via libc-commits]

sivachandra 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) {
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129916