Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend eligible params types to array generic interfaces and span #24080

Closed

Conversation

alrz
Copy link
Member

@alrz alrz commented Jan 6, 2018

Championed issues: dotnet/csharplang#179, dotnet/csharplang#1757

open questions:

  • do we want to forbid conflicting params overloads with identical element types? (otherwise all overloads would be always applicable in the expanded form and therefore ambiguous; note: the compiler wouldn't give ambiguous result for that case "for free", if we permit that, the overload resolution should be adjusted accordingly) -- this is now implemented in this prototype

Note: this is based on top of nested-stackalloc for spilling.

@alrz
Copy link
Member Author

alrz commented Jan 6, 2018

I'd like to add params Span<T> dotnet/csharplang#535 to the mix but I think it makes sense to wait for "stackalloc initilaizers" dotnet/csharplang#1122 first to reuse its lowering.

@jcouv jcouv added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Jan 9, 2018
@alrz
Copy link
Member Author

alrz commented Jan 15, 2018

stackalloc initializers is here: #24249. we should be able to add params Span support on top of that.

@jcouv
Copy link
Member

jcouv commented Mar 31, 2018

Tagging @gafter since he's the champion for that language feature.

@jcouv jcouv added this to the 16.0 milestone Mar 31, 2018
@jcouv
Copy link
Member

jcouv commented May 22, 2018

Sorry for the delay. I'll queue this for discussion in LDM.
The main open question is what set of types should be recognized for this scenario.

@alrz alrz force-pushed the features/params-ext branch from a53b268 to 85c742f Compare August 17, 2018 23:44
@alrz alrz requested review from a team as code owners August 17, 2018 23:44
@alrz alrz changed the title Extend eligible params types to array generic interfaces Extend eligible params types to array generic interfaces and span Aug 17, 2018
@alrz alrz changed the base branch from master to features/nested-stackalloc August 17, 2018 23:44
@alrz alrz force-pushed the features/params-ext branch 4 times, most recently from 0f2e9b8 to d4f104a Compare August 18, 2018 00:50
@alrz alrz force-pushed the features/params-ext branch from d4f104a to 518a0e5 Compare August 18, 2018 00:59
@alrz
Copy link
Member Author

alrz commented Aug 18, 2018

Since the majority of the work has been done as part of pattern-matching (spilling) and stackalloc init (codegen), this has became a rather smallish change now. @jcouv Could you please push this for the next LDM this month? Please let me know if a proposal is needed in csharplang. Thanks.

@gafter
Copy link
Member

gafter commented Aug 18, 2018

What happens if you call a params Spam method in a loop? Stack overflow? If so, that is not what we want.

@alrz
Copy link
Member Author

alrz commented Aug 18, 2018

@gafter As you said in the other thread, in a loop we can reuse the same Span if all elements are constants, otherwise we could fallback to Span(T[]) - I think we could still emit optimal code most of the time.

Edit: actually, there'd be no reason to fallback to array for this. we capture stackalloc pointer and only initialize it in the loop body. further optimizations are possible, e.g. initialize once if all constant, etc.

@gafter
Copy link
Member

gafter commented Aug 18, 2018

All arguments being constants is rare. If the caller usually has to allocate an array, the feature seems less valuable.

@alrz
Copy link
Member Author

alrz commented Aug 18, 2018

I'm experimenting with initializing a sized struct (ditching off stackalloc completely)

// assuming T is a sized value type
Sized_Struct s;
T* a = (T*)&s;
a[0] = e1;
a[1] = e2;
M(new Span<T>(a, sizeof(T) * 2));

Unlike stackalloc, this won't throw in a loop. (note: stackalloc init optimizations will apply here as well).

Is that something that we could rely on?

@alrz
Copy link
Member Author

alrz commented Sep 14, 2018

I've done a preliminary implementation for the "sized struct" to avoid stack overflow with stackalloc at alrz@aaed6cd. Currently it uses "ImplementationDetails" infra to generate the type, but I think we should extract that part into an standalone component (ImplementationDetails is disabled on debug builds).

While it is a limitation in practice, I think it covers most of the use cases that could possibly be optimized. A warning may be desirable where the underlying type is definitely not sized, except for generics because they could get a size after type substitution.

@jinujoseph jinujoseph modified the milestones: 16.0, 16.3 Jun 9, 2019
@gafter gafter modified the milestones: 16.3, Compiler.Next Jul 15, 2019
@alrz
Copy link
Member Author

alrz commented Jul 20, 2019

I see params span is triaged for 9.0. Do we want params IEnumerable for now or it's postponed altogether? (we'll probably need to adjust VB for new params types anyways)

@gafter
Copy link
Member

gafter commented Jul 22, 2019

I see params span is triaged for 9.0. Do we want params IEnumerable for now or it's postponed altogether? (we'll probably need to adjust VB for new params types anyways)

We're done with C# 8 and about to start triaging features for future versions. We have not done that yet so we don't know.

@alrz
Copy link
Member Author

alrz commented Sep 26, 2019

given the new params Span design (https://github.com/dotnet/csharplang/blob/master/proposals/format.md) this prototype is severely out of date. params IEnumerable support could be trivially added afterwards on top of that. Closing.

@alrz alrz closed this Sep 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Area-Language Design Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants