[PATCH] D67999: Fix `compiler_rt_logbf_test.c` test failure for Builtins-i386-darwin test suite.

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht added a comment.

So... on one hand, there are several reasons this is probably fine:

- The first spec I found [1] (linked from cppreference, not claiming it's the authoritative one) just says that `logb` should return "a NaN" and not specifically +/- NaN... so this is conforming
- The call sites I looked at don't seem to distinguish between +/- NaN, so it shouldn't have an effect on them anyway
- At any rate, this is not an exposed logb method that needs to comply w/ posix, this is just internal to builtins. The logb test just compares it to libm to make sure it's sane.

On the other hand, this makes builtins needlessly diverge on different platforms that may come up and bite someone when debugging this. A different (I won't claim "better" just yet) fix would be to just suppress/update test assertions on i386-darwin.

I'd like to let others weigh in too. What do you think?

(I don't think I can see the rdar link, lemme know if there's some public way to view it)

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/logb.html



================
Comment at: lib/builtins/fp_lib.h:279-281
+      if (rep & signBit) {
+        return fromRep((~signBit) & rep);
+      }
----------------
Instead of taking a branch, it might be faster to always return `fromRep((~signBit) & rep)` -- if the sign bit is not set, it shouldn't have an effect.


Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D67999