convert evalu8.c to evalu8.d #8322

WalterBright · 2018-06-02T05:59:13Z

No description provided.

dlang-bot · 2018-06-02T05:59:14Z

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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

rainers · 2018-06-02T07:11:47Z

Building on Appveyor fails because the D part assumes real exists on the C++ side, but that uses longdouble_soft.
As this is only a temporary problem until the other C files are translated aswell, it's probably easiest to just force the correct mangling with pragma(mangle) on a couple of functions.

WalterBright · 2018-06-02T07:22:53Z

Another possibility may be to just convert bcomplex.c to D as well.

rainers · 2018-06-02T07:27:19Z

Affected functions seem to be el_toldouble, _modulo and Complex_ld::abs.

rainers · 2018-06-02T07:39:13Z

Another option is to use longdouble_soft in D instead of real whenever version CRuntime_Microsoft is active. That needs a tiny change for y2lx, too.

WalterBright · 2018-06-02T20:04:02Z

Since it'll all be converted to D anyway, I figured might as well go ahead and convert bcomplex.c to D in this PR.

WalterBright · 2018-06-03T00:17:13Z

Appveyor failures:

unittest
core.exception.AssertError@src\core\math.d(116): unittest failure
----------------
0x01114497
0x011AA5D3 in gc_clrProxy
core.exception.AssertError@src\rt\util\typeinfo.d(250): Assertion failure
----------------
0x01114441
0x011AA5D3 in gc_clrProxy
2/42 unittests FAILED
--- errorlevel 1

which have nothing to do with this PR. @rainers what's going on?

WalterBright · 2018-06-03T08:03:35Z

@rainers are you able to replicate the AppVeyor failures? I can't on my machine.

WalterBright · 2018-06-03T08:31:45Z

You have to keep targ_ldouble here because when built with LDC on Windows, real is the same as double.

Making that change causes semaphoreci to fail with:

gdmd -c -of../generated/linux/release/64/divcoeff.o  -version=MARS -fPIC -w -de -O -release -inline -m64  dmd/backend/divcoeff.d
dmd/backend/bcomplex.d:217:1: error: struct dmd.backend.bcomplex.Complex_ld no size yet for forward reference
 struct Complex_ld
 ^
dmd/backend/cdef.d:798:1: error: union dmd.backend.cdef.eve no size yet for forward reference
 union eve
 ^

rainers · 2018-06-03T08:43:52Z

src/dmd/e2ir.d

-                    e.EV.Vcldouble.re = re;
-                    e.EV.Vcldouble.im = im;
+                    e.EV.Vcldouble.re = cast(real)re;
+                    e.EV.Vcldouble.im = cast(real)im;


This cast is no longer necessary and is harmful to the LDC build, too.

rainers · 2018-06-03T09:04:23Z

src/dmd/backend/evalu8.d

+ * instead of the soft long double ones.
+ */
+
+extern (D) real el_toldoubled(elem *e)


Sorry, still more uses of real instead of targ_ldouble...

rainers · 2018-06-03T09:09:21Z

src/dmd/backend/evalu8.d

