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

Improve CFTE support for float/double/real painting #4674

Closed
wants to merge 2 commits into from

Conversation

legrosbuffle
Copy link
Contributor

This extends the special case for float/int painting:
float f;
uint i = cast(uint)&f;

To code like:
float f;
ushort s1 = cast(ushort)&f;
ushort s2 = (cast(ushort*)&f)[1];

for any integer type smaller than float (resp. double and real).

This is required to make isNaN and isInfinite work at compile time in phobos (dlang/phobos#3307).

@yebblies
Copy link
Member

I don't really like the ctfe float-cast ability. If we added ctfe-able intrinsics to extract the bit pattern on the mantissa/exponent/sign bit would that be sufficient for implementing these math functions?

@legrosbuffle
Copy link
Contributor Author

That would work, yes.

@legrosbuffle
Copy link
Contributor Author

Actually we would need the ability to set them too:
git grep 'cast([a-z ]+*)&'
And overall we would also need to make sure not to hurt runtime performance.

@yebblies
Copy link
Member

Setting them is easy too. I would guess they'd be the same speed as C bitfields, so they would be slower than raw pointer manipulation.

Is it much of a burden to have a slow ctfe version? It should be easy to write one portable one:

ulong doSomething(real f)
{
    if (ctfe)
        return extractMantissa(f) + extractExponent(f) + extractSign(f);
    static if (f.sizeof == 10)
        ...
    else static if (f.sizeof == 16)
        ...
}

@legrosbuffle
Copy link
Contributor Author

Is it much of a burden to have a slow ctfe version?
Yes of course not, I was talking about the runtime one :)

I'm not a huge fan of having to specifically write an explicit ctfe version though - it's much better if "normal" (runtime) code can just work at compile time. This PR was a stab at "just having normal code work". Else it means different versions to maintain, more sources of bugs and possible discrepancies between runtime and compile time (note that the current solution is not much better since it's /still/ different code - but all in one place, in the compiler).
Personally I'd rather write my code thinking about runtime, then just use "static" and "enum" and boom! It works.
So I guess it all boils down to: are normal people writing normal (runtime) code going to stop writing pointer casts ? If yes (they /always/ use the helpers), then the helpers work, because it means their code is ctfeable by default. Else, it means that there will be some more code that requires rewriting to be ctfeable.

@ibuclaw
Copy link
Member

ibuclaw commented May 22, 2015

Tests will only work on x86.

@ibuclaw
Copy link
Member

ibuclaw commented May 22, 2015

Also, FYI I did this some time ago: https://github.com/ibuclaw/dmd/commits/ctfepaint

Never raise a PR because my intention is to eventually make it so that float bits are accessible via unions.

With CTFE-able unions, we can start removing the error-prone (cast(T*)&v) casts in std.math.

@yebblies
Copy link
Member

I'm not a huge fan of having to specifically write an explicit ctfe version though - it's much better if "normal" (runtime) code can just work at compile time.

Taking isNaN for an example, there are already five implementations! Including one that will work just fine at compile time, and IMO should replace all the others. A CTFE intrinsic version would make an easily verified, easily generic base implementation. The pointer casting versions are just optimizations.

I mean really, here is the 100% portable, completely ctfe-compatible and @safe isNaN implementation:

return x != x;

Adding more pointer casts and unions to ctfe is a step in the wrong direction.

@andralex
Copy link
Member

@yebblies like. x != x is part of isNaN's very IEEE definition.

@ibuclaw
Copy link
Member

ibuclaw commented May 22, 2015

isNaN isn't a problem. floor/ceil/log et al. are.

@legrosbuffle
Copy link
Contributor Author

Including one that will work just fine at compile time, and IMO should replace all the others.

@yebblies Very good point, that is much better than going the painting/union way (why is it not the only impl btw ?). However we still need a solution for other instances like ieeeMean.

isNaN isn't a problem.

@ibuclaw I disagree with that one. I should be able to use isNaN (and other std.math functions) at compile time. Even if this PR is not the right way to fix it it should be fixed.

@ibuclaw
Copy link
Member

ibuclaw commented May 23, 2015

@ibuclaw I disagree with that one.

You are misreading me. isNaN isn't a problem because you can implement it without pointer/bit checks.

@legrosbuffle
Copy link
Contributor Author

You are misreading me. isNaN isn't a problem because you can implement it without pointer/bit checks.

@ibuclaw OK got it !

@yebblies
Copy link
Member

Very good point, that is much better than going the painting/union way (why is it not the only impl btw ?). However we still need a solution for other instances like ieeeMean.

I'd guess the optimizer fails to reduce the floating point operation to an integer op.

@Biotronic
Copy link
Contributor

@legrosbuffle Will you be updating this for DDMD?

@legrosbuffle
Copy link
Contributor Author

legrosbuffle commented Apr 18, 2016

@Biotronic Nope; It feels like there was no consensus on this approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants