refactor intrinsic_op #8002

thewilsonator · 2018-03-11T11:16:40Z

Also allows to add overloads of intrinsics without updating intrinsic_op, this supersedes #7995 as a more elegant solution to the dmd part of the fix for 18559.

it should also be faster on account of not computing the mangle, using Identifier comparisons and early exits.

Also allows to add overloads of intrinsics without updating intrinsic_op

dlang-bot · 2018-03-11T11:16:41Z

Thanks for your pull request and interest in making D better, @thewilsonator! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.

ibuclaw · 2018-03-11T11:59:43Z

Well, not testing the mangle deco is no good, as not all overrides are intrinsics.

~~I'm not sure if this is faster, now you have linear search, rather than~~ ... for some reason I thought you were using binary search for matching intrinsics in your front-end.

ibuclaw · 2018-03-11T12:05:08Z

src/dmd/toir.d

+            if (id3 == Id.outpw) op = OPoutp;
+
+            if (id3 == Id.bswap)   op = OPbswap;
+            if (id3 == Id._popcnt) op = OPpopcnt;


It would be better to check the identifier length first before the pointer comparison.
i.e: (pseudo-code)

switch (id3.length) { case 3: if (id3 == Id.bsf) op = OPbsf; else if (...) // ... break; // other cases... }

true. Will do.

Hmm, LDC/GDC should be able to vectorise it pretty well without the switch as a masked gather , DMD obviously won't do that. Switching on the length requires loading it, its probably cold: I don't think it would buy all that much over the if chains because they are pretty short.

Well, there were once only half a dozen or so intrinsics in the past, that list has grown considerably since then, and it will only continue to grow.

thewilsonator · 2018-03-11T12:06:45Z

Its faster because there is no need to touch strings, just pointer compares with identifiers.

Well, not testing the mangle deco is no good, as not all overrides are intrinsics.

What do you mean by that? I've been mostly looking at the core.math ones. The one (well three) special cases are bsf, bsr and va_start which are handled.

bt[rcs] are take size_t parameters so that shouldn't matter I can't really see anything else.

Any ideas why this is segfaulting?

ibuclaw · 2018-03-11T12:32:41Z

src/dmd/toir.d

+    id3 = fd.ident;
+    auto md = fd.getModule().md;
+    if (!md)
+        return op;


Regarding segfault, I'm going to make a guess here.

Try:

auto m = fd.getModule(); if (!m || !m.md) return op;

ibuclaw · 2018-03-11T13:57:39Z

src/dmd/toir.d

+    id2 = md.id;
+
+    if (md.packages.dim != 1)
+        return op;


md.packages could also be null here, e.g: for module object.

WalterBright · 2018-03-12T08:51:15Z

src/dmd/toir.d

+        OutBuffer buf;
+        mangleToBuffer(m, &buf);
+        const s = buf.peekString();
+        if (!strcmp(s, "4core4stdc6stdarg"))


strcmp must die. use peekSlice which returns a string. See also https://digitalmars.com/d/archives/digitalmars/D/internals/DMD_Technical_Debt_470.html

This is just a relocation of existing code. I want to get this working first :)

WalterBright · 2018-03-12T08:53:50Z

src/dmd/toir.d

 */
 int intrinsic_op(FuncDeclaration fd)
 {
+    Identifier id1,id2,id3;


Move these declarations to where they are first set to a value.

Make 'em const, too.

WalterBright · 2018-03-12T08:55:30Z

src/dmd/toir.d

-        for (size_t i = 0; i < std_namearray64.length - 1; i++)
+        Lmath:
+        if (id3 == Id.cos)    op = OPcos;
+        if (id3 == Id.sin)    op = OPsin;


its making circle fail.

thewilsonator · 2018-03-12T12:46:57Z

src/dmd/toir.d

-        "5bitop4inplFNbNikZk",
-        "5bitop4inpwFNbNikZt",
-        "5bitop4outpFNbNikhZh",
-        "5bitop5bswapFNaNbNiNfkZk",


Why is only the 32-bit bswap an intrinsic (for both 64 and 32 bit)?

thewilsonator · 2018-03-13T02:39:46Z

Hmm, semaphore seems to fail on 32-bit on this line ( with T == long ? ).

thewilsonator · 2018-03-15T08:35:47Z

I have no idea why this is not working on 32-bit, so in the interest of moving forward with 18559 please review #7995

thewilsonator · 2018-11-11T06:28:22Z

rebased as #8942

refactor intrinsic_op

c05f4b3

Also allows to add overloads of intrinsics without updating intrinsic_op

thewilsonator added 4 commits March 11, 2018 19:18

typo

8b1564e

typos

cdbae70

typos

9758fe7

more

47885c7

ibuclaw reviewed Mar 11, 2018

View reviewed changes

thewilsonator added 4 commits March 11, 2018 20:35

null checks

9ee607c

typo

7f756dc

fix vectors

415cfd3

debug aids

ce073b8

ibuclaw reviewed Mar 11, 2018

View reviewed changes

src/dmd/toir.d

id2 = md.id;

if (md.packages.dim != 1)

return op;

Copy link

Member

ibuclaw Mar 11, 2018

Choose a reason for hiding this comment

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

md.packages could also be null here, e.g: for module object.

thewilsonator added 4 commits March 12, 2018 06:25

more null checks

d416917

reduce spam

0f17b64

reinclude va_start

bc9547f

typo

bfc7235

WalterBright reviewed Mar 12, 2018

View reviewed changes

thewilsonator added 6 commits March 12, 2018 18:14

Add check for deprecated

aad38bd

constify

c15c83c

ws

3b13c2c

Fix goto skips declaration

d5f24be

comment out debug

b05369c

its making circle fail.

add null check

843b18b

thewilsonator commented Mar 12, 2018

View reviewed changes

Don't recognise 64 bit bswap

e8ab6a4

thewilsonator added 3 commits March 12, 2018 20:57

refactor core.stdc.stdarg.va_start

d42331b

revert refactor va_start

61d6219

typo

568795c

remove wrong comment.

1c3a521

thewilsonator mentioned this pull request Apr 5, 2018

dmd part of fix for 18559 #7995

Merged

This was referenced Nov 7, 2018

Remove duplicate builtins from std.math #8926

Merged

refactor intrinsic_op #8942

Closed

thewilsonator closed this Nov 11, 2018

Uh oh!

refactor intrinsic_op #8002

refactor intrinsic_op #8002

Uh oh!

Conversation

thewilsonator commented Mar 11, 2018

Uh oh!

dlang-bot commented Mar 11, 2018

Bugzilla references

Uh oh!

ibuclaw commented Mar 11, 2018

Uh oh!

ibuclaw Mar 11, 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!

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

Uh oh!

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!

thewilsonator Mar 12, 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!

thewilsonator commented Mar 13, 2018

Uh oh!

thewilsonator commented Mar 15, 2018

Uh oh!

thewilsonator commented Nov 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ibuclaw Mar 11, 2018 •

edited

Loading

thewilsonator commented Mar 11, 2018 •

edited

Loading

thewilsonator Mar 12, 2018 •

edited

Loading