+ */
+version (CRuntime_Microsoft)
+{
+    extern (D) private real _modulo(real x, real y)


I think the casts should go into this function instead of the call.

rainers · 2018-06-03T09:10:05Z

src/dmd/backend/evalu8.d

-#include        "global.h"
-#include        "el.h"
-#include        "type.h"
+version (SPP)


BTW: no module name here? (I won't object to module names without package if that's that's how you want to do it ;-) )

No imports of it yet, I'll probably worry about that when imports happen. Right now, I just want to get it to work :-)

rainers · 2018-06-03T09:58:50Z

Still too many uses of real. Shall I push a fix?

WalterBright · 2018-06-03T10:01:22Z

Yes, I could use your help. I'm mostly just floundering about on these platforms I have little knowledge of.

rainers · 2018-06-03T10:20:34Z

Yes, I could use your help. I'm mostly just floundering about on these platforms I have little knowledge of.

Done. Now let's see the other platforms complain...

…d floats

rainers · 2018-06-03T13:13:21Z

This failed because comparison of two targ_ldoubles is forwarded to longdouble_soft.opCmp, but that cannot return "false" for all comparisons involving nan. Now added explicit checks.

WalterBright · 2018-06-03T23:32:32Z

It's all green and ready to pull! Thanks @rainers for your invaluable help. ping @JinShil @wilzbach @jacob-carlborg ?

JinShil · 2018-06-04T00:18:51Z

src/dmd/backend/bcomplex.d

+        {
+            r = y.re / y.im;
+            den = y.im + r * y.re;
+            q.re = cast(float)((x.re * r + x.im) / den);


Translation looks good, but I don't understand why the casts to float and double in this file are needed.

they make it clearer what is happening

and the casts are necessary if targ_ldouble resolves to longdouble_soft, as there is no implicit conversion to float/double then.

JinShil · 2018-06-04T00:25:28Z

Also, I see an obvious opportunity for templated code here (may even remove the need for the casts), but I understand if you want to keep the code easy to cross-reference to the C implementation.

wilzbach

Nice!

wilzbach · 2018-06-04T00:10:40Z

src/dmd/backend/bcomplex.d

 * Copyright:   public domain
 * License:     public domain
- * Source:      $(LINK2 https://github.com/dlang/dmd/blob/master/src/dmd/backend/bcomplex.d, backend/_bcomplex.d)
+ * Source:      $(DMDSRC backend/_bcomplex.d)


FYI underscore escaping is no longer needed. We enabled the opt-out about two months ago.
See: dlang/dlang.org#2307

Also last year you made a point of using absolute links here as they are clickable (see #7030).
Though I was a fan of DMDSRC, so I don't mind ;-)

yes, it should be the full version.

wilzbach · 2018-06-04T00:14:55Z

src/dmd/backend/evalu8.d

                case TYenum:
-#if !MARS
                case TYmemptr:
-#endif


Shouldn't this result in a version(MARS){} else { ... } ?

I decided that having too many version statements was clutter, and so removed the ones that weren't really necessary.

wilzbach · 2018-06-04T00:17:20Z

src/dmd/backend/evalu8.d

    assert((statusFE() & 0x3800) == 0);
-    assert(e && EOP(e));
-    op = e->Eoper;
+    assert(e && !OTleaf(e.Eoper));


Why not define EOP or EBIN as a function?

I don't think those macros paid off.

JinShil · 2018-06-04T02:29:44Z

It also appears bcomplex.c was not deleted in this PR. Shouldn't it be?

WalterBright · 2018-06-04T03:24:41Z

deleted ?

Yes:
#8328

WalterBright requested review from MartinNowak and wilzbach as code owners June 2, 2018 05:59

WalterBright force-pushed the evalu8.d branch from ddf1ef1 to c8efced Compare June 2, 2018 06:01

WalterBright force-pushed the evalu8.d branch from c8efced to 9372c4c Compare June 2, 2018 20:03

WalterBright force-pushed the evalu8.d branch 12 times, most recently from 9a1673f to f9e5f9c Compare June 2, 2018 23:40

WalterBright force-pushed the evalu8.d branch 7 times, most recently from 680a3cd to e9c315a Compare June 3, 2018 00:56

WalterBright force-pushed the evalu8.d branch from 7c04271 to 0bdad1b Compare June 3, 2018 08:06

WalterBright force-pushed the evalu8.d branch from 0bdad1b to d660b1b Compare June 3, 2018 08:33

rainers reviewed Jun 3, 2018

View reviewed changes

WalterBright force-pushed the evalu8.d branch from d660b1b to ce6af8b Compare June 3, 2018 08:48

rainers reviewed Jun 3, 2018

View reviewed changes

WalterBright force-pushed the evalu8.d branch from ce6af8b to ef352ac Compare June 3, 2018 09:06

rainers reviewed Jun 3, 2018

View reviewed changes

WalterBright force-pushed the evalu8.d branch from ef352ac to 012b565 Compare June 3, 2018 09:16

convert evalu8.c to evalu8.d

09434b9

WalterBright force-pushed the evalu8.d branch from 012b565 to 09434b9 Compare June 3, 2018 09:53

use targ_ldouble instead of real

b9a2139

rainers force-pushed the evalu8.d branch from 98612a9 to b9a2139 Compare June 3, 2018 10:48

workaround for longdouble_soft.opCmp being unable to emulate unordere…

6a48ddb

…d floats

WalterBright added the D Conversion label Jun 3, 2018

JinShil approved these changes Jun 4, 2018

View reviewed changes

wilzbach approved these changes Jun 4, 2018

View reviewed changes

WalterBright added the Merge:auto-merge label Jun 4, 2018

dlang-bot merged commit de5544c into dlang:master Jun 4, 2018

WalterBright deleted the evalu8.d branch June 4, 2018 03:17

WalterBright mentioned this pull request Jun 4, 2018

remove bcomplex.c #8328

Merged

Uh oh!

convert evalu8.c to evalu8.d #8322

convert evalu8.c to evalu8.d #8322

Uh oh!

Conversation

WalterBright commented Jun 2, 2018

Uh oh!

dlang-bot commented Jun 2, 2018

Bugzilla references

Testing this PR locally

Uh oh!

rainers commented Jun 2, 2018

Uh oh!

WalterBright commented Jun 2, 2018

Uh oh!

rainers commented Jun 2, 2018

Uh oh!

rainers commented Jun 2, 2018

Uh oh!

WalterBright commented Jun 2, 2018

Uh oh!

WalterBright commented Jun 3, 2018

Uh oh!

WalterBright commented Jun 3, 2018

Uh oh!

WalterBright commented Jun 3, 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!

rainers commented Jun 3, 2018

Uh oh!

WalterBright commented Jun 3, 2018

Uh oh!

rainers commented Jun 3, 2018

Uh oh!

rainers commented Jun 3, 2018

Uh oh!

WalterBright commented Jun 3, 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!

JinShil commented Jun 4, 2018

Uh oh!

wilzbach 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!

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!

JinShil commented Jun 4, 2018

Uh oh!