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

Implement stackalloc initializers #24249

Merged
merged 24 commits into from
Feb 8, 2018

Conversation

alrz
Copy link
Member

@alrz alrz commented Jan 15, 2018

Proposal: dotnet/csharplang#1122

stackalloc int[3]				// currently allowed
stackalloc int[3] { 1, 2, 3 }
stackalloc int[] { 1, 2, 3 }
stackalloc[] { 1, 2, 3 }

@alrz alrz requested a review from a team as a code owner January 15, 2018 22:29
@alrz
Copy link
Member Author

alrz commented Jan 15, 2018

stackalloc[] syntax is undone but I'd appreciate some AOT reviews. Thanks. #Resolved

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jan 16, 2018

@alrz Was the proposal approved by the LDM? #Resolved

@alrz
Copy link
Member Author

alrz commented Jan 16, 2018

As far as I can tell, the proposal has been approved to merge. though, it's not triaged yet. #Resolved


if (expression.InitializerOpt != null)
{
EmitStackAllocInitializers(expression.Type, expression.InitializerOpt);
Copy link
Member Author

Choose a reason for hiding this comment

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

If unused, should still emit init elements to preserve sideeffects

EmitOpCode(ILOpCode.Ldsflda);
EmitToken(field, syntaxNode, diagnostics);
EmitIntConstant(data.Length);
EmitOpCode(ILOpCode.Cpblk, -3);
Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense to emit initblk if all bytes are the same?

Copy link
Member

Choose a reason for hiding this comment

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

Good point.
The only issue could be is if we want to make Span stackalloc` verifiable, but adjusting verification rules. It is not a problem for now anyways.


In reply to: 162825107 [](ancestors = 162825107)

@@ -1442,9 +1442,17 @@
<Field Name="Initializers" Type="ImmutableArray&lt;BoundExpression&gt;"/>
</Node>

<Node Name="BoundStackAllocArrayInitialization" Base="BoundExpression">
Copy link
Member Author

Choose a reason for hiding this comment

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

Can we reuse BoundArrayInitialization?

@jcouv
Copy link
Member

jcouv commented Jan 22, 2018

I don't think this was discussed in LDM. It's not even a championed issue in csharplang yet. Will bring it up for discussion. #Resolved

@jaredpar
Copy link
Member

jaredpar commented Jan 22, 2018

@jcouv is correct. This feature hasn't been sanctioned yet.

I think this is something we'd likely need to open a feature branch for. Given the new syntax here there are a number of non-compiler concerns that we'd need to address: intellisense and ENC in particular.

#Resolved

@jcouv
Copy link
Member

jcouv commented Jan 24, 2018

LDM discussed this today and OK'ed the proposal.
We'll want to maintain the quirk with typing var in local declarations with stackalloc:

  • var x = stackalloc[] { 1, 2, 3 }; // x has type int*, just like var x = stackalloc int[0];
  • var x = (stackalloc[] { 1, 2, 3 }); // x has type Span<int>, just like var x = (stackalloc int[0]); #Resolved

@alrz
Copy link
Member Author

alrz commented Jan 25, 2018

Yeah, that's messy. we could have avoid that with dotnet/csharplang#877, but I guess we have to live with it now -- unless we're willing to take the breaking change. note, the motivating use case remains intact:

// the compiler fails to determine ternary type and tries to convert both operands to the target type
Span<byte> buffer = size < 128 ? stackalloc byte[size] : new byte[size];

except that we won't depend on stackalloc being a direct local init.

@VSadov @jcouv is this something we could do at this point? #Resolved

@VSadov
Copy link
Member

VSadov commented Jan 25, 2018

In the context of ternary stackalloc has natural Span type.
The special case is only for direct local initialization since that existed before. #Resolved

@VSadov
Copy link
Member

VSadov commented Jan 25, 2018

In the given example “Span” could be “var”. #Resolved

@alrz
Copy link
Member Author

alrz commented Jan 25, 2018

Correction: (stackalloc int[3]) is not permitted. See #22046 #Resolved

@jcouv jcouv added this to the 15.7 milestone Jan 30, 2018
@jcouv jcouv added Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Jan 30, 2018
@jcouv
Copy link
Member

jcouv commented Jan 30, 2018

@alrz @VSadov Just a heads up if you are still targeting this feature for C# 7.3, the approximate timeline is end of February to get features completed (including any critical IDE or other blocking issues) and the feature branches merged back into the main branch. #Resolved

@@ -4,6 +4,7 @@
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
Copy link
Member

@VSadov VSadov Jan 31, 2018

Choose a reason for hiding this comment

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

using static System.Linq.ImmutableArrayExtensions

We generally do not want to work with ImmutableArray through regular Linq extensions #Closed

public class StackAllocInitializerTests : CompilingTestBase
{
[Fact]
public void NoBestType()
Copy link
Member

@jcouv jcouv Feb 7, 2018

Choose a reason for hiding this comment

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

I see tests that have a best type which is managed. Can we also cover the case where the best type is void? #Closed

}
else
{
// If not used, just emit initializer elements to preserve possible sideeffects
Copy link
Member

@jcouv jcouv Feb 7, 2018

Choose a reason for hiding this comment

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

Could you point me to the test that covers that? I'm not sure how to hit this.
Never mind, found it (TestUnused) #Resolved

}
}
";
CompileAndVerify(text,
Copy link
Member

@jcouv jcouv Feb 8, 2018

Choose a reason for hiding this comment

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

Please also verify execution order with expectedOutput: and printing inside Method and some other method. #Closed

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass. A few test suggestions.
I would take a look a unifying syntax nodes and unifying binding logic, to see if that does make the code better.

@alrz
Copy link
Member Author

alrz commented Feb 8, 2018

the blob/cpblk optimization will need to be constrained to happen only when dealing with byte-sized elements.

@VSadov Does this apply to initblk as well?

@VSadov
Copy link
Member

VSadov commented Feb 8, 2018

@alrz - initblk is ok regardless of the element size.

@alrz
Copy link
Member Author

alrz commented Feb 8, 2018

initblk is ok regardless of the element size.

Do I need to cover that case in the current PR? that would need GetDataRaw to be called early in ShouldEmitBlockInitializerForStackAlloc which in turn would require a different ArrayInitializerStyle

@VSadov
Copy link
Member

VSadov commented Feb 8, 2018

@alrz - I think we should drive this PR to completion and then do directed PRs that address specific issues.

It may also be acceptable to restrict initblk to the same criteria as cpyblk. Not because it is unsafe to do, but just out of convenience.
Considering that stackalloc zero-initializes by default, initblk case might not be such a common scenario. In fact when we can be sure that localsinit flag is set on the method, it might be easier to just drop the zero-out initializer since JIT zeroinits anyways.

Anyways, it feels like addressing this issue of when and what optimization happens would be easier in a separate PR.

@alrz
Copy link
Member Author

alrz commented Feb 8, 2018

@jcouv I've addressed all comments. Not sure about symbol tests, please take look if it covers what you had in mind.

@alrz
Copy link
Member Author

alrz commented Feb 8, 2018

Considering that stackalloc zero-initializes by default,

You mean only after #23951, right? last time I checked it was not zero-initialized.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

One nit and two small test adjustments, then looks good to me. Thanks!

@alrz
Copy link
Member Author

alrz commented Feb 8, 2018

testing GetTypeInfo directly on the syntax of the stackalloc.

also test this when the type isn't bound

I think both are addressed now. Note that we call GetSemanticInfoSummary on the direct syntax of the stackalloc.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM

@VSadov
Copy link
Member

VSadov commented Feb 8, 2018

Thanks!!

@VSadov VSadov merged commit 916304f into dotnet:features/stackalloc-init Feb 8, 2018
@VSadov
Copy link
Member

VSadov commented Feb 8, 2018

Now it makes sense to tune when optimizations happen.

The idea is that

@alrz alrz deleted the features/stackalloc-init branch March 13, 2018 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers 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.

7 participants