Fix off-by-one error in X87DoubleExtended::from_bits

[jxors]

While trying to use the rustc_apfloat crate, I noticed that conversion to and from x87 80-bit floating points did not work correctly. In particular, doing a roundtrip via X87DoubleExtended::from_bits(X87DoubleExtended::to_bits(...)) would yield a different result.

I have added two tests and tried to fix the bug. I think I have fixed it correctly, but my knowledge of floating points is limited, so my change should probably be double-checked by someone else before merging.

The standard floating point formats use an implicit bit in the significand. For example, for a 64-bit floating point value (a 'double'), the significand might be a 53-bit value, stored as 52 bits. The most significant bit is implicitly assumed to be 1.
The X87 80-bit floating point format does not use an implicit bit. It stores a significand of 64 bits as 64 bits.

The Semantics::PRECISION constant defines the size of the significand including the implicit bit. So for a 64-bit floating point value, Semantics::PRECISION would be 53 even though only 52 bits are used to store the significand. The code for the standard floating point formats has to work around this, by subtracting 1 from PRECISION to compute the correct number of bits. The code in X87DoubleExtended::from_bits incorrectly also subtracted 1 from PRECISION, even though no implicit bit is used in this format. Thus computing a size that is off-by-one from the actual size.

[rustbot]

Changes rustc_apfloat. rustc_apfloat is currently in limbo and you almost certainly don't want to change it (see #55993).

cc @eddyb

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (or someone else) soon.

Please see the contribution instructions for more information.

[eddyb]

Can you check whether the C++ code in LLVM has the same bug or not? Also, a bit surprising that none of the existing tests cover such an aspect.

If this is divergence from LLVM's APFloat (i.e. a mistake during porting from C++ to Rust), we may accept it, but if it's a bug in LLVM you should report it to them instead.

_{(I really wanted to make this crate more broadly useful but the licensing quagmire has completely discouraged me and at this rate I would rather we scrape it and writ our own, but that also is a massive time sink)}

[jxors]

As far as I can tell, this is the equivalent LLVM code:

void IEEEFloat::initFromF80LongDoubleAPInt(const APInt &api) {
   uint64_t i1 = api.getRawData()[0];
   uint64_t i2 = api.getRawData()[1];
   uint64_t myexponent = (i2 & 0x7fff);
   uint64_t mysignificand = i1;
   uint8_t myintegerbit = mysignificand >> 63;
   ...
}

It reads two separate u64s from the input, rather than extracting the bits via bitshifts and masks. In particular, this code reads an entire u64 (i1) for the significand. The Rust code was incorrectly reading only 63 bits. So this seems to indeed be a mistake during porting.

[eddyb]

Thanks for checking!

First things first: if you look at the top of rustc_apfloat/src/lib.rs you'll see commit linked as the revision the port is based on: llvm-mirror/llvm@23efab2
And that appears to correspond to this commit in the new monorepo: llvm/llvm-project@f3598e8 (we should maybe update the comment in rustc_apfloat/src/lib.rs to point to the monorepo, not the old mirror)

So this is the version of the function we want to look at.
(Looks like at least one bug fix has been made to it since: llvm/llvm-project@b1bcafd)

Anyway, that makes the comparison more obvious, e.g. r.sig == [1 << (Self::PRECISION - 1)] clearly maps to the 0x8000000000000000ULL in the C++ code. That also makes the off-by-one more obvious - as in, without the fix in this PR, those comparisons would never matter because r.sig could not not have values that large.

r? @wesleywiser (r=me on the impl change, given the above reasoning if you want to land this, but I don't know what we should do about the tests - would be nice to keep ported ones and our own separate - and ofc there's the whole licensing issue I don't want to touch myself)

[JohnCSimon]

@jxors
Returning to you to address comments from reviewer

FYI: when a PR is ready for review, send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

@rustbot author

[jxors]

I am sorry, I might be misunderstanding something here. @JohnCSimon could you point out which specific point you would like me to address?

The way I read eddyb's message, there are some issues around licensing/whether to land this at all that need to be decided before this PR can move forward.

[JohnCSimon]

@jxors - I'm just Triage, putting PRs in their proper state. You can ask on Zulip or ping the reviewers - or if I've made a mistake...

send a message containing
@rustbot ready to switch to S-waiting-on-review so the PR is in the reviewer's backlog.

[jxors]

@rustbot ready

Once a decision has been made on whether to land this at all, I'd of course be happy to move tests around or make other changes.

[eddyb]

I posted a longer update to #55993 (comment) regarding the progress with the whole rustc_apfloat situation, but of note for this PR is that the WIP repo already has this bug fixed.

[bors]

☔ The latest upstream changes (presumably #113843) made this pull request unmergeable. Please resolve the merge conflicts.

[wesleywiser]

Hi @jxors, thanks for working on this fix! As @eddyb mentioned, this has been fixed in the new version of rustc_apfloat that just landed in #113843. I'm therefore going to close this PR.

Thanks again for your efforts here! 🙂