fix Issue 5227 - X ^^ FP at compile-time by WalterBright · Pull Request #8071 · dlang/dmd

WalterBright · 2018-03-23T08:50:51Z

Adds the CTFE functions:

real_t nearbyint(real_t x);
real_t rint(real_t x);
real_t round(real_t x);
real_t floor(real_t x);
real_t ceil(real_t x);
real_t trunc(real_t x);
real_t log(real_t x);
real_t log2(real_t x);
real_t log10(real_t x);
real_t pow(real_t x, real_t y);
real_t expm1(real_t x);
real_t exp2(real_t x);
real_t fmin(real_t x, real_t y);
real_t fmax(real_t x, real_t y);
real_t copysign(real_t x, real_t s);
real_t fma(real_t x, real_t y, real_t z);

dlang-bot · 2018-03-23T08:50:52Z

Thanks for your pull request, @WalterBright!

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	5227	blocker	X ^^ FP at compile-time

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Regression or critical bug fixes should always target the stable branch. Learn more about rebasing to stable or the D release process.

To target stable perform these two steps:

Rebase your branch to upstream/stable:

git rebase --onto upstream/stable upstream/master

Change the base branch of your PR to stable

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#8071"

ibuclaw · 2018-03-23T09:41:26Z

Wouldn't this just add more discrepancies between compile time and runtime?

WalterBright · 2018-03-23T09:50:54Z

These are only executed during CTFE, not optimized constant folding.

But certainly all the std.math functions need to be gone over again to ensure the ULPs are correct.

kinke · 2018-03-23T10:52:14Z

[I circumvented this problem in LDC by using the D host compiler's std.math, as we've already been depending on Phobos.]

UplinkCoder · 2018-03-23T12:55:29Z

more builtins .... sigh.
But what does fma do there, when infact it does not produce a fused-multiply-add.

TurkeyMan

I'd love to see a test for the problem case... although I'm sure it works ;)

TurkeyMan · 2018-03-23T17:19:34Z

test/compilable/test5227.d

+pragma(msg, "pow()");
+enum powf = pow(2.5f, -3.0f); pragma(msg, powf);
+enum powd = pow(2.5 , -3.0 ); pragma(msg, powd);
+enum powr = pow(2.5L, -3.0L); pragma(msg, powr);


There's no test here for the problem case. Please add a test for fractional exponent, eg: pow(2, 3.5);
That's the case that doesn't work in CTFE. Integer exponent seems to be handled as a different case.

TurkeyMan · 2018-03-23T17:21:58Z

OMFG @WalterBright is my homeboy!

Yeah, even if you guys reckon this isn't the most sanitary solution, compile errors when you type 2^^1.5 has never been okay.

WalterBright · 2018-03-23T17:31:16Z

But what does fma do there, when infact it does not produce a fused-multiply-add.

Neither does the std.math one, which is why I suggested a review of std.math.

I circumvented this problem in LDC by using the D host compiler's std.math

std.math may be better off in druntime. In any case, that's a separate issue. The ctfloat.d implementations are not terrible, they just rely on the C functions which aren't the best.

ibuclaw · 2018-03-23T17:44:56Z

@TurkeyMan - you'll start complaining again when 2^^1.5 yields a different result at compile-time vs. run-time. ;-)

There are a couple of functions that should not be here in my opinion, I'll pick them out if they haven't been removed since I started writing.

ibuclaw · 2018-03-23T17:45:43Z

src/dmd/builtin.d

+{
+    Expression arg0 = (*arguments)[0];
+    assert(arg0.op == TOK.float64);
+    return new RealExp(loc, CTFloat.nearbyint(arg0.toReal()), arg0.type);


This makes no sense, because there is no rounding mode at compile time.

Can 'compile time' specify a static rounding mode? Presumably 'nearest', since it's the hardware default...?

Having a look at my end, no, gcc emulated floating point does not.

And hardware default for who? SuperH? Motorola 6800? Soft processors on an FPGA? :-)

