Page MenuHomePhabricator

[APFloat] prevent NaN morphing into Inf on conversion (PR43907)
ClosedPublic

Authored by spatel on Sep 17 2020, 8:09 AM.

Details

Summary

We shift the significand right on a truncation, but that needs to be made NaN-safe: always set at least 1 bit in the significand.
https://llvm.org/PR43907

Diff Detail

Event Timeline

spatel created this revision.Sep 17 2020, 8:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 17 2020, 8:09 AM
spatel requested review of this revision.Sep 17 2020, 8:09 AM

Well, I went and did a whole investigation here, only to come around to the idea that what you're doing might be the most reasonable thing.

IEEE 754-2008 section 6.2.3 says:

An operation that propagates a NaN operand to its result and has a single NaN as an input should produce a NaN with the payload of the input NaN if representable in the destination format.
Conversion of a quiet NaN from a narrower format to a wider format in the same radix, and then back to the same narrower format, should not change the quiet NaN payload in any way except to make it canonical.
Conversion of a quiet NaN to a floating-point format of the same or a different radix that does not allow the payload to be preserved, shall return a quiet NaN that should provide some language-defined diagnostic information.

Also, operations on sNaN are permitted to preserve sNaN (if the result can represent it) but are not permitted to substitute a qNaN without signaling.

The standard is not super-clear about how to think about payloads, but probably the most useful idea for language implementors would be that they're an unsigned number right-aligned in the significand, consisting of all the bits following the qNaN indicator bit (and thus cannot be zero in an sNaN).

If I were interpreting all this completely in the abstract, I'd say it collectively implies that we really cannot implement this correctly on top of a right-shift, and the right implementation would have to be:

  • Check whether the payload is representable (i.e. whether truncating it would change any bits).
  • If so, truncate it and then preserve the original qNaN indicator bit.
  • If not, use a zero payload, preserve the original qNaN indicator bit, and then set the second-highest bit if we started with an sNaN (to preserve the sNaN).

But x86 double->float conversion does not keep the payload right-shifted or do overflow checks; it just shifts the payload around, dropping any extra bits. And x86 is probably the most faithful implementation around, and we probably shouldn't have APFloat diverge from its behavior.

So I think the only quibble here is whether it's okay for this to silently produce a qNaN when it might've started with an sNaN. And arguably the current contract of APFloat is to perform operations as if FP exceptions were disabled, and we'll need a slightly different interface if we want to model exceptions properly. So I think this might just be fine as-is.

Steve Canon might have further thoughts.

If we can do it without complication, it would be best to preserve signaling-ness, because that's the more faithful interpretation of IEEE 754 (even though it _doesn't_ match what the HW does, because the HW can signal and APFloat can't). A general principle (imperfectly adhered to) of IEEE 754 is that conversions on signaling NaNs should _either_ signal and produce a quiet NaN (if possible), or should produce a signaling NaN if no signal is possible.

We _certainly_ shouldn't produce infinity, so any movement here is a big improvement.

even though it _doesn't_ match what the HW does, because the HW can signal and APFloat can't

APFloat can signal exceptions: they're supposed to be returned in opStatus.

So I think the only quibble here is whether it's okay for this to silently produce a qNaN when it might've started with an sNaN. And arguably the current contract of APFloat is to perform operations as if FP exceptions were disabled, and we'll need a slightly different interface if we want to model exceptions properly. So I think this might just be fine as-is.

If we started with a qNaN, then we can't hit the bug, right? Ie, if the MSB of the significand is set, there's no way we could shift that out. If that's correct, then we could just always set the bottom bit? That way sNaN remains sNaN even if lose the payload.

Or maybe that's not true in the case of the x87 or other special formats?

So I think the only quibble here is whether it's okay for this to silently produce a qNaN when it might've started with an sNaN. And arguably the current contract of APFloat is to perform operations as if FP exceptions were disabled, and we'll need a slightly different interface if we want to model exceptions properly. So I think this might just be fine as-is.

If we started with a qNaN, then we can't hit the bug, right? Ie, if the MSB of the significand is set, there's no way we could shift that out. If that's correct, then we could just always set the bottom bit? That way sNaN remains sNaN even if lose the payload.

Or maybe that's not true in the case of the x87 or other special formats?

I think it's generally true, even on x87.

If we're going to try to stick with an sNaN, I think we should set the second bit in the significand, which will then be stable across formats for the same reason that the qNaN bit is.

spatel updated this revision to Diff 292772.Sep 18 2020, 5:58 AM

Patch updated:

  1. Set the 2nd bit of the significand to preserve NaN-ness and signalling.
  2. Added TODO comments for potentially better/different solutions.
  3. Adjusted tests to provide better coverage of current behavior.
rjmccall added inline comments.Sep 18 2020, 11:26 AM
llvm/lib/Support/APFloat.cpp
2248

Probably best to clarify:

This can only happen with sNaN, so set the 1st bit after the quiet bit so that we still have an sNaN.

2251

That might be more reasonable than manufacturing an artificial sNaN, yeah. But if we're going to do that, it should happen unconditionally, not just when we ended up with a zero significand.

spatel updated this revision to Diff 292887.Sep 18 2020, 1:19 PM
spatel marked 2 inline comments as done.

Patch updated:
Updated code comments; no code or test changes.

Okay. If we've decided that preserving an sNaN is the best thing to do, as opposed to making it quiet but setting invalid-op, then this looks good to me. But I would be happier if we just made that decision now (what better time?) and then didn't need the TODO.

Okay. If we've decided that preserving an sNaN is the best thing to do, as opposed to making it quiet but setting invalid-op, then this looks good to me. But I would be happier if we just made that decision now (what better time?) and then didn't need the TODO.

I don't have an opinion/experience on the sNaN details, so I'm just going on what's been posted in this review.
@scanon / @efriedma - is this good (enough)?

I think IEEE754's convertFormat operation transforms sNaN->qNaN; I think it makes sense for APFloat to do the same.

I think IEEE754's convertFormat operation transforms sNaN->qNaN; I think it makes sense for APFloat to do the same.

There's a potential snag:
LLVM IR doesn't allow single-precision (32-bit) FP hex values (not sure why) - we convert those from/to 64-bit as shown in the first test diff in this patch.
That means the following -instsimplify test calls APFloat::convert() with SNaN, so the naive fix would set the quiet bit as shown in the CHECK line before we ever get to -instsimplify in this test:

define float @fsub_snan_op1(float %x) {
; CHECK-LABEL: @fsub_nan_op1(
; CHECK-NEXT:    ret float 0x7FF9000000000000
;
  %r = fsub float %x, 0x7FF1000000000000
  ret float %r
}

We could say that's as intended, but we will then be inconsistent for float vs. every other FP type which can specify some form of SNaN in hex. (That's because -instsimplify does not explicitly set the quiet bit in its normal transforms currently.)
It's possible that we can fall back to the argument that non-constrained LLVM FP ignores FP exceptions, so it doesn't matter.
There are a few other regression tests failing, and I'm not sure what to make of them. I'll post the alternate patch alongside this one.

Okay. That honestly seems like something that should just be fixed — it's easy to sniff the literal length to figure out which format we have. But it's probably reasonable to preserve sNaNs in the short term and then come back to this.

spatel updated this revision to Diff 294088.Thu, Sep 24, 9:20 AM

Patch updated:

  1. Assert that losesInfo was set as expected if we lost the entire NaN payload.
  2. Change code comment from 'TODO' to 'FIXME' based on discussion here.
  3. Added APFloat unittests for better coverage of losesInfo and status.
spatel updated this revision to Diff 294091.Thu, Sep 24, 9:34 AM

Updated diff to include clang tests that fail with the setting of the quiet bit.

spatel updated this revision to Diff 294093.Thu, Sep 24, 9:37 AM

Ignore the prior comment/update - I got the review numbers inverted. Sorry about that.

LGTM. Although another option would just be to change how we do this "conversion" at parse time.

efriedma accepted this revision.Thu, Sep 24, 10:04 AM

LGTM

LLVM IR doesn't allow single-precision (32-bit) FP hex values (not sure why) - we convert those from/to 64-bit as shown in the first test diff in this patch.

Okay. That honestly seems like something that should just be fixed — it's easy to sniff the literal length to figure out which format we have. But it's probably reasonable to preserve sNaNs in the short term and then come back to this.

Currently, we use prefixes: "H", "K", "L", "M", "R" to indicate the floating-point format. We have a prefix for every format except double and single. An unprefixed hex constant is assumed to be in double-precision format; if the constant is in some narrower format, it's converted. (Note this is an issue specifically with the text format; bitcode doesn't do this sort of conversion.)

I think we should do two things here:

  1. For reading IR, the conversion shouldn't be using IEEE semantics. We should preserve SNaNs, and forbid any conversion that's not exact.
  2. Maybe add a prefix for single-precision constants, so we can write them with 32 bits, and avoid this sort of issue in the future.

But we can deal with that as a followup.

This revision is now accepted and ready to land.Thu, Sep 24, 10:04 AM