Collection expressions: use inline arrays for collection expressions with span target types #69412

cston · 2023-08-05T17:54:57Z

Use inline arrays for the code generated for collection expressions with Span<T> or ReadOnlySpan<T> target types; fall back to arrays on platforms that do not support inline arrays.

See collection-expressions.md#ref-safety.

Relates to test plan #66418

…with span target types

…en possible

jcouv · 2023-08-11T18:02:14Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

@@ -618,6 +619,41 @@ public override BoundNode VisitRefTypeOperator(BoundRefTypeOperator node)
            return node.Update(operand, getTypeFromHandle, type);
        }

+        private BoundStatement? RewriteFieldOrPropertyInitializer(BoundStatement initializer)
+        {
+            var previousLocals = _additionalLocals;


I didn't understand, why introduce this new _additionalLocals machinery, which affects every single block, when one can easily insert a new block during lowering of specific nodes when additional locals are needed? #Closed

The inline array temporary needs to be added in the enclosing scope (so the lifetime of the temporary matches that scope) rather than being created in a new nested block.

I see. Thanks much for the explanation. Consider adding a comment on the declaration of _additionalLocals (allows adding locals without inserting new blocks/scopes)

jcouv · 2023-08-15T19:11:10Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.cs

+                    {
+                        return statement;
+                    }
+                    return new BoundBlock(


I'm a little worried about the fact that a Visit method besides VisitBlock has to handle additional locals.
How do we know whether any other Visit methods (current or future) also needs to handle that? Is there anything we could do to help remembering to handle? #Resolved

I haven't thought of an easy way to assert that _additionalLocals is always at the expected scope. If there are Visit methods that represent blocks but are not setting _additionalLocals, then there two possibilities. If the block is a top-level block, we should fail the assert in VisitArrayOrSpanCollectionExpression() that _additionalLocals is { }. Otherwise, _additionalLocals will represent the containing block of the actual block. And for the latter case, that should just mean that the inline array temporaries will have longer scope than is strictly necessary.

Adding assert to LocalRewriter.VisitCollectionExpression() that _additionalLocals is { } to increase the chance of catching other cases.

jcouv · 2023-08-15T20:21:38Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

                Debug.Assert(elementType.Equals(spanType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0], TypeCompareKind.AllIgnoreOptions));
+
+                if (collectionTypeKind == CollectionExpressionTypeKind.ReadOnlySpan &&
+                    ShouldUseRuntimeHelpersCreateSpan(node, elementType.Type))


I'm not sure what all the requirements to be able to emit as a blob are. Is there any case where emitting as a blob could fail (thus fall back to a plain array instantiation) but we could emit as an inline array below? #Closed

If the collection is a candidate to be emitted in the assembly data section - that is, if it is a ReadOnlySpan<T> where T is one of the expected primitive types, and the collection contains constant values only - then it will be emitted as global data. The only condition that should affect the generated code is whether the particular RuntimeHelpers.CreateSpan() optimization is available, but the result is still global data. See CollectionExpressionTests.RuntimeHelpers_CreateSpan for instance.

jcouv · 2023-08-15T20:26:53Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

-            if (elements.Length > 0
-                && !elements.Any(i => i is BoundCollectionExpressionSpreadElement)
-                && _compilation.Assembly.RuntimeSupportsInlineArrayTypes
-                && (!constructMethod.ReturnType.IsRefLikeType || constructMethod.Parameters[0].EffectiveScope == ScopedKind.ScopedValue))


There used to be conditions related to ref-ness. Are those no longer needed? #Closed

In lowering, we only need to check that the collection expression is targeting a span, has a known length, and is not empty, and the runtime supports inline arrays. If those conditions are met, we'll use an inline array. Any checks for ref safety are handled in ref safety pass before lowering.

jcouv · 2023-08-15T23:53:21Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs

-                Construct(ImmutableArray.Create(TypeWithAnnotations.Create(inlineArrayType), elementType));
-            return _factory.Call(
+            // or
+            // ... InlineArrayAsSpan<<>y__InlineArrayN<ElementType>, ElementType>(ref tmp, N)


I understand this is an implementation detail, but should this be mentioned in the spec?

should this be mentioned in the spec?

Which part are you referring to? That we are using an inline array?

jcouv · 2023-08-16T00:02:50Z

                static void Create1() { {{spanType}}<int> s = []; s.Report(); }

nit: I didn't follow. What's the benefit of moving the .Report() call inside the methods? #Closed

In reply to: 1679779399

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:2675 in f3c7566. [](commit_id = f3c7566, deletion_comment = False)

cston · 2023-08-16T00:18:17Z

                static void Create1() { {{spanType}}<int> s = []; s.Report(); }

With this change, a ref safety error will be reported returning the span instance since the collection expression has local scope.

In reply to: 1679779399

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:2675 in f3c7566. [](commit_id = f3c7566, deletion_comment = False)

jcouv · 2023-08-16T00:27:41Z

            if (targetFramework == TargetFramework.Net80)

nit: should we be using a rolling target framework, instead of locking on .NET 8? However, it looks like we don't have a turn-key TargetFramework for that...

In reply to: 1679797271

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:10198 in f3c7566. [](commit_id = f3c7566, deletion_comment = False)

cston · 2023-08-16T23:16:24Z

            if (targetFramework == TargetFramework.Net80)

Yes, we could use the latest target framework here instead, if we had such a definition.

In reply to: 1679797271

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:10198 in f3c7566. [](commit_id = f3c7566, deletion_comment = False)

RikkiGibson

LGTM overall, had some comments and only skimmed the tests.

RikkiGibson · 2023-08-17T18:29:19Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

+                    case CollectionExpressionTypeKind.CollectionBuilder:
+                        // For a ref struct type with a builder method, the scope of the collection
+                        // expression is the scope of an invocation of the builder method with the
+                        // collection expression as the span argument.


pseudocode might be more clear than prose here. e.g. RS rs = [x, y, z] is equivalent to RS rs = RSFactory.Create((ReadOnlySpan<...>)[x, y, z])

RikkiGibson · 2023-08-17T18:30:03Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

+                        if (constructMethod is not { Parameters: [{ RefKind: RefKind.None } parameter] })
+                        {
+                            // Unexpected construct method. Restrict the collection to local scope.
+                            return true;


we expect an error to be reported elsewhere for this?

We should have filtered out the unexpected collection builder method in Binder.GetCollectionBuilderMethod(). The case here should not be hit.

RikkiGibson · 2023-08-17T18:33:14Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

+            {
+                var collectionTypeKind = ConversionsBase.GetCollectionExpressionTypeKind(_compilation, expr.Type, out var elementType);
+
+                switch (collectionTypeKind)


does this handle spreads correctly? Maybe we never expect to get any of the below TypeKinds when spreads are present? Might be worth asserting.

A collection expression targeting a span is considered local scope, even if the collection includes spreads. See CollectionExpressionTests.RefSafety_Return_02 for example.

RikkiGibson · 2023-08-17T19:08:54Z

src/Compilers/Core/Portable/WellKnownMembers.cs

@@ -3466,6 +3466,17 @@ static WellKnownMembers()
                    (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Int32,
                    (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Int32,

+                 // System_ReadOnlySpan_T__op_Implicit_ReadOnlySpan_T_Array


I wasn't sure why we want this symbol in addition to ReadOnlySpan<T>..ctor(T[])

RikkiGibson · 2023-08-23T18:13:39Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_Block.cs

-            VisitStatementSubList(builder, node.Statements);
-
-            var additionalLocals = TemporaryArray<LocalSymbol>.Empty;
+            var previousLocals = _additionalLocals;


A comment about what we are trying to do might be appropriate, e.g. when _additionalLocals is null here, it means we are entering the outermost block of the current function. Symbols inserted to _additionalLocals by child statements should be put in this block.

RikkiGibson · 2023-08-23T18:25:25Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

+                """;
+            var comp = CreateCompilation(source, targetFramework: TargetFramework.Net80);
+            comp.VerifyEmitDiagnostics(
+                // (7,26): error CS9203: A collection expression of type 'Span<int>' cannot be used in this context because it may be exposed outside of the current scope.


Perhaps the diagnostic message should say something like exposed outside of the current function.

RikkiGibson · 2023-08-23T18:29:53Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

+
+            string sourceB = $$"""
+                using System.Collections.Generic;
+                {{(useUnsafe ? "unsafe" : "")}} class Program


I wasn't sure why we expected presence or absence of unsafe to affect the test.

unsafe shouldn't affect the test, but since we have some ref safety diagnostics that vary based on unsafe, it seemed useful to include an explicit test.

RikkiGibson · 2023-08-23T18:30:32Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

+                }
+                else
+                {
+                    verifier.VerifyIL("Program.ThreeItems<T>", """


do we have a tracking issue for adding an "unexpected heap alloc" warning here?

jcouv · 2023-08-23T21:23:03Z

src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs

+        {
+            // A non-empty collection expression with span type may be stored
+            // on the stack. In those cases the expression may have local scope.
+            if (expr.Type?.IsRefLikeType == true && expr.Elements.Length > 0)


nit: consider inverting if to flatten method #Resolved

jcouv · 2023-08-23T22:36:21Z

                    ReadOnlySpan<int> s = [1, 2, 3];

nit: could do away with ceremony (just a top-level statement is enough)

Refers to: src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs:12990 in 7feae25. [](commit_id = 7feae25, deletion_comment = False)

jcouv

LGTM Thanks (iteration 9)

RikkiGibson · 2023-08-24T17:14:48Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

+                {
+                    public static T[] ToArray(ReadOnlySpan<T> s) => s.ToArray();
+                }
+                record class B<T>(T x, T y, T z) : A<T>(ToArray([x, y, z]));


nit: it feels like the test is not verifying the value of what is actually passed to the base constructor.

nevermind, we can trust ToArray to give an equivalent collection as the span. I just didn't have my records brain switched on when I reviewed this.

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 5, 2023

This was referenced Aug 5, 2023

Collection expressions: use inline array for spans in local initializers and method arguments when possible #69291

Closed

Collection expressions: use inline array for span argument when the argument cannot escape #69348

Closed

cston force-pushed the stack-allocation-2 branch from ef70352 to e72459d Compare August 5, 2023 18:01

Collection expressions: use inline arrays for collection expressions …

cd306e0

…with span target types

cston force-pushed the stack-allocation-2 branch from 7a0d691 to cd306e0 Compare August 6, 2023 05:32

cston marked this pull request as ready for review August 6, 2023 05:37

cston requested a review from a team as a code owner August 6, 2023 05:37

cston added 2 commits August 6, 2023 08:20

Cleanup

a2acbc8

Update ref analysis for builder methods

7fce767

jaredpar added this to the 17.8 milestone Aug 7, 2023

cston added 2 commits August 8, 2023 10:36

Merge remote-tracking branch 'upstream/main' into stack-allocation-2

f7f1fa8

Additional tests

9803f2e

jaredpar added New Feature - Collection Expressions and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 9, 2023

Merge remote-tracking branch 'upstream/main' into stack-allocation-2

eda04a9

cston mentioned this pull request Aug 9, 2023

Implement optimizations for constructing collections from collection literals #68785

Open

17 tasks

jcouv self-assigned this Aug 10, 2023

Collection expressions: Use RuntimeHelpers.CreateSpan optimization wh…

f3c7566

…en possible

jcouv reviewed Aug 11, 2023

View reviewed changes

jcouv reviewed Aug 15, 2023

View reviewed changes

Add comment

1031821

cston requested review from RikkiGibson and a team August 16, 2023 23:24

RikkiGibson self-assigned this Aug 17, 2023

Move inline array temporaries to outermost scope

7feae25

RikkiGibson reviewed Aug 23, 2023

View reviewed changes

jcouv reviewed Aug 23, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into stack-allocation-2

9ee705d

jcouv approved these changes Aug 23, 2023

View reviewed changes

Address feedback

09355c9

This was referenced Aug 24, 2023

Remove WellKnownMembers.System_ReadOnlySpan_T__op_Implicit_ReadOnlySpan_T_Array if not needed #69692

Closed

Collection expressions: Report warning when the compiler generates a heap allocation for a ref struct #69693

Closed

cston added 2 commits August 23, 2023 20:36

Merge remote-tracking branch 'upstream/main' into stack-allocation-2

377194c

Fix test

4d29bb0

RikkiGibson reviewed Aug 24, 2023

View reviewed changes

RikkiGibson approved these changes Aug 24, 2023

View reviewed changes

cston merged commit a830649 into dotnet:main Aug 24, 2023
25 checks passed

cston deleted the stack-allocation-2 branch August 24, 2023 17:27

ghost modified the milestones: 17.8, Next Aug 24, 2023

dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023

stephentoub mentioned this pull request Oct 14, 2023

Replace some stackallocs with collection expressions dotnet/runtime#93126

Merged

Tanya-Solyanik mentioned this pull request Nov 3, 2023

Use collection expressions to init spans where possible dotnet/winforms#10114

Merged

Tanya-Solyanik mentioned this pull request Nov 19, 2023

unpin the csc version, use compiler that comes with the arcade instead dotnet/winforms#10335

Merged

Collection expressions: use inline arrays for collection expressions with span target types #69412

Collection expressions: use inline arrays for collection expressions with span target types #69412

Conversation

cston commented Aug 5, 2023 • edited Loading

jcouv Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Aug 15, 2023 • edited by cston Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv commented Aug 16, 2023 • edited Loading

cston commented Aug 16, 2023

jcouv commented Aug 16, 2023 • edited by cston Loading

cston commented Aug 16, 2023

RikkiGibson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Aug 23, 2023 • edited by cston Loading

Choose a reason for hiding this comment

jcouv commented Aug 23, 2023

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

cston commented Aug 5, 2023 •

edited

Loading

jcouv Aug 11, 2023 •

edited

Loading

jcouv Aug 15, 2023 •

edited by cston

Loading

jcouv Aug 15, 2023 •

edited

Loading

jcouv Aug 15, 2023 •

edited

Loading

jcouv commented Aug 16, 2023 •

edited

Loading

jcouv commented Aug 16, 2023 •

edited by cston

Loading

jcouv Aug 23, 2023 •

edited by cston

Loading

RikkiGibson Aug 24, 2023 •

edited

Loading