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

Make std.math.isIdentical work in CTFE #7515

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

n8sh
Copy link
Member

@n8sh n8sh commented Jun 6, 2020

Works because we are in CTFE and there is no way in CTFE to set more bits of NaN payload than can fit in a double, and since 2.087 changed real.init to be non-signaling I think there is no way in CTFE for a real to be a signaling NaN unless real and double have the same representation so real's bits can be manipulated directly.

If the above is not true this should not be approved.

@n8sh n8sh requested a review from ibuclaw as a code owner June 6, 2020 01:32
@dlang-bot
Copy link
Contributor

dlang-bot commented Jun 6, 2020

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close Bugzilla Severity Description
20197 enhancement Make std.math.isIdentical work in CTFE

Testing this PR locally

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

dub run digger -- build "master + phobos#7515"

@n8sh n8sh changed the base branch from master to stable June 6, 2020 01:44
@n8sh n8sh changed the base branch from stable to master June 6, 2020 01:44
@kinke
Copy link
Contributor

kinke commented Jun 6, 2020

In general, it seems the only reason for that function is that DMD includes the padding bytes for real is real. LDC handles this (only comparing the 10 payload bytes for x87 real), so this would work:

bool isIdentical(real a, real b) {
    return a is b;
}

static assert(isIdentical(2, 0x1p1));
static assert(!isIdentical(0.0L, -0.0L));

The comment is misleading as well, as equal but different representations (think 0x2p0, 0x1p1) aren't (bitwise) identical, so it's not just about sign for zero and NaN payloads.

Edit: GDC apparently also just compares the 10 bytes: https://godbolt.org/z/3z3W36

@n8sh
Copy link
Member Author

n8sh commented Jun 7, 2020

In general, it seems the only reason for that function is that DMD includes the padding bytes for real is real.

I'd give another reason: is considers all NaNs equivalent regardless of representation.

@kinke
Copy link
Contributor

kinke commented Jun 7, 2020

If you look at the asm, LDC and GDC definitely don't - they use memcmp.

@n8sh
Copy link
Member Author

n8sh commented Jun 7, 2020

If you look at the asm, LDC and GDC definitely don't - they use memcmp.

Really? Because assert(real.nan is -real.nan) runs fine with LDC on run.dlang.io and on my computer.

@kinke
Copy link
Contributor

kinke commented Jun 7, 2020

Yes, really, as easily shown in IR or ASM output. When looking at the AST output, you'll see that the frontend 'optimizes' your assert to an assert(true).

@kinke
Copy link
Contributor

kinke commented Jun 7, 2020

std/math.d Outdated
if (__ctfe)
{
if (x !is y) return false;
if (x == x) return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't that be x == y?

Copy link
Member Author

@n8sh n8sh Mar 9, 2021

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, now I remember. It doesn't matter: the only circumstance where x is y but not x == y is if both x and y are NaN. So checking x == x is the same as checking x == y (it's a NaN check).

@n8sh
Copy link
Member Author

n8sh commented Mar 9, 2021

By the current specification

For struct objects and floating point values, identity is defined as the bits in the operands being identical

so this whole function could in principle be rewritten as return x is y. However as of DMD 2.095 that still isn't true for floating point numbers during CTFE. Possibly related open PR: dlang/dmd#7568

@LightBender LightBender added the Review:Salvage This PR needs work, but we want to keep it and it can be salvaged. label Oct 27, 2024
@LightBender
Copy link
Contributor

Sending this to @thewilsonator to see if he can Salvage it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Merge:Needs Rebase Merge:stalled Review:Needs Work Review:Salvage This PR needs work, but we want to keep it and it can be salvaged. Severity:Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants