Fix `compiler_rt_logbf_test.c` test failure for Builtins-i386-darwin test suite.

[delcypher]

Summary

It seems that compiler-rt's implementation and Darwin
libm's implementation of logbf() differ when given a NaN
with raised sign bit. Strangely this behaviour only happens with
i386 Darwin libm. For x86_64 and x86_64h the existing compiler-rt
implementation matched Darwin libm.

To workaround this the compiler_rt_logbf_test.c has been modified
to do a comparison on the fp_t type and if that fails check if both
values are NaN. If both values are NaN they are equivalent and no
error needs to be raised.

rdar://problem/55565503

[rupprecht]

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

lib/builtins/fp_lib.h

279–281 ↗

(On Diff #221629)

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.

[compnerd]

@rupprecht as much as I really want to agree with you in that this isn't needed to match the behaviour of the system's libm, the problem is that __builtin_logb can be misused to implement a library. So, really, I would really urge Apple to consider maintaining similarity across all the platforms for the builtins - there is no official guarantee that is made on the behaviour of the builtin operations, so If there is a strong reason to have the divergence, please do share that, and I think that we can probably accommodate that with the small tweak that @rupprecht points out:

if (x != x) {
#if defined(__APPLE__) && defined(__i386__)
  return fromRep(~signBit & rep);
#else
  return x;
#endif
} else if ((rep & signBit) == 0) {

[rupprecht]

@efriedma reviewed my original change, so may also have context on this.

In D67999#1682753, @compnerd wrote:

... the problem is that __builtin_logb can be misused to implement a library ...

The method here is __compiler_rt_logbX (defined as __compiler_rt_logbf etc.), not __builtin_logb. Any code that generates a call to __builtin_logb would need that to be satisfied by linking against libm for the logb symbol definition, not compiler-rt.

IIRC, when I tested this a long time ago by building the builtins library and poking it with nm, none of the __compiler_rt_logbX methods were being exposed (due to static __inline), so I'm not sure how this could be misused to implement a library outside of compiler-rt. I may be missing something.

[delcypher]

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

Well I'm not very happy with this patch but it was a quick fix that got the test passing again so I thought I'd suggest this first. I do have an alternative in mind. If we don't care about bit identical representations of NaN then what we could do is change the test (compiler_rt_logbf_test.c) by replacing

if (toRep(crt_value) !=  toRep(libm_value)) {

with

if (canonicalizeReprNan(toRep(crt_value)) !=  canonicalizeReprNan(toRep(libm_value))) {

Here canonicalizeReprNan() would be a function that returns the input it is given unless it is the bit pattern for a NaN. If the bit pattern is a NaN then it returns a canonical representation of NaN (probably a quiet NaN with the sign bit set to zero).

This means that we don't need to change the implementation of __compiler_rt_logbX() and in testing we will tolerate libm implementations that decide to return different NaN representations than compiler-rt does.

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)

The radar number here is just so Apple can track upstream changes landing into internal repos. The contents of this particular radar doesn't add anything to this discussion because it's just a report that this test is failing.

[scanon]

Backing up what everyone says here: logb doesn't define the sign of NaN results, and 754 explicitly says not to interpret the sign of NaN as having any meaning except in the copySign, absoluteValue, negate, and copy operations. (That's a semantically meaningless statement, since those operations do not exist in a vacuum, which means that you can't actually say anything about the sign of NaN from a formal perspective, but, well, it's the standard we have.)

Matching the libm behavior is nice, but normally outside the scope of compiler-rt w.r.t. NaN encodings (sign, payload, and signaling status--since compiler-rt is meant for use in soft-float contexts, it doesn't have a meaningful notion of signaling NaNs, and generally doesn't even try. We could pin that down more for hybrid situations, but that's a much larger change.)

There's nothing *wrong* with making this match, but we should also relax the test to not expect that it do so.

[delcypher]

Modify test instead.

[delcypher]

@rupprecht @scanon I've reworked this patch to make the test more forgiving of different NaN representations.

[rupprecht]

LGTM, thanks! Please wait for someone else to stamp too, it's been a while since I touched compiler-rt.

[delcypher]

Compare using comparison operator for fp_t and use crt_isnan() to
check for NaNs.

[delcypher]

Fix comment typos.

[delcypher]

@rupprecht I talked with @scanon offline and he came up with a much simpler solution. Could you re-approve this change if you're happy with it?

[rupprecht]

In D67999#1690052, @delcypher wrote:

@rupprecht I talked with @scanon offline and he came up with a much simpler solution. Could you re-approve this change if you're happy with it?

Sorry, didn't see this earlier. The committed patch LGTM!

test/builtins/Unit/compiler_rt_logbf_test.c

 // RUN: %clang_builtins %s %librt -o %t && %run %t
 //===-- compiler_rt_logbf_test.c - Test __compiler_rt_logbf ---------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file checks __compiler_rt_logbf from the compiler_rt library for
 // conformance against libm.
 //
 //===----------------------------------------------------------------------===//
 
 #define SINGLE_PRECISION
+#include "fp_lib.h"
+#include "int_math.h"
 #include <math.h>
 #include <stdio.h>
-#include "fp_lib.h"
 
 int test__compiler_rt_logbf(fp_t x) {
   fp_t crt_value = __compiler_rt_logbf(x);
   fp_t libm_value = logbf(x);
-  // Compare actual rep, e.g. to avoid NaN != the same NaN
-  if (toRep(crt_value) != toRep(libm_value)) {
+  // `!=` operator on fp_t returns false for NaNs so also check if operands are
+  // both NaN. We don't do `toRepr(crt_value) != toRepr(libm_value)` because
+  // that treats different representations of NaN as not equivalent.
+  if (crt_value != libm_value &&
+      !(crt_isnan(crt_value) && crt_isnan(libm_value))) {
     printf("error: in __compiler_rt_logb(%a [%X]) = %a [%X] !=  %a [%X]\n", x,
            toRep(x), crt_value, toRep(crt_value), libm_value,
            toRep(libm_value));
     return 1;
   }
   return 0;
 }