fix Issue 12412. CTFE-able math #2521

9il · 2014-09-16T14:41:43Z

Issue:
https://issues.dlang.org/show_bug.cgi?id=12412

Make isNaN, isNormal, isSubnormal, isFinite, isInfinity CTFE-able.

CyberShadow · 2014-09-16T15:21:59Z

Please rebase to remove the merge commit.

I see the function return value changed. This is technically a breaking change, however f() != 0 or f() == 0 seems to continue working if f returns bool.

9il · 2014-09-16T15:49:15Z

@CyberShadow, rebased.

Previously this functions returned either 0 or 1. But with auto declarations it can be breaking change.
isNaN, isInfinity already returns bool.
I have changed return type for isFinite, isNormal and isSubnormal.

quickfur · 2014-09-16T17:05:38Z

LGTM.

schuetzm · 2014-09-16T19:19:31Z

Stupid question: Why can't the CTFE versions be used at runtime as well, instead of the bit testing? This would simplify the code greatly. Would it have bad performance?

9il · 2014-09-16T19:28:09Z

@schuetzm Bad performance would be with isNormal and isSubnormal.
For other functions I don't now. I think if(someInt == otherInt && f.isFinity) can be optimized with bit testing.

monarchdodra · 2014-09-16T21:26:46Z

None of these functions need an else after the __ctfe block. IMO, it looks clearer to have a special case for CTFE, and then simply have the regular non-indented code if we haven't prematurely returned yet.

It also reduces the amount of diff we need to review, or the amount of code blame that gets assigned to you.

9il · 2014-09-17T04:37:42Z

@monarchdodra PR rebased

monarchdodra · 2014-09-17T07:27:49Z

std/math.d