You can however convert the float to mpfr and explicitly request a rounding mode.

ibuclaw · 2018-03-23T17:47:57Z

src/dmd/builtin.d

+{
+    Expression arg0 = (*arguments)[0];
+    assert(arg0.op == TOK.float64);
+    return new RealExp(loc, CTFloat.rint(arg0.toReal()), arg0.type);


Likewise having this as a built-in too.

These two are there for compatibility with LDC, and also as a simple matter of convenience so the user doesn't have to use two paths for the same code.

Well I'd rather not add builtins that aren't common for all, as it means you will have to use two paths anyway. For example y2lx comes with version(INLINE_Y2LX).

What's the issue with adding them to gdc?

These are non existent builtins in gcc.

More generally though adding them as builtins would make them more susceptible to wrong code in the const folder/optimizer given.

setRoundingMode(down); immutable x = rint(1.8); setRoundingMode(up); immutable y = rint(1.8);

TurkeyMan · 2018-03-23T17:54:31Z

@ibuclaw And I'll be happy to complain when that comes up! Current situation is much worse that that hypothetical (albeit probable) moment.
None of the cases where I'm blocked on this will compare compile time results against runtime results that I'm aware of.
I need to build shit-loads of look-up tables.

TurkeyMan · 2018-03-23T17:55:22Z

Incidentally; I'm used to building with fast-math and the like. I'm accustomed to CTFE not precisely matching runtime results ;)

wilzbach · 2018-03-23T18:19:27Z

@TurkeyMan - you'll start complaining again when 2^^1.5 yields a different result at compile-time vs. run-time. ;-)

Yes different behavior between CTFE and runtime is horrible to debug, because once you add enum somewhere, your tests fail (or worse: they still pass, but the small difference might add up in your production program.)
For many applications that's okay and they can opt-in with fast-math, but I fully agree with @ibuclaw that this shouldn't be the default.

WalterBright · 2018-03-23T18:30:07Z

Yes different behavior between CTFE and runtime is horrible to debug

There are always going to be slight differences between compile time and run time. Java early on tried to define away the difference, and they were forced to relent. If your algorithm cares about ULP, you really really need to pay close attention to how your expressions are constructed and compiled.

It isn't integer math and cannot be treated like integer math.

Anyhow, again, issues with accuracy are off topic for this PR.

JackStouffer · 2018-03-23T18:33:17Z

This should get a changlog entry.

rainers · 2018-03-23T20:20:49Z

src/dmd/builtin.d


+    add_builtin("_D3std4math3logFNaNbNiNfeZe", &eval_log);
+    add_builtin("_D3std4math3logFNaNbNiNfeZe", &eval_log);
+    add_builtin("_D3std4math3logFNaNbNiNfeZe", &eval_log);


You are adding the same function 3 times here. If the float and double overloads don't exist one builtin should be enough. Same for a number of other functions.

jacob-carlborg · 2018-03-24T12:52:07Z

test/compilable/test5227.d

+import std.math;
+
+pragma(msg, "log()");
+enum logf = log(5.5f); pragma(msg, logf);


How come pragma(msg) is used to test this instead of static assert?

Right. The current tests also test that CTFE log(float|double) etc. return a real, i.e., that there are no float/double overloads, which I'm not sure is desirable.

The std.math.log function does not have overloads, and passing float/double to it will return a real. I don't think CTFE should do something different than the corresponding std.math function.

I don't think CTFE should do something different than the corresponding std.math function.

That isn't the question, it's whether the float/double tests should be there if there's no corresponding function. As soon as Phobos gets proper overloads, these DMD tests will fail, so DMD has to be patched in tandem. I'm in favor of adding corresponding CTFE tests after extending Phobos.

A similar thing is blocking dlang/phobos#6272; I added those CTFE tests myself a while ago. ;)

Put the functions in Phobos first, then in dmd. Not the other way around, because they cannot be tested if put in dmd first.

I'd also recommend skipping these tests for now: Testing them here to return real will block adding the overloads to phobos.

Put the functions in Phobos first, then in dmd. Not the other way around, because they cannot be tested if put in dmd first.

Absolutely, but this patch here puts them in dmd first, that's what I've been trying to tell the whole time.

kinke · 2018-03-24T17:12:33Z

src/dmd/builtin.d

    add_builtin("_D4core4math4fabsFNaNbNiNfeZe", &eval_fabs);
    add_builtin("_D4core4math5expm1FNaNbNiNfeZe", &eval_unimp);
-    add_builtin("_D4core4math4exp21FNaNbNiNfeZe", &eval_unimp);
+    add_builtin("_D4core4math4exp2FNaNbNiNfeZe", &eval_unimp);


&eval_unimp => &eval_exp2 (likewise for expm1 above)

There isn't an exp2 function in core.math. We could add to it, but that discussion is beyond the scope of this PR.

So you noticed the wrong mangle & fixed it instead of just removing that useless entry?

Yes. I don't know why the eval_unimp entries are there, I didn't write that code. But I felt it was beyond the scope of this PR to address that.

FWIW, the same goes for the @trusted/@safe overloads, the functions are all @safe IIRC.

kinke · 2018-03-24T17:12:44Z

src/dmd/builtin.d

    add_builtin("_D4core4math4fabsFNaNbNiNeeZe", &eval_fabs);
    add_builtin("_D4core4math5expm1FNaNbNiNeeZe", &eval_unimp);
-    add_builtin("_D4core4math4exp21FNaNbNiNeeZe", &eval_unimp);
+    add_builtin("_D4core4math4exp2FNaNbNiNeeZe", &eval_unimp);


ditto too :-)

BTW: this replaces @safe with @trusted from the overload above. I think this overload can be safely deleted.

kinke · 2018-03-24T17:14:11Z

src/dmd/builtin.d

    add_builtin("_D3std4math4fabsFNaNbNiNfeZe", &eval_fabs);
    add_builtin("_D3std4math5expm1FNaNbNiNfeZe", &eval_unimp);
-    add_builtin("_D3std4math4exp21FNaNbNiNfeZe", &eval_unimp);
+    add_builtin("_D3std4math4exp2FNaNbNiNfeZe", &eval_unimp);


Likewise, not ditto. :-)

std.math has annotated exp2 with @trusted, so this overload can go in favor of the one a few lines below.

RazvanN7 · 2018-04-10T15:33:51Z

Tests are all green. ping @ibuclaw

WalterBright · 2018-04-14T09:31:25Z

@ibuclaw please respond

rainers · 2018-04-15T10:38:02Z

I agree with Ian that rint and nearbyint don't make much sense without also supporting fesetround or similar. You can always use floor, ceil, trunc or round as explicit replacements.

Please also avoid testing the return type of fallbacks for non-existing functions, these block adding them to core/std.math. You could replace them with static asserts checking the value, but not the type.

WalterBright · 2018-04-15T20:47:23Z

@rainers ok, done. Except I didn't do the static assert, it seemed pointless.

rainers · 2018-04-15T21:47:33Z

src/dmd/builtin.d

+    assert(arg0.op == TOK.float64);
+    Expression arg1 = (*arguments)[1];
+    assert(arg1.op == TOK.float64);
+    return new RealExp(loc, CTFloat.copysign(arg0.toReal(), cast(int) arg1.toInteger()), arg0.type);


LDC fails to comple this line (no implicit conversion to real_t). Why do you cast to int here? copysign expects a real parameter.

@rainers - Doesn't ldc implement their own builtins module independent of dmd?

I think so, but as noted, this is a usage of struct longdouble introduced by compilation with LDC. AFAICT passing int here is wrong anyway, should just be arg1.toReal().

rainers · 2018-04-15T21:48:42Z

Apart from the failure LGTM

ibuclaw · 2018-04-16T06:31:13Z

I guess the build failure is for platforms where dmd uses struct longdouble. Check and use explicit casts to real_t

WalterBright · 2018-04-16T09:53:11Z

