Skip to content

Comments

Make -preview=in means [ref] const scope and accept rvalues#11000

Merged
dlang-bot merged 2 commits intodlang:masterfrom
Geod24:infix
Aug 24, 2020
Merged

Make -preview=in means [ref] const scope and accept rvalues#11000
dlang-bot merged 2 commits intodlang:masterfrom
Geod24:infix

Conversation

@Geod24
Copy link
Member

@Geod24 Geod24 commented Apr 3, 2020

As explained in the changelog entry, this puts `in` more in line with its meaning:
that of an input parameter.
This change was prompted by the introduction of `-preview=in` to make `in` means
`const scope` (its original meaning). If we introduce a `preview` switch,
we might as well go all the way.

Needs test, I know, and won't pass the CI because druntime has scope const scope inheriting in.
But before I dig into making this work, I'd like to start the discussion on the concept itself.

The goal is to make in the "go to" type for input parameter. I had this in mind for a long time, and discussed it with @JinShil on some occasions.
Now that -preview=in has been merged ( #10769 ), I figured it would be the right time to introduce it, hoping to get his approved before the next release.

CC @atilaneves @WalterBright

@Geod24 Geod24 added Severity:Enhancement Review:Needs Approval Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Review:Needs Tests labels Apr 3, 2020
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @Geod24!

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 run digger -- build "master + dmd#11000"

@kinke
Copy link
Contributor

kinke commented Apr 3, 2020

I think the optimal solution would be either const scope or const scope ref, depending on the type and target ABI, for the most efficient way to pass an input parameter. I.e., no ref (extra indirection) for class references and small PODs (incl. delegates and slices for most ABIs), but a ref for non-PODs and large PODs. To be coupled with -preview=rvaluerefparam as you've mentioned in the forum thread.

@Geod24
Copy link
Member Author

Geod24 commented Apr 3, 2020

@kinke : I agree. However, why limit it to in ? Could we make it an ABI thing that small types can be copied instead of passed by const ref when possible ?

@atilaneves
Copy link
Contributor

I think this takes too much control away from the programmer. What if I don't want to pass by ref? Sure, I could declare const scope instead of in, but given that in D move semantics are achieved with pass-by-value, is ref something we want to encourage?

@kinke
Copy link
Contributor

kinke commented Apr 3, 2020

but given that in D move semantics are achieved with pass-by-value, is ref something we want to encourage?

(Optimized) DMD codegen: godbolt
LDC: godbolt
GDC: godbolt

And that's a trivial POD, no postblit/copy ctor and no move ctor either.

@kinke
Copy link
Contributor

kinke commented Apr 3, 2020

However, why limit it to in ? Could we make it an ABI thing that small types can be copied instead of passed by const ref when possible ?

You mean const ref T param could suddenly mean const T param for small PODs, and so &param yield something completely different? That won't work IMO.

Limited to in as a way to make this potential optimization transparent, and to give the user the possibility to check whether it's a ref or not via __traits(isRef) if need be.

Edit: The opposite direction, passing some suited by-value args by-ref under the hood, was discussed in lengthy https://forum.dlang.org/thread/oipegxuwqmrmmzefrqcx@forum.dlang.org, and later in a mailing list with Walter. That'd work with mutable params too, but it's not trivial.

@benjones
Copy link
Contributor

benjones commented Apr 3, 2020

Not sure if this is the best place to leave this, but Herb Sutter's preliminary proposal seems related. Basically, he says the programmer should specify intent (read only, output parameter, etc) and the compiler should choose whether things are passed by pointer or value and ensure legal usage: https://www.youtube.com/watch?v=qx22oxlQmKc&t=3454s

@Geod24
Copy link
Member Author

Geod24 commented Apr 4, 2020

@kinke : Good point, I didn't think about taking the address of it. And things like fiber context switch would make that visible to the user, even if scope is involved. So yes, having it selectively applied sounds better.

I think this takes too much control away from the programmer.

It doesn't take any control away. One is still free to apply the storage classes as one see fit. It doesn't restrict what can be done.

However, it makes it easy to do the right thing by default. You have an input parameter, you put in on it, and you're done. It's self-documenting and it doesn't make you think about the details of what's happening under the hood. It's all about making things easy in the common case.

Regarding move semantic, what @kinke said. The code generated by DMD is terrible, and uncontrollable (you can't prevent a struct being moved). Over the past two years, we've seen work in that direction:

The abstract of that last link directly mention copy elision as a goal for the DIP.

@Geod24 Geod24 requested a review from ibuclaw as a code owner July 14, 2020 14:39
@Geod24 Geod24 changed the title [RFC] Make -preview=in means ref const scope [RFC] Make -preview=in means [ref] const scope and accept rvalues Jul 27, 2020
@Geod24 Geod24 force-pushed the infix branch 2 times, most recently from 00cbd58 to 4a59fea Compare July 29, 2020 05:04
@Geod24 Geod24 force-pushed the infix branch 2 times, most recently from 36a8c78 to 129b874 Compare July 31, 2020 10:15
@Geod24
Copy link
Member Author

Geod24 commented Jul 31, 2020

Tests with my projects pass. Going to extend the usage of in and see if it holds, but this is definitely ready for review.

@UplinkCoder
Copy link
Member

Wow this is a big one.

We should have left a Prefix character for mangle extensions ....

Geod24 added a commit to Geod24/vibe-core that referenced this pull request Aug 1, 2020
As explained in details in dlang/dmd#11000 and dlang/dmd#11474,
'in' has been for a very long time lowered to simply 'const',
or 'scope const' when v2.092.0's '-preview=in' switch is used.
DMD PR 11474 aims to change this, so 'in' is not (visibly)
lowered, and shows up in any user-visible string.
This includes header generation, error messages, and stringof.
Since this code was testing stringof directly, it is affected
by the change. To support testing of both aforementioned DMD PRs,
the change is not tied to a version,
but uses the (soon-to-be) old way as a fallback.
@Geod24
Copy link
Member Author

Geod24 commented Aug 24, 2020

Great, a Windows-only SEGV creeping in...

@kinke
Copy link
Contributor

kinke commented Aug 24, 2020

I told you so: #11000 (comment) due to

dmd/src/dmd/target.d

Lines 558 to 559 in de995df

if (global.params.is64bit && global.params.isWindows)
return null;

@Geod24
Copy link
Member Author

Geod24 commented Aug 24, 2020

Ah I missed that comment. So, any suggestion ?

@kinke
Copy link
Contributor

kinke commented Aug 24, 2020

IMO, just leave proper ABI handling to a follow-up.

@Geod24
Copy link
Member Author

Geod24 commented Aug 24, 2020

Works for me, reverted to the size check

@ibuclaw
Copy link
Member

ibuclaw commented Aug 24, 2020

IMO, just leave proper ABI handling to a follow-up.

Right, the intent be fine though.

if (auto ts = p.type.isTypeStruct())
{
    if (ts.sym.numArgTypes())
         p.storageClass |= STC.ref_;
}
else if (auto tup = .toArgTypes(p.type))  // bypasses target.toArgTypes()
{
    if (tup.arguments)
         p.storageClass |= STC.ref_;
}

Just having a guess though.

As explained in the changelog entry, this puts `in` more in line with
its intended meaning, that of an input parameter.
The compiler should be free to decide whether or not an input parameter
is passed by value or by ref.

Additionally, building on the rationale for rejecting DIP1016,
this adds the ability to accept rvalues for `in` parameter,
and deprecate the switch that was introduced for the rejected DIP.
@Geod24
Copy link
Member Author

Geod24 commented Aug 24, 2020

All green again.

@dlang-bot dlang-bot merged commit e8d254f into dlang:master Aug 24, 2020
@Geod24 Geod24 deleted the infix branch August 24, 2020 16:58
@nordlow
Copy link
Contributor

nordlow commented Aug 24, 2020

Wonderful!

@PetarKirov
Copy link
Member

@Geod24 that's a great achievement, thanks for pushing it through! @kinke @ibuclaw and everyone else, thank you guys for the help as well!

@ibuclaw
Copy link
Member

ibuclaw commented Aug 24, 2020

image

nordlow pushed a commit to nordlow/phobos that referenced this pull request Sep 10, 2020
'in' and 'in ref' overloads will conflict in the future if dlang/dmd#11000 is accepted.
Even if not, 'in' currently means something different based on a switch,
however the intention of this code is obviously the full 'const scope'.
In the some places, 'scope' was omitted as it is meaningless on value types.
kinke added a commit to kinke/dmd that referenced this pull request Oct 7, 2020
This is the promised follow-up on dlang#11000 and makes DMD exploit the
specifics of the few supported ABIs (Win64, SysV x86_64, 32-bit x86).

It also almost perfectly matches the proposed LDC implementation in
ldc-developers/ldc#3578 (just a minor divergence for Win64 and dynamic
arrays, but in that point the LDC and DMD Win64 ABI diverges in
general).
kinke added a commit to kinke/dmd that referenced this pull request Oct 7, 2020
This is the promised follow-up on dlang#11000 and makes DMD exploit the
specifics of the few supported ABIs (Win64, SysV x86_64, 32-bit x86).

It also almost perfectly matches the proposed LDC implementation in
ldc-developers/ldc#3578 (just a minor divergence for Win64 and dynamic
arrays, but in that point the LDC and DMD Win64 ABI diverges in
general).
kinke added a commit to kinke/dmd that referenced this pull request Oct 31, 2020
This is the promised follow-up on dlang#11000 and makes DMD exploit the
specifics of the few supported ABIs (Win64, SysV x86_64, 32-bit x86).

It also almost perfectly matches the proposed LDC implementation in
ldc-developers/ldc#3578 (just a minor divergence for Win64 and dynamic
arrays, but in that point the LDC and DMD Win64 ABI diverges in
general).
AsterMiha pushed a commit to AsterMiha/dmd that referenced this pull request Nov 9, 2020
This is the promised follow-up on dlang#11000 and makes DMD exploit the
specifics of the few supported ABIs (Win64, SysV x86_64, 32-bit x86).

It also almost perfectly matches the proposed LDC implementation in
ldc-developers/ldc#3578 (just a minor divergence for Win64 and dynamic
arrays, but in that point the LDC and DMD Win64 ABI diverges in
general).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.