Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue 19905 Floating point .init should be bitwise identical to .nan #9897

Merged
merged 1 commit into from
May 30, 2019

Conversation

thewilsonator
Copy link
Contributor

@thewilsonator thewilsonator commented May 27, 2019

See also #7568 in particular #7568 (comment)

Walter wrote:
They should have the same bit patterns. We tried to make .init a signalling nan, but this was a complete failure. .init and .nan should both be a quiet nan
Because a signalling nan converts to a quiet nan when used. The problem was nobody could come up with a consistent definition of "used".

@thewilsonator thewilsonator added Review:Needs Changelog A changelog entry needs to be added to /changelog Review:WIP Work In Progress - not ready for review or pulling Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org labels May 27, 2019
@thewilsonator
Copy link
Contributor Author

thewilsonator commented May 27, 2019

One last vestige is

align(2) ulong mantissa = 0xC000000000000001UL; // default to snan
which looks to me wrong unless I'm reading https://www.topcoder.com/community/competitive-programming/tutorials/representation-of-integers-and-reals-section-2/ (subsection Summary of all possible values) since it has (at least) the top and bottom bits sets (which means distinction between MSB and LSB is moot).

@rainers
Copy link
Member

rainers commented May 27, 2019

which looks to me wrong

Yeah, that looks dubious. I don't remember well but I think we settled on to use qnan throughout. This one is a qnan, so the comment is wrong.

These look dubious, too, the highest bit should be used to set signalling/quiet:

longdouble_soft ld_qnan = longdouble_soft(0xC000000000000000UL, 0x7fff);
longdouble_soft ld_snan = longdouble_soft(0xC000000000000001UL, 0x7fff);

@thewilsonator
Copy link
Contributor Author

Yeah, that looks dubious

Good I'm not crazy then.

I don't remember well but I think we settled on to use qnan throughout

#7568 (comment)

These look dubious, too,

If we go through with this then I think we can get rid of snan completely.

@thewilsonator thewilsonator removed Review:Needs Changelog A changelog entry needs to be added to /changelog Review:WIP Work In Progress - not ready for review or pulling labels May 27, 2019
@thewilsonator thewilsonator marked this pull request as ready for review May 27, 2019 08:21
src/dmd/root/longdouble.d Outdated Show resolved Hide resolved
src/dmd/root/longdouble.d Outdated Show resolved Hide resolved
src/dmd/root/longdouble.d Outdated Show resolved Hide resolved
@rainers
Copy link
Member

rainers commented May 27, 2019

I wonder whether problematic values might stick from bad nans in the host compiler as they are more or less reused. A test with an explicit check of the bit patterns would be nice.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @thewilsonator! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
19905 minor Floating point .init should be bitwise identical to .nan

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9897"

@thewilsonator
Copy link
Contributor Author

Added a test, lets find out.

@thewilsonator thewilsonator removed the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label May 29, 2019
@thewilsonator
Copy link
Contributor Author

The spec apparently already says that double.init is double.nan.

I think this is good to go.

@thewilsonator thewilsonator added the Review:Blocking Other Work review and pulling should be a priority label May 29, 2019
@thewilsonator
Copy link
Contributor Author

This blocks #7568

@rainers
Copy link
Member

rainers commented May 29, 2019

Added a test, lets find out.

Thanks, looks good now. I was thinking about a more explicit and extensive test, though, e.g.

import core.stdc.stdio;

T byCTFE(T)()
{
    T x;
    return x;
}

void printNaN(T)(T x)
{
    ubyte[] px = cast(ubyte[])((&x)[0..1]);

    printf(T.stringof.ptr);
    printf(".nan = 0x");
    foreach_reverse(p; 0..T.sizeof)
        printf("%02x", px[p]);
    printf(" mantissa=%d\n", T.mant_dig);
}

bool bittst(ubyte[] ba, uint pos)
{
    uint mask = 1 << (pos % 8);
    version(LittleEndian)
        return (ba[pos / 8] & mask) != 0;
    else
        return (ba[$ - 1 - pos / 8] & mask) != 0;
}

void test(T)()
{
    T a = T.init, b = T.nan;
    printNaN(a);
    ubyte[] pa = cast(ubyte[])((&a)[0..1]);
    ubyte[] pb = cast(ubyte[])((&b)[0..1]);
    assert(pa[] == pb[]);
    
    enum c = byCTFE!T();
    a = c;
    assert(pa[] == pb[]);
    
    // the highest 2 bits of the mantissa should be set, everythng else zero
    assert(bittst(pa, T.mant_dig - 1));
    assert(bittst(pa, T.mant_dig - 2));
    foreach(p; 0..T.mant_dig - 2)
        assert(!bittst(pa, p));
}

void main()
{
    test!float();
    test!double();
    test!real();
}

It should better be merged with some related test files, runnable/nan.d would be a good candidate.

@thewilsonator
Copy link
Contributor Author

Applied that test.

@thewilsonator
Copy link
Contributor Author

@rainers I think this is good to go.

@thewilsonator thewilsonator added the Merge:72h no objection -> merge The PR will be merged if there are no objections raised. label May 30, 2019
Copy link
Member

@rainers rainers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Applied that test.

Thanks. Sorry for the delay.

@dlang-bot dlang-bot merged commit cfa71ec into dlang:master May 30, 2019
@thewilsonator
Copy link
Contributor Author

No problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:auto-merge Merge:72h no objection -> merge The PR will be merged if there are no objections raised. Review:Blocking Other Work review and pulling should be a priority Severity:Bug Fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants