add __traits(isReturnOnStack, func) by WalterBright · Pull Request #7908 · dlang/dmd

WalterBright · 2018-02-18T03:20:24Z

Determining if a function return value is on the stack or in registers is both complex, with a lot of special cases, and hard to accurately test for. With it as a testable __trait, at lease we can make the testing easy and accurate.

Spec Change: dlang/dlang.org#2216

dlang-bot · 2018-02-18T03:20:25Z

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.

WalterBright · 2018-02-18T04:41:11Z

To get this to work, I had to move retStyle from toir.d to target.d

WalterBright · 2018-02-18T05:40:20Z

@JinShil spec update is there now, too.

jacob-carlborg · 2018-02-18T08:00:50Z

src/dmd/target.d

+            //printf("  3 true\n");
+            return true;
+        }
+        else if ((global.params.isLinux || global.params.isOSX || global.params.isFreeBSD || global.params.isSolaris) &&


Missing isDragonFlyBSD in the condition.

jacob-carlborg · 2018-02-18T08:02:00Z

test/compilable/isreturnonstack.d

+
+static assert(__traits(isReturnOnStack, test1) == false);
+static assert(__traits(isReturnOnStack, test2) == true);
+


I suggest adding a test where a struct is not returned on the stack.

It's quite a few different conditions and branches in the isReturnOnStack function. Are those covered by other tests? I guess they're implicitly covered by other tests?

Check the green lines in the code tab, they are lines that are covered.

Are you referring to this https://codecov.io/gh/dlang/dmd/compare/bde2b0aee87d3199db22d606a63fe034c48f1c61...87a0811b3fd408c54e77569f3f3c666e8c44203c/diff ? I see quite a few lines that are red instead of green.

All the new code is green. The move of retStyle from one module to another is not new code. Also, the coverage tester does not test Windows specific code.

jacob-carlborg · 2018-02-18T08:03:51Z

changelog/isReturnOnStack.dd

@@ -0,0 +1 @@
+Implement https://dlang.org/spec/traits.html#isReturnOnStack


Please add a description, not just linking to the spec.

There's no real point to copy/pasting. That's what links are for.

The title is already a link. You can't put a link here. Check the preview at DAutoTest.

Also FWIW Ddoc doesn't detect URLs and you have to use LINK2...

Ddoc does detect URLs starting with http: or https:

DAutoTest fails:

.generated/stable_dmd-2.078.2/dmd2/linux/bin64/rdmd --compiler=.generated/stable_dmd-2.078.2/dmd2/linux/bin64/dmd -conf=.generated/stable_dmd-2.078.2/dmd2/linux/bin64/dmd.conf -version=Contributors_Lib ../tools/changed.d \ -o changelog/pending.dd --version "2.080.0" \ --date "To be released" object.Exception@../tools/changed.d(214): Changelog entries should consist of one title line, a blank separator line, and a description.

To fix this you need add a blank line and copy part of description from your spec PR.

I'll approve this PR after this changelog entry issue is addressd.

jacob-carlborg · 2018-02-18T08:04:54Z

src/dmd/target.d

+     * Returns:
+     *   true if return value from function is on the stack
+     */
+    extern (C++) static bool isReturnOnStack(TypeFunction tf)


Although it's pretty obvious, you usually complain of missing Params: section 😉.

ibuclaw · 2018-02-18T08:13:49Z

To get this to work, I had to move retStyle from toir.d to target.d

Isn't retStyle called from other parts of the frontend also?

ibuclaw · 2018-02-18T09:01:45Z

Isn't retStyle called from other parts of the frontend also?

It is! Can you do this also please, which removes retStyle from the gluelayer module.

diff --git a/src/dmd/gluelayer.d b/src/dmd/gluelayer.d
index f163abadb..7caee33c5 100644
--- a/src/dmd/gluelayer.d
+++ b/src/dmd/gluelayer.d
@@ -47,7 +47,6 @@ version (NoBackend)
         Statement asmSemantic(AsmStatement s, Scope* sc) { assert(0); }
 
         // toir
-        RET retStyle(TypeFunction tf)               { return RET.regs; }
         void toObjFile(Dsymbol ds, bool multiobj)   {}
 
         extern(C++) abstract class ObjcGlue
@@ -77,7 +76,6 @@ else version (MARS)
 
         Statement asmSemantic(AsmStatement s, Scope* sc);
 
-        RET retStyle(TypeFunction tf);
         void toObjFile(Dsymbol ds, bool multiobj);
 
         extern(C++) abstract class ObjcGlue
@@ -96,7 +94,6 @@ else version (IN_GCC)
 
     extern (C++)
     {
-        RET retStyle(TypeFunction tf);
         Statement asmSemantic(AsmStatement s, Scope* sc);
     }
 
diff --git a/src/dmd/semantic3.d b/src/dmd/semantic3.d
index 52d827958..40c8bec0d 100644
--- a/src/dmd/semantic3.d
+++ b/src/dmd/semantic3.d
@@ -41,7 +41,6 @@ import dmd.expression;
 import dmd.expressionsem;
 import dmd.func;
 import dmd.globals;
-import dmd.gluelayer;
 import dmd.id;
 import dmd.identifier;
 import dmd.init;
@@ -564,7 +563,7 @@ private extern(C++) final class Semantic3Visitor : Visitor
                     }
                 }
 