+    foreach(T; TypeTuple!(float, double, real))
+    {
+        // CTFE-able tests
+        assert(isFinite(T(1.23)));


How are these "CTFE-able" tests? Shouldn't those be static asserts?

BTW, you might be interested in assertCTFEable(alias dg)() in std.exception. This will run your entire "dg", both in runtime and ctfe time.

Thanks, I will use it.

9il · 2014-09-17T08:32:48Z

@monarchdodra I will add CTFE signbit, frexp, isIdentical.
Should I create new PR or add new commits into this PR?

…unittests minor update remove else after ctfe update unittests remove line remove line

9il · 2014-09-17T08:48:21Z

Rebased

9il · 2014-09-17T14:04:17Z

:-( I have no idea how make signbit, frexp, isIdentical CTFE-able .

quickfur · 2014-09-17T14:10:21Z

Welcome to the club. :-P I've tried this before. I think the only solution is to use bit manipulation on the IEEE representation, but unfortunately that's not yet possible in CTFE. Basically, we need to implement support for type reinterpretation via unions in CTFE; once that is done, then actually most of the std.math functions will work as-is.

9il · 2014-09-17T14:36:37Z

@quickfur I have tried:

int signbit(X)(X x) @nogc @trusted pure nothrow
{
    alias F = floatTraits!(X);
    static union Rep
    {
        X x;
        ubyte[X.sizeof] b;
    }
    Rep rep = Rep(x);
    return (rep.b[F.SIGNPOS_BYTE] & 0x80) != 0;
}

... without any hope, Error: rep.b[7] is used before initialized

quickfur · 2014-09-17T14:39:17Z

Yes, because CTFE doesn't support that yet. The first thing to do would be to implement support in dmd, then we can proceed. Unfortunately, CTFE is one of the places I'm completely unfamiliar with, so I've no idea where to even begin.

quickfur · 2014-09-17T18:07:50Z

Blocked on dmd limitation.

monarchdodra · 2014-09-18T17:57:09Z

Blocked on dmd limitation.

Well, partially, right? What's currently in the pull works.

quickfur · 2014-09-18T18:04:21Z

True. I suppose we could merge this first, and then do a followup PR once the dmd limitation is addressed.

9il · 2014-09-18T18:06:39Z

Yes, the easy part done.

monarchdodra · 2014-09-18T18:11:45Z

Well, I don't see anything wrong with this pull, but I'm no numerics expert.

quickfur · 2014-09-19T16:45:29Z

std/math.d

@@ -4525,76 +4533,24 @@ bool isInfinity(X)(X x) @nogc @trusted pure nothrow
    }
    else
    {
-        return (x - 1) == x;
+        return x == x && x - x;


Just trying to understand the change here: what was wrong with (x - 1) == x?

enum M = 2.0L^^(real.max_exp-1) ; static assert(M - 1 == M);//OK

Ah, I see.

Also, is there a performance hit associated with writing x == x && x - x != 0.0? I find that a little clearer, as it isn't obvious that x-x here is being implicitly cast to bool unless you're reading the code carefully.

I think performance is same.

sgn function for example uses implicite cast to bool. It is faster to understand the line of code. When I see f == 0.0 this really mean
f == 0.0 || f == -0.0 so I need to think about that.

DmitryOlshansky · 2014-09-20T08:22:16Z

LGTM with the same notes as @monarchdodra I'm no numerics guy.

ibuclaw · 2014-09-20T10:45:08Z

std/math.d

-        assert(!isNaN(-f));
-        f = cast(T)53.6;
-        assert(!isNaN(f));
-        assert(!isNaN(-f));


I'd rather that you don't remove the runtime tests. These were put here because the compiler - at least gdc - was able to const-fold the tests using literals above.

9il · 2014-09-20T11:28:12Z

PR Closed.
Reasons:

if(__ctfe) kills inlining in DMD.
Other CTFE math can not be implemented until CTFE-able unions.

ibuclaw · 2014-09-20T11:37:33Z

Other CTFE math can not be implemented until CTFE-able unions.

Has been on my todo list, but keeps being pushed back. ;)

yebblies · 2014-09-20T13:16:24Z

Can some of these can have their current implementation replaced with the ctfe version without performance loss? I'd like to see all the bitmasking replaced eventually...

9il · 2014-09-20T13:26:37Z

Need tests with LDC and GDC like

foreach(double a; ar)
{
   if(a.isNaN && some_int==other_int)
   {
    ...
   }
   else
   {
    ...
   }
}

monarchdodra reviewed Sep 17, 2014
View reviewed changes

fix Issue 12412. isNaN and brothers update: CTFE-able, returns bool, …

ef3c675

…unittests minor update remove else after ctfe update unittests remove line remove line

quickfur added the Merge:Blocked label Sep 17, 2014

9il changed the title ~~fix Issue 12412~~ fix Issue 12412. CTFE-able math Sep 17, 2014

9il mentioned this pull request Sep 18, 2014

Precise fsum #2513

Closed

quickfur removed the Merge:Blocked label Sep 18, 2014

quickfur reviewed Sep 19, 2014
View reviewed changes

ibuclaw reviewed Sep 20, 2014
View reviewed changes

9il closed this Sep 20, 2014

9il deleted the ctfe-math branch September 23, 2014 20:05

9il mentioned this pull request May 15, 2016

Make isNaN(), isFinite(), and isInfinity() work at CTFE #4328

Closed

Uh oh!

fix Issue 12412. CTFE-able math #2521

fix Issue 12412. CTFE-able math #2521

Uh oh!

Conversation

9il commented Sep 16, 2014

Uh oh!

CyberShadow commented Sep 16, 2014

Uh oh!

9il commented Sep 16, 2014

Uh oh!

quickfur commented Sep 16, 2014

Uh oh!

schuetzm commented Sep 16, 2014

Uh oh!

9il commented Sep 16, 2014

Uh oh!

monarchdodra commented Sep 16, 2014

Uh oh!

9il commented Sep 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

9il commented Sep 17, 2014

Uh oh!

9il commented Sep 17, 2014

Uh oh!

9il commented Sep 17, 2014

Uh oh!

quickfur commented Sep 17, 2014

Uh oh!

9il commented Sep 17, 2014

Uh oh!

quickfur commented Sep 17, 2014

Uh oh!

quickfur commented Sep 17, 2014

Uh oh!

monarchdodra commented Sep 18, 2014

Uh oh!

quickfur commented Sep 18, 2014

Uh oh!

9il commented Sep 18, 2014

Uh oh!

monarchdodra commented Sep 18, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DmitryOlshansky commented Sep 20, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

9il commented Sep 20, 2014

Uh oh!

ibuclaw commented Sep 20, 2014

Uh oh!

yebblies commented Sep 20, 2014

Uh oh!

9il commented Sep 20, 2014

Uh oh!

Uh oh!