From d5eb62541f86099345cdcd33d7f6cf89a92f84a8 Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Fri, 19 Apr 2024 15:33:52 +0300 Subject: [PATCH 01/12] Optimize codegen for collections expression of single spread of `ReadOnlySpan` in case of collection builder emit strategy --- .../LocalRewriter_CollectionExpression.cs | 32 ++- .../Semantics/CollectionExpressionTests.cs | 251 +++++++++++++++++- 2 files changed, 266 insertions(+), 17 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs index ef6e145345c23..5bf0087694e4b 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs @@ -56,8 +56,13 @@ private BoundExpression RewriteCollectionExpressionConversion(Conversion convers case CollectionExpressionTypeKind.CollectionBuilder: // If the collection type is ImmutableArray, then construction is optimized to use // ImmutableCollectionsMarshal.AsImmutableArray. + // The only exception is when collection expression is just `[.. readOnlySpan]` of the same element type, in such cases + // it is more efficient to emit a direct call of `ImmutableArray.Create` if (ConversionsBase.IsSpanOrListType(_compilation, node.Type, WellKnownType.System_Collections_Immutable_ImmutableArray_T, out var arrayElementType) && - _compilation.GetWellKnownTypeMember(WellKnownMember.System_Runtime_InteropServices_ImmutableCollectionsMarshal__AsImmutableArray_T) is MethodSymbol asImmutableArray) + _compilation.GetWellKnownTypeMember(WellKnownMember.System_Runtime_InteropServices_ImmutableCollectionsMarshal__AsImmutableArray_T) is MethodSymbol asImmutableArray && + !(node is { Elements: [BoundCollectionExpressionSpreadElement { Expression.Type: NamedTypeSymbol { TypeArgumentsWithAnnotationsNoUseSiteDiagnostics: [var spreadElementType] } spreadType }] } && + spreadType.OriginalDefinition == (object)_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T) && + spreadElementType.Equals(arrayElementType, TypeCompareKind.CLRSignatureCompareOptions))) { return VisitImmutableArrayCollectionExpression(node, arrayElementType, asImmutableArray); } @@ -177,14 +182,25 @@ private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpr return _factory.Default(collectionType); } - if (collectionTypeKind == CollectionExpressionTypeKind.ReadOnlySpan && - ShouldUseRuntimeHelpersCreateSpan(node, elementType.Type)) + if (collectionTypeKind == CollectionExpressionTypeKind.ReadOnlySpan) { - // Assert that binding layer agrees with lowering layer about whether this collection-expr will allocate. - Debug.Assert(!IsAllocatingRefStructCollectionExpression(node, collectionTypeKind, elementType.Type, _compilation)); - var constructor = ((MethodSymbol)_factory.WellKnownMember(WellKnownMember.System_ReadOnlySpan_T__ctor_Array)).AsMember(spanType); - var rewrittenElements = elements.SelectAsArray(static (element, rewriter) => rewriter.VisitExpression((BoundExpression)element), this); - return _factory.New(constructor, _factory.Array(elementType.Type, rewrittenElements)); + // If collection expression os of form `[.. anotherReadOnlySpan]` + // with `anotherReadOnlySpan` being a ReadOnlySpan of the same type as target collection type + // we can directly return `anotherReadOnlySpan` since we know that ReadOnlySpan is an immutable type + if (node is { Elements: [BoundCollectionExpressionSpreadElement { Expression: { Type: { } spreadType } spreadExpression }] } && + spreadType.Equals(collectionType, TypeCompareKind.CLRSignatureCompareOptions)) + { + return spreadExpression; + } + + if (ShouldUseRuntimeHelpersCreateSpan(node, elementType.Type)) + { + // Assert that binding layer agrees with lowering layer about whether this collection-expr will allocate. + Debug.Assert(!IsAllocatingRefStructCollectionExpression(node, collectionTypeKind, elementType.Type, _compilation)); + var constructor = ((MethodSymbol)_factory.WellKnownMember(WellKnownMember.System_ReadOnlySpan_T__ctor_Array)).AsMember(spanType); + var rewrittenElements = elements.SelectAsArray(static (element, rewriter) => rewriter.VisitExpression((BoundExpression)element), this); + return _factory.New(constructor, _factory.Array(elementType.Type, rewrittenElements)); + } } if (ShouldUseInlineArray(node, _compilation) && diff --git a/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs b/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs index 87f26b8e0675b..3fb38898f62a5 100644 --- a/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs +++ b/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs @@ -8188,16 +8188,14 @@ .locals init (System.ReadOnlySpan V_0) ("ReadOnlySpan", "ReadOnlySpan") => """ { - // Code size 22 (0x16) - .maxstack 2 + // Code size 10 (0xa) + .maxstack 1 .locals init (System.ReadOnlySpan V_0) //y - IL_0000: ldloca.s V_0 - IL_0002: ldarga.s V_0 - IL_0004: call "int[] System.ReadOnlySpan.ToArray()" - IL_0009: call "System.ReadOnlySpan..ctor(int[])" - IL_000e: ldloca.s V_0 - IL_0010: call "void CollectionExtensions.Report(in System.ReadOnlySpan)" - IL_0015: ret + IL_0000: ldarg.0 + IL_0001: stloc.0 + IL_0002: ldloca.s V_0 + IL_0004: call "void CollectionExtensions.Report(in System.ReadOnlySpan)" + IL_0009: ret } """, _ => null @@ -9230,6 +9228,146 @@ static void Main() Diagnostic(ErrorCode.ERR_AnonMethGrpInForEach, "Main").WithArguments("method group").WithLocation(5, 22)); } + [Fact] + public void SpreadElement_15() + { + string source = """ + using System; + using System.Collections; + using System.Collections.Generic; + using System.Runtime.CompilerServices; + + [CollectionBuilder(typeof(MyCollectionBuilder), nameof(MyCollectionBuilder.Create))] + class MyCollection : IEnumerable + { + private readonly T[] _arr; + + public IEnumerator GetEnumerator() => ((IEnumerable)_arr).GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + + public MyCollection(T[] arr) + { + _arr = arr; + } + } + + class MyCollectionBuilder + { + public static MyCollection Create(ReadOnlySpan span) => new(span.ToArray()); + } + + class Program + { + static void Main() + { + M([1, 2, 3]).Report(); + } + + static MyCollection M(ReadOnlySpan span) => [.. span]; + } + """; + + var verifier = CompileAndVerify([source, s_collectionExtensions], targetFramework: TargetFramework.Net80, expectedOutput: IncludeExpectedOutput("[1, 2, 3], "), verify: Verification.Skipped); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("Program.M", """ + { + // Code size 7 (0x7) + .maxstack 1 + IL_0000: ldarg.0 + IL_0001: call "MyCollection MyCollectionBuilder.Create(System.ReadOnlySpan)" + IL_0006: ret + } + """); + } + + [Fact] + public void SpreadElement_16() + { + string source = """ + using System; + using System.Collections; + using System.Collections.Generic; + using System.Runtime.CompilerServices; + + [CollectionBuilder(typeof(MyCollectionBuilder), nameof(MyCollectionBuilder.Create))] + class MyCollection : IEnumerable + { + private readonly T[] _arr; + + public IEnumerator GetEnumerator() => ((IEnumerable)_arr).GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + + public MyCollection(T[] arr) + { + _arr = arr; + } + } + + class MyCollectionBuilder + { + public static MyCollection Create(ReadOnlySpan span) => new(span.ToArray()); + } + + class Program + { + static void Main() + { + M([1, 2, 3]).Report(); + } + + static MyCollection M(ReadOnlySpan span) => [.. span]; + } + """; + + var verifier = CompileAndVerify([source, s_collectionExtensions], targetFramework: TargetFramework.Net80, expectedOutput: IncludeExpectedOutput("[1, 2, 3], "), verify: Verification.Skipped); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("Program.M", """ + { + // Code size 72 (0x48) + .maxstack 3 + .locals init (System.ReadOnlySpan V_0, + int V_1, + object[] V_2, + System.ReadOnlySpan.Enumerator V_3, + int V_4) + IL_0000: ldarg.0 + IL_0001: stloc.0 + IL_0002: ldc.i4.0 + IL_0003: stloc.1 + IL_0004: ldloca.s V_0 + IL_0006: call "int System.ReadOnlySpan.Length.get" + IL_000b: newarr "object" + IL_0010: stloc.2 + IL_0011: ldloca.s V_0 + IL_0013: call "System.ReadOnlySpan.Enumerator System.ReadOnlySpan.GetEnumerator()" + IL_0018: stloc.3 + IL_0019: br.s IL_0033 + IL_001b: ldloca.s V_3 + IL_001d: call "ref readonly int System.ReadOnlySpan.Enumerator.Current.get" + IL_0022: ldind.i4 + IL_0023: stloc.s V_4 + IL_0025: ldloc.2 + IL_0026: ldloc.1 + IL_0027: ldloc.s V_4 + IL_0029: box "int" + IL_002e: stelem.ref + IL_002f: ldloc.1 + IL_0030: ldc.i4.1 + IL_0031: add + IL_0032: stloc.1 + IL_0033: ldloca.s V_3 + IL_0035: call "bool System.ReadOnlySpan.Enumerator.MoveNext()" + IL_003a: brtrue.s IL_001b + IL_003c: ldloc.2 + IL_003d: newobj "System.ReadOnlySpan..ctor(object[])" + IL_0042: call "MyCollection MyCollectionBuilder.Create(System.ReadOnlySpan)" + IL_0047: ret + } + """); + } + [Fact] public void SpreadElement_Dynamic_02() { @@ -28547,6 +28685,101 @@ .locals init (System.Collections.Immutable.ImmutableArray V_0, //arr """); } + [Fact] + public void ImmutableArray_09() + { + string sourceA = """ + using System; + using System.Collections.Immutable; + + class Program + { + static void Main() + { + M([1, 2, 3]).Report(); + } + + static ImmutableArray M(ReadOnlySpan span) => [.. span]; + } + """; + + var verifier = CompileAndVerify([sourceA, s_collectionExtensions], targetFramework: TargetFramework.Net80, expectedOutput: IncludeExpectedOutput("[1, 2, 3],"), verify: Verification.Skipped); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("Program.M", """ + { + // Code size 7 (0x7) + .maxstack 1 + IL_0000: ldarg.0 + IL_0001: call "System.Collections.Immutable.ImmutableArray System.Collections.Immutable.ImmutableArray.Create(System.ReadOnlySpan)" + IL_0006: ret + } + """); + } + + [Fact] + public void ImmutableArray_10() + { + string sourceA = """ + using System; + using System.Collections.Immutable; + + class Program + { + static void Main() + { + M([1, 2, 3]).Report(); + } + + static ImmutableArray M(ReadOnlySpan span) => [.. span]; + } + """; + + var verifier = CompileAndVerify([sourceA, s_collectionExtensions], targetFramework: TargetFramework.Net80, expectedOutput: IncludeExpectedOutput("[1, 2, 3],"), verify: Verification.Skipped); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("Program.M", """ + { + // Code size 67 (0x43) + .maxstack 3 + .locals init (System.ReadOnlySpan V_0, + int V_1, + object[] V_2, + System.ReadOnlySpan.Enumerator V_3, + int V_4) + IL_0000: ldarg.0 + IL_0001: stloc.0 + IL_0002: ldc.i4.0 + IL_0003: stloc.1 + IL_0004: ldloca.s V_0 + IL_0006: call "int System.ReadOnlySpan.Length.get" + IL_000b: newarr "object" + IL_0010: stloc.2 + IL_0011: ldloca.s V_0 + IL_0013: call "System.ReadOnlySpan.Enumerator System.ReadOnlySpan.GetEnumerator()" + IL_0018: stloc.3 + IL_0019: br.s IL_0033 + IL_001b: ldloca.s V_3 + IL_001d: call "ref readonly int System.ReadOnlySpan.Enumerator.Current.get" + IL_0022: ldind.i4 + IL_0023: stloc.s V_4 + IL_0025: ldloc.2 + IL_0026: ldloc.1 + IL_0027: ldloc.s V_4 + IL_0029: box "int" + IL_002e: stelem.ref + IL_002f: ldloc.1 + IL_0030: ldc.i4.1 + IL_0031: add + IL_0032: stloc.1 + IL_0033: ldloca.s V_3 + IL_0035: call "bool System.ReadOnlySpan.Enumerator.MoveNext()" + IL_003a: brtrue.s IL_001b + IL_003c: ldloc.2 + IL_003d: call "System.Collections.Immutable.ImmutableArray System.Runtime.InteropServices.ImmutableCollectionsMarshal.AsImmutableArray(object[])" + IL_0042: ret + } + """); + } + [Fact] public void SpanImplicitAllocationWarning_01() { From 60457cf9f33a662903d498ce0d81760e2051f534 Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Fri, 19 Apr 2024 20:47:38 +0300 Subject: [PATCH 02/12] Ensure copying semantics when directly constructing ROS --- .../LocalRewriter_CollectionExpression.cs | 16 ++++--- .../Semantics/CollectionExpressionTests.cs | 46 ++++++++++++++++--- 2 files changed, 48 insertions(+), 14 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs index 5bf0087694e4b..91e2f0cee6e8e 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs @@ -52,7 +52,7 @@ private BoundExpression RewriteCollectionExpressionConversion(Conversion convers case CollectionExpressionTypeKind.Span: case CollectionExpressionTypeKind.ReadOnlySpan: Debug.Assert(elementType is { }); - return VisitArrayOrSpanCollectionExpression(node, collectionTypeKind, node.Type, TypeWithAnnotations.Create(elementType)); + return VisitArrayOrSpanCollectionExpression(node, collectionTypeKind, node.Type, TypeWithAnnotations.Create(elementType), canReuseSpan: false); case CollectionExpressionTypeKind.CollectionBuilder: // If the collection type is ImmutableArray, then construction is optimized to use // ImmutableCollectionsMarshal.AsImmutableArray. @@ -149,12 +149,13 @@ private BoundExpression VisitImmutableArrayCollectionExpression(BoundCollectionE node, CollectionExpressionTypeKind.Array, ArrayTypeSymbol.CreateSZArray(_compilation.Assembly, elementType), - elementType); + elementType, + canReuseSpan: false); // ImmutableCollectionsMarshal.AsImmutableArray(arrayCreation) return _factory.StaticCall(asImmutableArray.Construct(ImmutableArray.Create(elementType)), ImmutableArray.Create(arrayCreation)); } - private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpression node, CollectionExpressionTypeKind collectionTypeKind, TypeSymbol collectionType, TypeWithAnnotations elementType) + private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpression node, CollectionExpressionTypeKind collectionTypeKind, TypeSymbol collectionType, TypeWithAnnotations elementType, bool canReuseSpan) { Debug.Assert(!_inExpressionLambda); Debug.Assert(_additionalLocals is { }); @@ -184,10 +185,11 @@ private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpr if (collectionTypeKind == CollectionExpressionTypeKind.ReadOnlySpan) { - // If collection expression os of form `[.. anotherReadOnlySpan]` + // If collection expression is of form `[.. anotherReadOnlySpan]` // with `anotherReadOnlySpan` being a ReadOnlySpan of the same type as target collection type - // we can directly return `anotherReadOnlySpan` since we know that ReadOnlySpan is an immutable type - if (node is { Elements: [BoundCollectionExpressionSpreadElement { Expression: { Type: { } spreadType } spreadExpression }] } && + // we can directly return `anotherReadOnlySpan` since we know that ReadOnlySpan is an immutable type. + if (canReuseSpan && + node is { Elements: [BoundCollectionExpressionSpreadElement { Expression: { Type: { } spreadType } spreadExpression }] } && spreadType.Equals(collectionType, TypeCompareKind.CLRSignatureCompareOptions)) { return spreadExpression; @@ -392,7 +394,7 @@ private BoundExpression VisitCollectionBuilderCollectionExpression(BoundCollecti Debug.Assert(spanType.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions)); var elementType = spanType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0]; - BoundExpression span = VisitArrayOrSpanCollectionExpression(node, CollectionExpressionTypeKind.ReadOnlySpan, spanType, elementType); + BoundExpression span = VisitArrayOrSpanCollectionExpression(node, CollectionExpressionTypeKind.ReadOnlySpan, spanType, elementType, canReuseSpan: true); var invocation = new BoundCall( node.Syntax, diff --git a/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs b/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs index 3fb38898f62a5..c9e68504d220c 100644 --- a/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs +++ b/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs @@ -8188,14 +8188,16 @@ .locals init (System.ReadOnlySpan V_0) ("ReadOnlySpan", "ReadOnlySpan") => """ { - // Code size 10 (0xa) - .maxstack 1 + // Code size 22 (0x16) + .maxstack 2 .locals init (System.ReadOnlySpan V_0) //y - IL_0000: ldarg.0 - IL_0001: stloc.0 - IL_0002: ldloca.s V_0 - IL_0004: call "void CollectionExtensions.Report(in System.ReadOnlySpan)" - IL_0009: ret + IL_0000: ldloca.s V_0 + IL_0002: ldarga.s V_0 + IL_0004: call "int[] System.ReadOnlySpan.ToArray()" + IL_0009: call "System.ReadOnlySpan..ctor(int[])" + IL_000e: ldloca.s V_0 + IL_0010: call "void CollectionExtensions.Report(in System.ReadOnlySpan)" + IL_0015: ret } """, _ => null @@ -23958,6 +23960,36 @@ .locals init (<>y__InlineArray3 V_0) """); } + [Fact] + public void ReadOnlySpan_EnsureCopyingSemantics() + { + string source = """ + using System; + + class Program + { + static void Main() + { + int[] arr = { 1, 2, 3 }; + ReadOnlySpan span1 = arr; + ReadOnlySpan span2 = [.. span1]; + + arr[1] = 4; + + span1.Report(); + span2.Report(); + } + } + """; + + var verifier = CompileAndVerify( + [source, s_collectionExtensionsWithSpan], + targetFramework: TargetFramework.Net80, + verify: Verification.Skipped, + expectedOutput: IncludeExpectedOutput("[1, 4, 3], [1, 2, 3], ")); + verifier.VerifyDiagnostics(); + } + [Fact] public void RuntimeHelpers_CreateSpan_MissingCreateSpan() { From 2ff4c44f44dad9c2e3d8895931494f4318fc55b4 Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Fri, 19 Apr 2024 22:00:04 +0300 Subject: [PATCH 03/12] Move check up --- .../LocalRewriter_CollectionExpression.cs | 50 ++++++++++--------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs index 91e2f0cee6e8e..711c74780de73 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs @@ -52,7 +52,7 @@ private BoundExpression RewriteCollectionExpressionConversion(Conversion convers case CollectionExpressionTypeKind.Span: case CollectionExpressionTypeKind.ReadOnlySpan: Debug.Assert(elementType is { }); - return VisitArrayOrSpanCollectionExpression(node, collectionTypeKind, node.Type, TypeWithAnnotations.Create(elementType), canReuseSpan: false); + return VisitArrayOrSpanCollectionExpression(node, collectionTypeKind, node.Type, TypeWithAnnotations.Create(elementType)); case CollectionExpressionTypeKind.CollectionBuilder: // If the collection type is ImmutableArray, then construction is optimized to use // ImmutableCollectionsMarshal.AsImmutableArray. @@ -149,13 +149,12 @@ private BoundExpression VisitImmutableArrayCollectionExpression(BoundCollectionE node, CollectionExpressionTypeKind.Array, ArrayTypeSymbol.CreateSZArray(_compilation.Assembly, elementType), - elementType, - canReuseSpan: false); + elementType); // ImmutableCollectionsMarshal.AsImmutableArray(arrayCreation) return _factory.StaticCall(asImmutableArray.Construct(ImmutableArray.Create(elementType)), ImmutableArray.Create(arrayCreation)); } - private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpression node, CollectionExpressionTypeKind collectionTypeKind, TypeSymbol collectionType, TypeWithAnnotations elementType, bool canReuseSpan) + private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpression node, CollectionExpressionTypeKind collectionTypeKind, TypeSymbol collectionType, TypeWithAnnotations elementType) { Debug.Assert(!_inExpressionLambda); Debug.Assert(_additionalLocals is { }); @@ -183,26 +182,14 @@ private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpr return _factory.Default(collectionType); } - if (collectionTypeKind == CollectionExpressionTypeKind.ReadOnlySpan) + if (collectionTypeKind == CollectionExpressionTypeKind.ReadOnlySpan && + ShouldUseRuntimeHelpersCreateSpan(node, elementType.Type)) { - // If collection expression is of form `[.. anotherReadOnlySpan]` - // with `anotherReadOnlySpan` being a ReadOnlySpan of the same type as target collection type - // we can directly return `anotherReadOnlySpan` since we know that ReadOnlySpan is an immutable type. - if (canReuseSpan && - node is { Elements: [BoundCollectionExpressionSpreadElement { Expression: { Type: { } spreadType } spreadExpression }] } && - spreadType.Equals(collectionType, TypeCompareKind.CLRSignatureCompareOptions)) - { - return spreadExpression; - } - - if (ShouldUseRuntimeHelpersCreateSpan(node, elementType.Type)) - { - // Assert that binding layer agrees with lowering layer about whether this collection-expr will allocate. - Debug.Assert(!IsAllocatingRefStructCollectionExpression(node, collectionTypeKind, elementType.Type, _compilation)); - var constructor = ((MethodSymbol)_factory.WellKnownMember(WellKnownMember.System_ReadOnlySpan_T__ctor_Array)).AsMember(spanType); - var rewrittenElements = elements.SelectAsArray(static (element, rewriter) => rewriter.VisitExpression((BoundExpression)element), this); - return _factory.New(constructor, _factory.Array(elementType.Type, rewrittenElements)); - } + // Assert that binding layer agrees with lowering layer about whether this collection-expr will allocate. + Debug.Assert(!IsAllocatingRefStructCollectionExpression(node, collectionTypeKind, elementType.Type, _compilation)); + var constructor = ((MethodSymbol)_factory.WellKnownMember(WellKnownMember.System_ReadOnlySpan_T__ctor_Array)).AsMember(spanType); + var rewrittenElements = elements.SelectAsArray(static (element, rewriter) => rewriter.VisitExpression((BoundExpression)element), this); + return _factory.New(constructor, _factory.Array(elementType.Type, rewrittenElements)); } if (ShouldUseInlineArray(node, _compilation) && @@ -394,7 +381,22 @@ private BoundExpression VisitCollectionBuilderCollectionExpression(BoundCollecti Debug.Assert(spanType.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions)); var elementType = spanType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0]; - BoundExpression span = VisitArrayOrSpanCollectionExpression(node, CollectionExpressionTypeKind.ReadOnlySpan, spanType, elementType, canReuseSpan: true); + BoundExpression span; + + // If collection expression is of form `[.. anotherReadOnlySpan]` + // with `anotherReadOnlySpan` being a ReadOnlySpan of the same type as target collection type + // we can directly use `anotherReadOnlySpan` as collection builder argument + // since we know that ReadOnlySpan is an immutable type. + if (node is { Elements: [BoundCollectionExpressionSpreadElement { Expression: { Type: NamedTypeSymbol { TypeArgumentsWithAnnotationsNoUseSiteDiagnostics: [var spreadElementType] } spreadType } spreadExpression }] } && + spreadType.OriginalDefinition == (object)_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T) && + spreadElementType.Equals(elementType, TypeCompareKind.CLRSignatureCompareOptions)) + { + span = spreadExpression; + } + else + { + span = VisitArrayOrSpanCollectionExpression(node, CollectionExpressionTypeKind.ReadOnlySpan, spanType, elementType); + } var invocation = new BoundCall( node.Syntax, From 368cad4dddf6007a1ff7aa6c8209308d1b23e5ee Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Fri, 19 Apr 2024 22:08:41 +0300 Subject: [PATCH 04/12] Extract common logic --- .../LocalRewriter_CollectionExpression.cs | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs index 711c74780de73..bdae62b0d7744 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs @@ -60,9 +60,7 @@ private BoundExpression RewriteCollectionExpressionConversion(Conversion convers // it is more efficient to emit a direct call of `ImmutableArray.Create` if (ConversionsBase.IsSpanOrListType(_compilation, node.Type, WellKnownType.System_Collections_Immutable_ImmutableArray_T, out var arrayElementType) && _compilation.GetWellKnownTypeMember(WellKnownMember.System_Runtime_InteropServices_ImmutableCollectionsMarshal__AsImmutableArray_T) is MethodSymbol asImmutableArray && - !(node is { Elements: [BoundCollectionExpressionSpreadElement { Expression.Type: NamedTypeSymbol { TypeArgumentsWithAnnotationsNoUseSiteDiagnostics: [var spreadElementType] } spreadType }] } && - spreadType.OriginalDefinition == (object)_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T) && - spreadElementType.Equals(arrayElementType, TypeCompareKind.CLRSignatureCompareOptions))) + !IsSingleReadOnlySpanSpread(node, arrayElementType, out _)) { return VisitImmutableArrayCollectionExpression(node, arrayElementType, asImmutableArray); } @@ -143,6 +141,20 @@ static BoundNode unwrapListElement(BoundCollectionExpression node, BoundNode ele } } + private bool IsSingleReadOnlySpanSpread(BoundCollectionExpression node, TypeWithAnnotations elementType, [NotNullWhen(true)] out BoundExpression? spreadExpression) + { + spreadExpression = null; + + if (node is { Elements: [BoundCollectionExpressionSpreadElement { Expression: { Type: NamedTypeSymbol { TypeArgumentsWithAnnotationsNoUseSiteDiagnostics: [var spreadElementType] } spreadType } expr }] } && + spreadType.OriginalDefinition == (object)_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T) && + spreadElementType.Equals(elementType, TypeCompareKind.CLRSignatureCompareOptions)) + { + spreadExpression = expr; + } + + return spreadExpression is not null; + } + private BoundExpression VisitImmutableArrayCollectionExpression(BoundCollectionExpression node, TypeWithAnnotations elementType, MethodSymbol asImmutableArray) { var arrayCreation = VisitArrayOrSpanCollectionExpression( @@ -381,22 +393,14 @@ private BoundExpression VisitCollectionBuilderCollectionExpression(BoundCollecti Debug.Assert(spanType.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions)); var elementType = spanType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0]; - BoundExpression span; // If collection expression is of form `[.. anotherReadOnlySpan]` // with `anotherReadOnlySpan` being a ReadOnlySpan of the same type as target collection type // we can directly use `anotherReadOnlySpan` as collection builder argument // since we know that ReadOnlySpan is an immutable type. - if (node is { Elements: [BoundCollectionExpressionSpreadElement { Expression: { Type: NamedTypeSymbol { TypeArgumentsWithAnnotationsNoUseSiteDiagnostics: [var spreadElementType] } spreadType } spreadExpression }] } && - spreadType.OriginalDefinition == (object)_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T) && - spreadElementType.Equals(elementType, TypeCompareKind.CLRSignatureCompareOptions)) - { - span = spreadExpression; - } - else - { - span = VisitArrayOrSpanCollectionExpression(node, CollectionExpressionTypeKind.ReadOnlySpan, spanType, elementType); - } + BoundExpression span = IsSingleReadOnlySpanSpread(node, elementType, out var spreadExpression) + ? spreadExpression + : VisitArrayOrSpanCollectionExpression(node, CollectionExpressionTypeKind.ReadOnlySpan, spanType, elementType); var invocation = new BoundCall( node.Syntax, From 5a2c7a2d444edf1cb5a7ac8f4c74355ab66a7a04 Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Fri, 19 Apr 2024 22:45:45 +0300 Subject: [PATCH 05/12] Ref struct work --- .../LocalRewriter_CollectionExpression.cs | 7 +- .../Semantics/CollectionExpressionTests.cs | 198 ++++++++++++++++++ 2 files changed, 202 insertions(+), 3 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs index bdae62b0d7744..15b131b2a4c7c 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs @@ -396,9 +396,10 @@ private BoundExpression VisitCollectionBuilderCollectionExpression(BoundCollecti // If collection expression is of form `[.. anotherReadOnlySpan]` // with `anotherReadOnlySpan` being a ReadOnlySpan of the same type as target collection type - // we can directly use `anotherReadOnlySpan` as collection builder argument - // since we know that ReadOnlySpan is an immutable type. - BoundExpression span = IsSingleReadOnlySpanSpread(node, elementType, out var spreadExpression) + // and that span cannot be captured in a returned ref struct + // we can directly use `anotherReadOnlySpan` as collection builder argument and skip the copying assignment. + var canSkipCopyingArgument = !constructMethod.ReturnType.IsRefLikeType || constructMethod.Parameters[0].EffectiveScope == ScopedKind.ScopedValue; + BoundExpression span = canSkipCopyingArgument && IsSingleReadOnlySpanSpread(node, elementType, out var spreadExpression) ? spreadExpression : VisitArrayOrSpanCollectionExpression(node, CollectionExpressionTypeKind.ReadOnlySpan, spanType, elementType); diff --git a/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs b/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs index c9e68504d220c..f439524039b9d 100644 --- a/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs +++ b/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs @@ -20620,6 +20620,204 @@ public void RefStruct_04() ); } + [Fact] + public void RefStruct_EnsureCopyingSemanticsWhenReadOnlySpanParameterOfCollectionBuilderIsNotScoped() + { + string source = """ + using System; + using System.Runtime.CompilerServices; + + [CollectionBuilder(typeof(SpanWrap), "Create")] + ref struct SpanWrap + { + private readonly ReadOnlySpan _ros; + + public int Length => _ros.Length; + public T this[int index] => _ros[index]; + + public Enumerator GetEnumerator() => default; + + public SpanWrap(ReadOnlySpan ros) + { + _ros = ros; + } + + public ref struct Enumerator + { + public bool MoveNext() => default; + public T Current => default; + } + } + + static class SpanWrap + { + public static SpanWrap Create(ReadOnlySpan values) => new SpanWrap(values); + } + + class Program + { + static void Main() + { + int[] arr = { 1, 2, 3 }; + ReadOnlySpan span = arr; + SpanWrap spanWrap = [.. span]; + + arr[1] = 4; + + span.Report(); + ReportSpanWrap(spanWrap); + } + + static void ReportSpanWrap(SpanWrap spanWrap) + { + Console.Write('['); + + for (int i = 0; i < spanWrap.Length - 1; i++) + { + Console.Write(spanWrap[i] + ", "); + } + + Console.Write(spanWrap[^1]); + Console.Write(']'); + } + } + """; + + var verifier = CompileAndVerify( + [source, s_collectionExtensionsWithSpan], + targetFramework: TargetFramework.Net80, + verify: Verification.Skipped, + expectedOutput: IncludeExpectedOutput("[1, 4, 3], [1, 2, 3]")); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("Program.Main", """ + { + // Code size 59 (0x3b) + .maxstack 3 + .locals init (System.ReadOnlySpan V_0, //span + SpanWrap V_1) //spanWrap + IL_0000: ldc.i4.3 + IL_0001: newarr "int" + IL_0006: dup + IL_0007: ldtoken ".__StaticArrayInitTypeSize=12 .4636993D3E1DA4E9D6B8F87B79E8F7C6D018580D52661950EABC3845C5897A4D" + IL_000c: call "void System.Runtime.CompilerServices.RuntimeHelpers.InitializeArray(System.Array, System.RuntimeFieldHandle)" + IL_0011: dup + IL_0012: call "System.ReadOnlySpan System.ReadOnlySpan.op_Implicit(int[])" + IL_0017: stloc.0 + IL_0018: ldloca.s V_0 + IL_001a: call "int[] System.ReadOnlySpan.ToArray()" + IL_001f: newobj "System.ReadOnlySpan..ctor(int[])" + IL_0024: call "SpanWrap SpanWrap.Create(System.ReadOnlySpan)" + IL_0029: stloc.1 + IL_002a: ldc.i4.1 + IL_002b: ldc.i4.4 + IL_002c: stelem.i4 + IL_002d: ldloca.s V_0 + IL_002f: call "void CollectionExtensions.Report(in System.ReadOnlySpan)" + IL_0034: ldloc.1 + IL_0035: call "void Program.ReportSpanWrap(SpanWrap)" + IL_003a: ret + } + """); + } + + [Fact] + public void RefStruct_CanSkipCopyingReadOnlySpanWhenCollectionBuilderParameterIsScoped() + { + string source = """ + using System; + using System.Runtime.CompilerServices; + + [CollectionBuilder(typeof(ArrayWrap), "Create")] + ref struct ArrayWrap + { + private readonly T[] _arr; + + public int Length => _arr.Length; + public T this[int index] => _arr[index]; + + public Enumerator GetEnumerator() => default; + + public ArrayWrap(T[] arr) + { + _arr = arr; + } + + public ref struct Enumerator + { + public bool MoveNext() => default; + public T Current => default; + } + } + + static class ArrayWrap + { + public static ArrayWrap Create(scoped ReadOnlySpan values) => new ArrayWrap(values.ToArray()); + } + + class Program + { + static void Main() + { + int[] arr = { 1, 2, 3 }; + ReadOnlySpan span = arr; + ArrayWrap arrWrap = [.. span]; + + arr[1] = 4; + + span.Report(); + ReportArrayWrap(arrWrap); + } + + static void ReportArrayWrap(ArrayWrap arrWrap) + { + Console.Write('['); + + for (int i = 0; i < arrWrap.Length - 1; i++) + { + Console.Write(arrWrap[i] + ", "); + } + + Console.Write(arrWrap[^1]); + Console.Write(']'); + } + } + """; + + var verifier = CompileAndVerify( + [source, s_collectionExtensionsWithSpan], + targetFramework: TargetFramework.Net80, + verify: Verification.Skipped, + expectedOutput: IncludeExpectedOutput("[1, 4, 3], [1, 2, 3]")); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("Program.Main", """ + { + // Code size 48 (0x30) + .maxstack 3 + .locals init (System.ReadOnlySpan V_0, //span + ArrayWrap V_1) //arrWrap + IL_0000: ldc.i4.3 + IL_0001: newarr "int" + IL_0006: dup + IL_0007: ldtoken ".__StaticArrayInitTypeSize=12 .4636993D3E1DA4E9D6B8F87B79E8F7C6D018580D52661950EABC3845C5897A4D" + IL_000c: call "void System.Runtime.CompilerServices.RuntimeHelpers.InitializeArray(System.Array, System.RuntimeFieldHandle)" + IL_0011: dup + IL_0012: call "System.ReadOnlySpan System.ReadOnlySpan.op_Implicit(int[])" + IL_0017: stloc.0 + IL_0018: ldloc.0 + IL_0019: call "ArrayWrap ArrayWrap.Create(scoped System.ReadOnlySpan)" + IL_001e: stloc.1 + IL_001f: ldc.i4.1 + IL_0020: ldc.i4.4 + IL_0021: stelem.i4 + IL_0022: ldloca.s V_0 + IL_0024: call "void CollectionExtensions.Report(in System.ReadOnlySpan)" + IL_0029: ldloc.1 + IL_002a: call "void Program.ReportArrayWrap(ArrayWrap)" + IL_002f: ret + } + """); + } + [CombinatorialData] [Theory] public void RefSafety_Return_01([CombinatorialValues(TargetFramework.Net70, TargetFramework.Net80)] TargetFramework targetFramework) From bb722db74c453c5304167be34762da69a4842492 Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Sat, 20 Apr 2024 10:55:25 +0300 Subject: [PATCH 06/12] Add comment --- .../CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs b/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs index f439524039b9d..1fb761a494685 100644 --- a/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs +++ b/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs @@ -9286,6 +9286,7 @@ .maxstack 1 [Fact] public void SpreadElement_16() { + // spread operand element type differs from result collection element type string source = """ using System; using System.Collections; From a2ce5d84a45b82eb1df7731adce8adeccd56a756 Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Tue, 23 Apr 2024 17:05:50 +0300 Subject: [PATCH 07/12] Reorder checks --- .../LocalRewriter/LocalRewriter_CollectionExpression.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs index 15b131b2a4c7c..6545e170d7b27 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs @@ -398,8 +398,7 @@ private BoundExpression VisitCollectionBuilderCollectionExpression(BoundCollecti // with `anotherReadOnlySpan` being a ReadOnlySpan of the same type as target collection type // and that span cannot be captured in a returned ref struct // we can directly use `anotherReadOnlySpan` as collection builder argument and skip the copying assignment. - var canSkipCopyingArgument = !constructMethod.ReturnType.IsRefLikeType || constructMethod.Parameters[0].EffectiveScope == ScopedKind.ScopedValue; - BoundExpression span = canSkipCopyingArgument && IsSingleReadOnlySpanSpread(node, elementType, out var spreadExpression) + BoundExpression span = IsSingleReadOnlySpanSpread(node, elementType, out var spreadExpression) && (!constructMethod.ReturnType.IsRefLikeType || constructMethod.Parameters[0].EffectiveScope == ScopedKind.ScopedValue) ? spreadExpression : VisitArrayOrSpanCollectionExpression(node, CollectionExpressionTypeKind.ReadOnlySpan, spanType, elementType); From 41c4dfde5a292deb9472165fee1f478de3bc417a Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Tue, 23 Apr 2024 19:32:00 +0300 Subject: [PATCH 08/12] Add more tests --- .../Semantics/CollectionExpressionTests.cs | 266 +++++++++++++++++- 1 file changed, 265 insertions(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs b/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs index 1fb761a494685..06db3e26e9747 100644 --- a/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs +++ b/src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs @@ -9233,6 +9233,7 @@ static void Main() [Fact] public void SpreadElement_15() { + // Optimization: pass ReadOnlySpan directly to collection builder method without copying string source = """ using System; using System.Collections; @@ -9286,7 +9287,7 @@ .maxstack 1 [Fact] public void SpreadElement_16() { - // spread operand element type differs from result collection element type + // Spread operand element type differs from result collection element type by CLR signature string source = """ using System; using System.Collections; @@ -9371,6 +9372,269 @@ .locals init (System.ReadOnlySpan V_0, """); } + [Fact] + public void SpreadElement_17() + { + // 'dynamic' and 'object' are the same things during runtime, so apply 'no-copy' optimization + string source = """ + using System; + using System.Collections; + using System.Collections.Generic; + using System.Runtime.CompilerServices; + + [CollectionBuilder(typeof(MyCollectionBuilder), nameof(MyCollectionBuilder.Create))] + class MyCollection : IEnumerable + { + private readonly T[] _arr; + + public IEnumerator GetEnumerator() => ((IEnumerable)_arr).GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + + public MyCollection(T[] arr) + { + _arr = arr; + } + } + + class MyCollectionBuilder + { + public static MyCollection Create(ReadOnlySpan span) => new(span.ToArray()); + } + + class Program + { + static void Main() + { + M([1, 2, 3]).Report(); + } + + static MyCollection M(ReadOnlySpan span) => [.. span]; + } + """; + + var verifier = CompileAndVerify([source, s_collectionExtensions], targetFramework: TargetFramework.Net80, expectedOutput: IncludeExpectedOutput("[1, 2, 3], "), verify: Verification.Skipped); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("Program.M", """ + { + // Code size 7 (0x7) + .maxstack 1 + IL_0000: ldarg.0 + IL_0001: call "MyCollection MyCollectionBuilder.Create(System.ReadOnlySpan)" + IL_0006: ret + } + """); + } + + [Fact] + public void SpreadElement_18() + { + // Tuple element names only have effect in the code, + // thus '(int A, int B)' and just '(int, int)' are the same thing, + // so apply 'no-copy' optimization here as well + string source = """ + using System; + using System.Collections; + using System.Collections.Generic; + using System.Runtime.CompilerServices; + + [CollectionBuilder(typeof(MyCollectionBuilder), nameof(MyCollectionBuilder.Create))] + class MyCollection : IEnumerable + { + private readonly T[] _arr; + + public IEnumerator GetEnumerator() => ((IEnumerable)_arr).GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + + public MyCollection(T[] arr) + { + _arr = arr; + } + } + + class MyCollectionBuilder + { + public static MyCollection Create(ReadOnlySpan span) => new(span.ToArray()); + } + + class Program + { + static void Main() + { + M([(1, 1), (2, 2), (3, 3)]).Report(); + } + + static MyCollection<(int A, int B)> M(ReadOnlySpan<(int, int)> span) => [.. span]; + } + """; + + var verifier = CompileAndVerify([source, s_collectionExtensions], targetFramework: TargetFramework.Net80, expectedOutput: IncludeExpectedOutput("[(1, 1), (2, 2), (3, 3)], "), verify: Verification.Skipped); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("Program.M", """ + { + // Code size 7 (0x7) + .maxstack 1 + IL_0000: ldarg.0 + IL_0001: call "MyCollection> MyCollectionBuilder.Create>(System.ReadOnlySpan>)" + IL_0006: ret + } + """); + } + + [Fact] + public void SpreadElement_19() + { + // Spread operand element type differs from result collection element type by CLR signature + string source = """ + using System; + using System.Collections; + using System.Collections.Generic; + using System.Runtime.CompilerServices; + + [CollectionBuilder(typeof(MyCollectionBuilder), nameof(MyCollectionBuilder.Create))] + class MyCollection : IEnumerable + { + private readonly T[] _arr; + + public IEnumerator GetEnumerator() => ((IEnumerable)_arr).GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + + public MyCollection(T[] arr) + { + _arr = arr; + } + } + + class MyCollectionBuilder + { + public static MyCollection Create(ReadOnlySpan span) => new(span.ToArray()); + } + + class Program + { + static void Main() + { + M([(1, 1), (2, 2), (3, 3)]).Report(); + } + + static MyCollection<(object, object)> M(ReadOnlySpan<(int, int)> span) => [.. span]; + } + """; + + var verifier = CompileAndVerify([source, s_collectionExtensions], targetFramework: TargetFramework.Net80, expectedOutput: IncludeExpectedOutput("[(1, 1), (2, 2), (3, 3)], "), verify: Verification.Skipped); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("Program.M", """ + { + // Code size 106 (0x6a) + .maxstack 4 + .locals init (System.ReadOnlySpan> V_0, + int V_1, + System.ValueTuple[] V_2, + System.ReadOnlySpan>.Enumerator V_3, + System.ValueTuple V_4, + System.ValueTuple V_5) + IL_0000: ldarg.0 + IL_0001: stloc.0 + IL_0002: ldc.i4.0 + IL_0003: stloc.1 + IL_0004: ldloca.s V_0 + IL_0006: call "int System.ReadOnlySpan>.Length.get" + IL_000b: newarr "System.ValueTuple" + IL_0010: stloc.2 + IL_0011: ldloca.s V_0 + IL_0013: call "System.ReadOnlySpan>.Enumerator System.ReadOnlySpan>.GetEnumerator()" + IL_0018: stloc.3 + IL_0019: br.s IL_0055 + IL_001b: ldloca.s V_3 + IL_001d: call "ref readonly System.ValueTuple System.ReadOnlySpan>.Enumerator.Current.get" + IL_0022: ldobj "System.ValueTuple" + IL_0027: stloc.s V_4 + IL_0029: ldloc.2 + IL_002a: ldloc.1 + IL_002b: ldloc.s V_4 + IL_002d: stloc.s V_5 + IL_002f: ldloc.s V_5 + IL_0031: ldfld "int System.ValueTuple.Item1" + IL_0036: box "int" + IL_003b: ldloc.s V_5 + IL_003d: ldfld "int System.ValueTuple.Item2" + IL_0042: box "int" + IL_0047: newobj "System.ValueTuple..ctor(object, object)" + IL_004c: stelem "System.ValueTuple" + IL_0051: ldloc.1 + IL_0052: ldc.i4.1 + IL_0053: add + IL_0054: stloc.1 + IL_0055: ldloca.s V_3 + IL_0057: call "bool System.ReadOnlySpan>.Enumerator.MoveNext()" + IL_005c: brtrue.s IL_001b + IL_005e: ldloc.2 + IL_005f: newobj "System.ReadOnlySpan>..ctor(System.ValueTuple[])" + IL_0064: call "MyCollection> MyCollectionBuilder.Create>(System.ReadOnlySpan>)" + IL_0069: ret + } + """); + } + + [Fact] + public void SpreadElement_20() + { + // Nullability annotations only affect diagnostics and have no effect on the runtime, + // so 'T?[]' and 'T[]' are the same times, therefore apply 'no-copy' optimization here + string source = """ + #nullable enable + + using System; + using System.Collections; + using System.Collections.Generic; + using System.Runtime.CompilerServices; + + [CollectionBuilder(typeof(MyCollectionBuilder), nameof(MyCollectionBuilder.Create))] + class MyCollection : IEnumerable + { + private readonly T[] _arr; + + public IEnumerator GetEnumerator() => ((IEnumerable)_arr).GetEnumerator(); + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + + public MyCollection(T[] arr) + { + _arr = arr; + } + } + + class MyCollectionBuilder + { + public static MyCollection Create(ReadOnlySpan span) => new(span.ToArray()); + } + + class Program + { + static void Main() + { + M([[1], [2], [3]]).Report(); + } + + static MyCollection M(ReadOnlySpan span) => [.. span]; + } + """; + + var verifier = CompileAndVerify([source, s_collectionExtensions], targetFramework: TargetFramework.Net80, expectedOutput: IncludeExpectedOutput("[[1], [2], [3]], "), verify: Verification.Skipped); + verifier.VerifyDiagnostics(); + verifier.VerifyIL("Program.M", """ + { + // Code size 7 (0x7) + .maxstack 1 + IL_0000: ldarg.0 + IL_0001: call "MyCollection MyCollectionBuilder.Create(System.ReadOnlySpan)" + IL_0006: ret + } + """); + } + [Fact] public void SpreadElement_Dynamic_02() { From fd9403152fb420f4b204f62c2ce0bd0e98503a08 Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Tue, 23 Apr 2024 19:51:14 +0300 Subject: [PATCH 09/12] Replace type equality with conversion check --- .../LocalRewriter_CollectionExpression.cs | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs index 6545e170d7b27..68a7d580052de 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs @@ -60,7 +60,7 @@ private BoundExpression RewriteCollectionExpressionConversion(Conversion convers // it is more efficient to emit a direct call of `ImmutableArray.Create` if (ConversionsBase.IsSpanOrListType(_compilation, node.Type, WellKnownType.System_Collections_Immutable_ImmutableArray_T, out var arrayElementType) && _compilation.GetWellKnownTypeMember(WellKnownMember.System_Runtime_InteropServices_ImmutableCollectionsMarshal__AsImmutableArray_T) is MethodSymbol asImmutableArray && - !IsSingleReadOnlySpanSpread(node, arrayElementType, out _)) + !IsSingleReadOnlySpanSpread(node, out _)) { return VisitImmutableArrayCollectionExpression(node, arrayElementType, asImmutableArray); } @@ -141,13 +141,18 @@ static BoundNode unwrapListElement(BoundCollectionExpression node, BoundNode ele } } - private bool IsSingleReadOnlySpanSpread(BoundCollectionExpression node, TypeWithAnnotations elementType, [NotNullWhen(true)] out BoundExpression? spreadExpression) + private bool IsSingleReadOnlySpanSpread(BoundCollectionExpression node, [NotNullWhen(true)] out BoundExpression? spreadExpression) { spreadExpression = null; - if (node is { Elements: [BoundCollectionExpressionSpreadElement { Expression: { Type: NamedTypeSymbol { TypeArgumentsWithAnnotationsNoUseSiteDiagnostics: [var spreadElementType] } spreadType } expr }] } && - spreadType.OriginalDefinition == (object)_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T) && - spreadElementType.Equals(elementType, TypeCompareKind.CLRSignatureCompareOptions)) + if (node is + { + Elements: + [ + BoundCollectionExpressionSpreadElement { Expression: { Type: NamedTypeSymbol spreadType } expr, IteratorBody: not BoundExpressionStatement { Expression: BoundConversion { ConversionKind: not ConversionKind.Identity } } } + ] + } && + spreadType.OriginalDefinition == (object)_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T)) { spreadExpression = expr; } @@ -398,7 +403,7 @@ private BoundExpression VisitCollectionBuilderCollectionExpression(BoundCollecti // with `anotherReadOnlySpan` being a ReadOnlySpan of the same type as target collection type // and that span cannot be captured in a returned ref struct // we can directly use `anotherReadOnlySpan` as collection builder argument and skip the copying assignment. - BoundExpression span = IsSingleReadOnlySpanSpread(node, elementType, out var spreadExpression) && (!constructMethod.ReturnType.IsRefLikeType || constructMethod.Parameters[0].EffectiveScope == ScopedKind.ScopedValue) + BoundExpression span = IsSingleReadOnlySpanSpread(node, out var spreadExpression) && (!constructMethod.ReturnType.IsRefLikeType || constructMethod.Parameters[0].EffectiveScope == ScopedKind.ScopedValue) ? spreadExpression : VisitArrayOrSpanCollectionExpression(node, CollectionExpressionTypeKind.ReadOnlySpan, spanType, elementType); From 03158cd40e6178eceb54b1362432b434fc946de5 Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Tue, 23 Apr 2024 22:08:35 +0300 Subject: [PATCH 10/12] Reduce nesting --- .../LocalRewriter_CollectionExpression.cs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs index 68a7d580052de..efd89aafff1e1 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs @@ -145,13 +145,10 @@ private bool IsSingleReadOnlySpanSpread(BoundCollectionExpression node, [NotNull { spreadExpression = null; - if (node is - { - Elements: - [ - BoundCollectionExpressionSpreadElement { Expression: { Type: NamedTypeSymbol spreadType } expr, IteratorBody: not BoundExpressionStatement { Expression: BoundConversion { ConversionKind: not ConversionKind.Identity } } } - ] - } && + if (node.Elements is + [ + BoundCollectionExpressionSpreadElement { Expression: { Type: NamedTypeSymbol spreadType } expr, IteratorBody: not BoundExpressionStatement { Expression: BoundConversion { ConversionKind: not ConversionKind.Identity } } } + ] && spreadType.OriginalDefinition == (object)_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T)) { spreadExpression = expr; From 41afa8a3033aeba84256782f506f700d1d6a03e8 Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Thu, 25 Apr 2024 21:55:14 +0300 Subject: [PATCH 11/12] Strengthen condition --- .../LocalRewriter/LocalRewriter_CollectionExpression.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs index efd89aafff1e1..7f616793b9ff9 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs @@ -147,7 +147,7 @@ private bool IsSingleReadOnlySpanSpread(BoundCollectionExpression node, [NotNull if (node.Elements is [ - BoundCollectionExpressionSpreadElement { Expression: { Type: NamedTypeSymbol spreadType } expr, IteratorBody: not BoundExpressionStatement { Expression: BoundConversion { ConversionKind: not ConversionKind.Identity } } } + BoundCollectionExpressionSpreadElement { Expression: { Type: NamedTypeSymbol spreadType } expr, IteratorBody: BoundExpressionStatement { Expression: not BoundConversion { ConversionKind: not ConversionKind.Identity } } } ] && spreadType.OriginalDefinition == (object)_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T)) { From fec4b745e879b193659c339e45ae2143099eb98d Mon Sep 17 00:00:00 2001 From: DoctorKrolic Date: Wed, 10 Jul 2024 17:41:08 +0300 Subject: [PATCH 12/12] Feedback --- .../LocalRewriter_CollectionExpression.cs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs index 0fd77d4db1e22..f8ec243af6ba1 100644 --- a/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs +++ b/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs @@ -60,7 +60,7 @@ private BoundExpression RewriteCollectionExpressionConversion(Conversion convers // it is more efficient to emit a direct call of `ImmutableArray.Create` if (ConversionsBase.IsSpanOrListType(_compilation, node.Type, WellKnownType.System_Collections_Immutable_ImmutableArray_T, out var arrayElementType) && _compilation.GetWellKnownTypeMember(WellKnownMember.System_Runtime_InteropServices_ImmutableCollectionsMarshal__AsImmutableArray_T) is MethodSymbol asImmutableArray && - !IsSingleReadOnlySpanSpread(node, out _)) + !CanOptimizeSingleSpreadAsCollectionBuilderArgument(node, out _)) { return VisitImmutableArrayCollectionExpression(node, arrayElementType, asImmutableArray); } @@ -141,15 +141,17 @@ static BoundNode unwrapListElement(BoundCollectionExpression node, BoundNode ele } } - private bool IsSingleReadOnlySpanSpread(BoundCollectionExpression node, [NotNullWhen(true)] out BoundExpression? spreadExpression) + private static bool CanOptimizeSingleSpreadAsCollectionBuilderArgument(BoundCollectionExpression node, [NotNullWhen(true)] out BoundExpression? spreadExpression) { spreadExpression = null; - if (node.Elements is - [ - BoundCollectionExpressionSpreadElement { Expression: { Type: NamedTypeSymbol spreadType } expr, IteratorBody: BoundExpressionStatement { Expression: not BoundConversion { ConversionKind: not ConversionKind.Identity } } } - ] && - spreadType.OriginalDefinition == (object)_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T)) + if (node is + { + CollectionBuilderMethod: { } builder, + Elements: [BoundCollectionExpressionSpreadElement { Expression: { Type: NamedTypeSymbol spreadType } expr }], + } && + ConversionsBase.HasIdentityConversion(builder.Parameters[0].Type, spreadType) && + (!builder.ReturnType.IsRefLikeType || builder.Parameters[0].EffectiveScope == ScopedKind.ScopedValue)) { spreadExpression = expr; } @@ -399,7 +401,7 @@ private BoundExpression VisitCollectionBuilderCollectionExpression(BoundCollecti // with `anotherReadOnlySpan` being a ReadOnlySpan of the same type as target collection type // and that span cannot be captured in a returned ref struct // we can directly use `anotherReadOnlySpan` as collection builder argument and skip the copying assignment. - BoundExpression span = IsSingleReadOnlySpanSpread(node, out var spreadExpression) && (!constructMethod.ReturnType.IsRefLikeType || constructMethod.Parameters[0].EffectiveScope == ScopedKind.ScopedValue) + BoundExpression span = CanOptimizeSingleSpreadAsCollectionBuilderArgument(node, out var spreadExpression) ? spreadExpression : VisitArrayOrSpanCollectionExpression(node, CollectionExpressionTypeKind.ReadOnlySpan, spanType, elementType);