@rainers I'm shooting in the dark here with appveyor. Now I'm getting the error:

..\dmd\root\ctfloat.d(88): error : cannot cast expression `roundl(cast(real)x.opCast())` of type `real` to `longdouble`

What do you advise I do to make this work?

rainers · 2018-04-16T18:27:17Z

It needs a small part of #8169:

diff --git a/src/dmd/root/longdouble.d b/src/dmd/root/longdouble.d
index 4b3cedc..0ce7acd 100644
--- a/src/dmd/root/longdouble.d
+++ b/src/dmd/root/longdouble.d
@@ -45,6 +45,13 @@ nothrow:
         else
             ld_set(&this, d);
     }
+    this(real r)
+    {
+        static if (real.sizeof > 8)
+            *cast(real*)&this = r;
+        else
+            this(cast(double)r);
+    }
 
     void opAssign(float f) { ld_set(&this, f); }
     void opAssign(double f) { ld_set(&this, f); }
@@ -71,6 +78,11 @@ nothrow:
         else static if(is(T == double)) return cast(T)ld_read(&this);
         else static if(is(T == long))   return ld_readll(&this);
         else static if(is(T == ulong))  return ld_readull(&this);
+        else static if(is(T == real))
+        {
+            static if (real.sizeof > 8) return *cast(real*)&this;
+            else                        return cast(T)ld_read(&this);
+        }
         else static assert(false, "usupported type");
     }

Then this works:

        static real_t round(real_t x) { return real_t(core.stdc.math.roundl(cast(real)x)); }
        static real_t floor(real_t x) { return real_t(core.stdc.math.floor(cast(real)x)); }
        static real_t ceil(real_t x) { return real_t(core.stdc.math.ceil(cast(real)x)); }
        static real_t trunc(real_t x) { return real_t(core.stdc.math.trunc(cast(real)x)); }
        static real_t log(real_t x) { return real_t(core.stdc.math.logl(cast(real)x)); }
        static real_t log2(real_t x) { return real_t(core.stdc.math.log2l(cast(real)x)); }
        static real_t log10(real_t x) { return real_t(core.stdc.math.log10l(cast(real)x)); }
        static real_t pow(real_t x, real_t y) { return real_t(core.stdc.math.powl(cast(real)x, cast(real)y)); }
        static real_t expm1(real_t x) { return real_t(core.stdc.math.expm1l(cast(real)x)); }
        static real_t exp2(real_t x) { return real_t(core.stdc.math.exp2l(cast(real)x)); }
        static real_t copysign(real_t x, real_t s) { return real_t(core.stdc.math.copysignl(cast(real)x, cast(real)s)); }

and could even be used for the part is(real_t == real).

WalterBright · 2018-04-16T20:13:18Z

@rainers sigh, now I get this error with appveyor:

lib\druntime32mscoff.lib: Error: MS-Coff object module `errno_c_32mscoff.obj` has magic = 5ad5, should be 8664
lib\druntime32mscoff.lib: Error: MS-Coff object module `msvc_32mscoff.obj` has magic = 5ad5, should be 8664
lib\druntime32mscoff.lib: Error: MS-Coff object module `msvc_math_32mscoff.obj` has magic = 5ad5, should be 8664

rainers · 2018-04-16T21:12:18Z

Sorry, atm I have no idea why the object files start with 0x5ad5. Locally the files have magic 0x14c (the message is still for x64 only) and building works fine.

Maybe just another spurious failure? You can restart the build by amending the same commit again.

MartinNowak · 2018-04-17T14:33:25Z

You can restart the build by amending the same commit again.

git commit --allow-empty --amend is handy for that purpose, basically just updates the commit date, make sure you haven't staged anything though.

MartinNowak · 2018-04-17T14:38:13Z

Thanks for fixing this @WalterBright, missing log/exp support in CTFE has been a long-standing annoyance, in particular for precomputing scientific tables.

TurkeyMan · 2018-04-17T17:27:14Z

Thanks @WalterBright! This is the most exciting patch in years! :P
Looking forward to the next release.