-                if (!funcdecl.inferRetType && retStyle(f) != RET.stack)
+                if (!funcdecl.inferRetType && !Target.isReturnOnStack(f))
                     funcdecl.nrvo_can = 0;
 
                 bool inferRef = (f.isref && (funcdecl.storage_class & STC.auto_));
@@ -618,7 +617,7 @@ private extern(C++) final class Semantic3Visitor : Visitor
                     if (funcdecl.storage_class & STC.auto_)
                         funcdecl.storage_class &= ~STC.auto_;
                 }
-                if (retStyle(f) != RET.stack)
+                if (!Target.isReturnOnStack(f))
                     funcdecl.nrvo_can = 0;
 
                 if (funcdecl.fbody.isErrorStatement())

WalterBright · 2018-02-18T09:04:17Z

Isn't retStyle called from other parts of the frontend also?

In semantic3(), but it appeared that importing toir.d directly by traits.d caused it to fail on some platforms. It properly belongs in target.d anyway.

WalterBright · 2018-02-18T09:19:35Z

@ibuclaw that indeed was the culprit.

wilzbach · 2018-02-18T09:33:37Z

src/dmd/traits.d

+    if (e.ident == Id.isReturnOnStack)
+    {
+        /* Extract as a boolean if function return value is in registers
+         * https://dlang.org/spec/traits.html#isReturnOnStack


FWIW we could easily generate the spec from this. It's in the PR queue for a few months already and just needs an approval: #6877

That PR is labeled "Needs Work" and the title contains "[Proof of Concept]".

FWIW it was labelled PoC because it has never been formally approved as this would be a precedent in the spec depending on the actual implementation.
"Needs work" was just a result of merge conflicts that happened over the time ...

jacob-carlborg · 2018-02-18T11:01:24Z

test/fail_compilation/isreturnonstack.d

+TEST_OUTPUT:
+---
+fail_compilation/isreturnonstack.d(11): Error: argument to `__traits(isReturnOnStack, int)` is not a function
+fail_compilation/isreturnonstack.d(12): Error: expected 1 arguments for `isReturnOnStack` but had 2


Isn't it a bit inconsistent to have __traits(isReturnOnStack ... in one error message and only isReturnOnStack in another?

It's consistent with all the other error messages put out by traits.d. If you want to redo all that, that's ok, but it would be a separate PR.

Didn't know that.

Geod24 · 2018-02-18T15:42:14Z

What's the motivation behind this ?

WalterBright · 2018-02-18T19:20:04Z

What's the motivation behind this ?

See the opening comment.

JinShil · 2018-02-18T22:29:30Z

What's the motivation behind this ?

I suspect it has to do with @WalterBright taking an interest in #6246, but I'm only speculating. @WalterBright, you could elaborate more about why this is needed.

WalterBright · 2018-02-19T02:34:52Z

why

Determining if a function return value is on the stack or in registers is both complex, with a lot of special cases, and hard to accurately test for. With it as a testable __trait, at lease we can make the testing easy and accurate.

$(BEST_PRACTICE This has applications in:
    $(OL
    $(LI Returning values in registers is often faster, so this can be used as
    a check on a hot function to ensure it is using the fastest method.)
    $(LI When using inline assembly to correctly call a function.)
    $(LI Testing that the compiler does this correctly is normally hackish and awkward,
    this enables efficient, direct, and simple testing.)
    ))

addressed the issues

ibuclaw · 2018-02-26T18:14:12Z

I would merge this myself, but someone seems to have revoked that ability from me... @wilzbach. :-)

wilzbach · 2018-02-26T19:57:05Z

braddr/d-tester#70 (comment)

wilzbach · 2018-02-26T20:05:45Z

but someone seems to have revoked that ability from me..

That someone wasn't me - this time I'm innocent!
Merging manually...

WalterBright force-pushed the isReturnOnStack branch 5 times, most recently from 84646db to 060cfb9 Compare February 18, 2018 04:40

WalterBright requested review from MartinNowak and ibuclaw as code owners February 18, 2018 04:40

WalterBright force-pushed the isReturnOnStack branch from 060cfb9 to 3731145 Compare February 18, 2018 04:41

JinShil added Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Review:Needs Changelog A changelog entry needs to be added to /changelog labels Feb 18, 2018

WalterBright mentioned this pull request Feb 18, 2018

add __traits(isReturnOnStack, func) dlang/dlang.org#2216

Merged

WalterBright force-pushed the isReturnOnStack branch from 3731145 to f85fe38 Compare February 18, 2018 05:38

WalterBright removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label Feb 18, 2018

JinShil removed the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Feb 18, 2018

jacob-carlborg previously requested changes Feb 18, 2018

View reviewed changes

jacob-carlborg reviewed Feb 18, 2018

View reviewed changes

WalterBright force-pushed the isReturnOnStack branch 2 times, most recently from 81b44c1 to cc0a605 Compare February 18, 2018 09:32

wilzbach reviewed Feb 18, 2018

View reviewed changes

WalterBright force-pushed the isReturnOnStack branch 3 times, most recently from 4710fe3 to 87a0811 Compare February 18, 2018 10:04

jacob-carlborg reviewed Feb 18, 2018

View reviewed changes

WalterBright force-pushed the isReturnOnStack branch from 87a0811 to 365d154 Compare February 21, 2018 21:05

JinShil added the Severity:Enhancement label Feb 23, 2018

add __traits(isReturnOnStack, func)

ab0737e

WalterBright force-pushed the isReturnOnStack branch from 365d154 to ab0737e Compare February 23, 2018 00:26

JinShil added the Merge:auto-merge label Feb 24, 2018

JinShil approved these changes Feb 24, 2018

View reviewed changes

jacob-carlborg approved these changes Feb 24, 2018

View reviewed changes

ibuclaw approved these changes Feb 25, 2018

View reviewed changes

wilzbach merged commit 7c8dc62 into dlang:master Feb 26, 2018

WalterBright deleted the isReturnOnStack branch February 27, 2018 02:33


		static assert(__traits(isReturnOnStack, test1) == false);
		static assert(__traits(isReturnOnStack, test2) == true);

		@@ -0,0 +1 @@
		Implement https://dlang.org/spec/traits.html#isReturnOnStack

Uh oh!

Conversation

WalterBright commented Feb 18, 2018 • edited by JinShil Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Feb 18, 2018

Bugzilla references

Uh oh!

WalterBright commented Feb 18, 2018

Uh oh!

WalterBright commented Feb 18, 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!

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!

ibuclaw commented Feb 18, 2018

Uh oh!

ibuclaw commented Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WalterBright commented Feb 18, 2018

Uh oh!

WalterBright commented Feb 18, 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!

jacob-carlborg Feb 18, 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!

Geod24 commented Feb 18, 2018

Uh oh!

WalterBright commented Feb 18, 2018

Uh oh!

JinShil commented Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WalterBright commented Feb 19, 2018

Uh oh!

ibuclaw commented Feb 26, 2018

Uh oh!

wilzbach commented Feb 26, 2018

Uh oh!

wilzbach commented Feb 26, 2018

Uh oh!

Reviewers

Assignees

WalterBright commented Feb 18, 2018 •

edited by JinShil

Loading

ibuclaw commented Feb 18, 2018 •

edited

Loading

jacob-carlborg Feb 18, 2018 •

edited

Loading

JinShil commented Feb 18, 2018 •

edited

Loading