WalterBright · 2018-04-18T05:19:59Z

My pleasure!

WalterBright requested review from dnadlinger and ibuclaw as code owners March 23, 2018 08:50

dlang-bot added the Severity:Bug Fix label Mar 23, 2018

WalterBright force-pushed the fix5227 branch 2 times, most recently from 6a3b4e0 to f2ea327 Compare March 23, 2018 09:18

WalterBright force-pushed the fix5227 branch from f2ea327 to 6a59d54 Compare March 23, 2018 09:45

TurkeyMan requested changes Mar 23, 2018

View reviewed changes

WalterBright force-pushed the fix5227 branch from 6a59d54 to 3076077 Compare March 23, 2018 17:34

ibuclaw requested changes Mar 23, 2018

View reviewed changes

WalterBright force-pushed the fix5227 branch from 3076077 to 8cf8ac4 Compare March 23, 2018 19:40

rainers reviewed Mar 23, 2018

View reviewed changes

WalterBright force-pushed the fix5227 branch from 8cf8ac4 to 27ae266 Compare March 24, 2018 00:52

jacob-carlborg reviewed Mar 24, 2018

View reviewed changes

kinke reviewed Mar 24, 2018

View reviewed changes

kinke mentioned this pull request Mar 25, 2018

Phobos: Boost some transcendental math functions for float/double ldc-developers/ldc#2624

Closed

WalterBright mentioned this pull request Apr 15, 2018

dmd part of fix for 18559 #7995

Merged

WalterBright force-pushed the fix5227 branch from 1ccbfe1 to bf3616b Compare April 15, 2018 20:35

rainers reviewed Apr 15, 2018

View reviewed changes

ibuclaw approved these changes Apr 16, 2018

View reviewed changes

WalterBright force-pushed the fix5227 branch from bf3616b to ee23e47 Compare April 16, 2018 08:22

WalterBright added the Merge:auto-merge label Apr 16, 2018

WalterBright force-pushed the fix5227 branch from ee23e47 to 0549649 Compare April 16, 2018 08:54

dlang-bot removed the Merge:auto-merge label Apr 16, 2018

WalterBright force-pushed the fix5227 branch from 0549649 to 4fafe5f Compare April 16, 2018 09:17

WalterBright force-pushed the fix5227 branch 2 times, most recently from abf9651 to f1f46b5 Compare April 16, 2018 19:28

fix Issue 5227 - X ^^ FP at compile-time

6767a32

WalterBright force-pushed the fix5227 branch from f1f46b5 to 6767a32 Compare April 16, 2018 22:13

WalterBright added the Merge:auto-merge label Apr 17, 2018

dlang-bot merged commit a4fbc98 into dlang:master Apr 17, 2018

WalterBright deleted the fix5227 branch April 18, 2018 05:20

Uh oh!

Conversation

WalterBright commented Mar 23, 2018

Uh oh!

dlang-bot commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

Testing this PR locally

Uh oh!

ibuclaw commented Mar 23, 2018

Uh oh!

WalterBright commented Mar 23, 2018

Uh oh!

kinke commented Mar 23, 2018

Uh oh!

UplinkCoder commented Mar 23, 2018

Uh oh!

TurkeyMan left a comment

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!

TurkeyMan commented Mar 23, 2018

Uh oh!

WalterBright commented Mar 23, 2018

Uh oh!

ibuclaw commented Mar 23, 2018

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!

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!

TurkeyMan commented Mar 23, 2018

Uh oh!

TurkeyMan commented Mar 23, 2018

Uh oh!

wilzbach commented Mar 23, 2018

Uh oh!

WalterBright commented Mar 23, 2018

Uh oh!

JackStouffer commented Mar 23, 2018

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!

kinke Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

kinke Mar 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

dlang-bot commented Mar 23, 2018 •

edited

Loading

kinke Mar 25, 2018 •

edited

Loading

kinke Mar 24, 2018 •

edited

Loading

rainers Mar 28, 2018 •

edited

Loading