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.

Sign up for GitHub

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

Collection expression spread optimization #71195

Merged
merged 20 commits into from
Dec 16, 2023
Merged

Collection expression spread optimization #71195

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
511526c
Collection expression spread optimization
RikkiGibson Dec 11, 2023
76cd2bb
Merge remote-tracking branch 'upstream/main' into collection-expr-spr…
RikkiGibson Dec 12, 2023
fd3145e
cleanup
RikkiGibson Dec 12, 2023
1a3ed67
Merge remote-tracking branch 'upstream/main' into collection-expr-spr…
RikkiGibson Dec 12, 2023
00a17f6
Adjust test baseline to reflect intention of test
RikkiGibson Dec 12, 2023
d295100
Fix WellKnownTypeValidationTests
RikkiGibson Dec 12, 2023
853f8aa
address feedback and fixup tests
RikkiGibson Dec 12, 2023
38ee84e
Fix MissingSpecialMember tests
RikkiGibson Dec 12, 2023
63dbe87
more
RikkiGibson Dec 13, 2023
3eac1bf
cleanup, feedback, TODOs
RikkiGibson Dec 14, 2023
882a643
more tests
RikkiGibson Dec 14, 2023
501d5b1
Get rid of temp for destination span. Adjust test baselines
RikkiGibson Dec 14, 2023
36fb608
Address more feedback
RikkiGibson Dec 15, 2023
dc7ef72
More tests. Address TODOs.
RikkiGibson Dec 15, 2023
c93244e
Fix raw string indenting
RikkiGibson Dec 15, 2023
cce39d0
Skip verification on a few more tests
RikkiGibson Dec 15, 2023
8625ac3
Address feedback
RikkiGibson Dec 15, 2023
da6ad13
address feedback
RikkiGibson Dec 15, 2023
a071343
Adjust assertion
RikkiGibson Dec 15, 2023
1b1ad03
Add targetFramework to pointer tests. Fix impl.
RikkiGibson Dec 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
245 changes: 243 additions & 2 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_CollectionExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.CodeGen;
using Microsoft.CodeAnalysis.CSharp.Symbols;
Expand Down Expand Up @@ -520,6 +521,44 @@ private BoundExpression CreateAndPopulateArray(BoundCollectionExpression node, A
throw ExceptionUtilities.UnexpectedValue(node);
}

// Collection-expr is of the form `[..spreadExpression]`, where 'spreadExpression' has same element type as the target collection.
// Optimize to `spreadExpression.ToArray()` if possible.
if (node is { Elements: [BoundCollectionExpressionSpreadElement { Expression: { } spreadExpression } spreadElement] }
&& spreadElement.IteratorBody is BoundExpressionStatement expressionStatement
&& expressionStatement.Expression is not BoundConversion)
{
var spreadTypeOriginalDefinition = spreadExpression.Type!.OriginalDefinition;
if (tryGetToArrayMethod(spreadTypeOriginalDefinition, WellKnownType.System_Collections_Generic_List_T, WellKnownMember.System_Collections_Generic_List_T__ToArray) is { } listToArrayMethod)
{
var rewrittenSpreadExpression = VisitExpression(spreadExpression);
return Factory.Call(rewrittenSpreadExpression, listToArrayMethod.AsMember((NamedTypeSymbol)spreadExpression.Type!));
}

if (TryGetSpanConversion(spreadExpression.Type, out var asSpanMethod))
{
var spanType = (asSpanMethod is null ? spreadExpression : CallAsSpanMethod(spreadExpression, asSpanMethod)).Type!.OriginalDefinition;
if ((tryGetToArrayMethod(spanType, WellKnownType.System_Span_T, WellKnownMember.System_Span_T__ToArray)
?? tryGetToArrayMethod(spanType, WellKnownType.System_ReadOnlySpan_T, WellKnownMember.System_ReadOnlySpan_T__ToArray))
is { } toArrayMethod)
{
var rewrittenSpreadExpression = VisitExpression(spreadExpression);
if (asSpanMethod is not null)
rewrittenSpreadExpression = CallAsSpanMethod(rewrittenSpreadExpression, asSpanMethod);
return Factory.Call(rewrittenSpreadExpression, toArrayMethod.AsMember((NamedTypeSymbol)rewrittenSpreadExpression.Type!));
}
}

MethodSymbol? tryGetToArrayMethod(TypeSymbol spreadTypeOriginalDefinition, WellKnownType wellKnownType, WellKnownMember wellKnownMember)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really a try-get in traditional form, and it feels like the code that uses it is a bit convoluted because of it. Consider refactoring to be a standard tryGet with an out parameter.

{
if (spreadTypeOriginalDefinition.Equals(this._compilation.GetWellKnownType(wellKnownType)))
{
return _factory.WellKnownMethod(wellKnownMember, isOptional: true);
}

return null;
}
}

if (numberIncludingLastSpread == 0)
{
int knownLength = elements.Length;
Expand Down Expand Up @@ -569,6 +608,7 @@ private BoundExpression CreateAndPopulateArray(BoundCollectionExpression node, A
localsBuilder.Add(arrayTemp);
sideEffects.Add(assignmentToTemp);

BoundLocal? targetSpanTemp = null;
AddCollectionExpressionElements(
elements,
arrayTemp,
Expand Down Expand Up @@ -599,6 +639,25 @@ private BoundExpression CreateAndPopulateArray(BoundCollectionExpression node, A
_factory.Binary(BinaryOperatorKind.Addition, indexTemp.Type, indexTemp, _factory.Literal(1)),
isRef: false,
indexTemp.Type));
},
(sideEffects, targetArray, spreadElement, rewrittenSpreadOperand) =>
{
if (TryPrepareCopyToOptimization(spreadElement, rewrittenSpreadOperand) is not var (spanSliceMethod, spreadElementAsSpan, getLengthMethod, copyToMethod))
return false;

if (targetSpanTemp is null)
{
var targetSpan = TryConvertToSpanOrReadOnlySpan(targetArray);
if (targetSpan is null)
return false;

targetSpanTemp = _factory.StoreToTemp(targetSpan, out var assignmentToTemp);
localsBuilder.Add(targetSpanTemp);
sideEffects.Add(assignmentToTemp);
}

PerformCopyToOptimization(sideEffects, localsBuilder, indexTemp, targetSpanTemp, rewrittenSpreadOperand, spanSliceMethod, spreadElementAsSpan, getLengthMethod, copyToMethod);
return true;
});

var locals = localsBuilder.SelectAsArray(l => l.LocalSymbol);
Expand All @@ -612,6 +671,157 @@ private BoundExpression CreateAndPopulateArray(BoundCollectionExpression node, A
arrayType);
}

/// <summary>
/// Returns true if type is convertible to Span or ReadOnlySpan.
/// If non-identity conversion, also returns a non-null asSpanMethod.
/// </summary>
private bool TryGetSpanConversion(TypeSymbol type, out MethodSymbol? asSpanMethod)
Copy link
Member

Choose a reason for hiding this comment

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

Let's rearrange this method to simplify things. First, check to see if it's the array case. The rest of the cases depend on it being a named type symbol, so you can do that check once and bail if false, rather than type-checking against NamedTypeSymbol 3 times.

{
if (type is NamedTypeSymbol spanType
&& (spanType.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_Span_T), TypeCompareKind.ConsiderEverything)
|| spanType.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.ConsiderEverything)))
{
asSpanMethod = null;
return true;
}

if (type is ArrayTypeSymbol { IsSZArray: true } arrayType
&& _factory.WellKnownMethod(WellKnownMember.System_Span_T__ctor_Array, isOptional: true) is { } spanCtorArray)
{
asSpanMethod = spanCtorArray.AsMember(spanCtorArray.ContainingType.Construct(arrayType.ElementType));
Copy link
Member

Choose a reason for hiding this comment

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

What if the element type is something that can't be a valid type parameter, like a pointer? Do we have a test covering that?

return true;
}

if (type is NamedTypeSymbol immutableArrayType
&& immutableArrayType.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_Collections_Immutable_ImmutableArray_T), TypeCompareKind.ConsiderEverything)
&& _factory.WellKnownMethod(WellKnownMember.System_Collections_Immutable_ImmutableArray_T__AsSpan, isOptional: true) is { } immutableArrayAsSpanMethod)
{
asSpanMethod = immutableArrayAsSpanMethod!.AsMember(immutableArrayType);
return true;
}

if (type is NamedTypeSymbol listType
&& listType.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_List_T), TypeCompareKind.ConsiderEverything)
&& _factory.WellKnownMethod(WellKnownMember.System_Runtime_InteropServices_CollectionsMarshal__AsSpan_T, isOptional: true) is { } collectionsMarshalAsSpanMethod)
{
asSpanMethod = collectionsMarshalAsSpanMethod.Construct(listType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0].Type);
return true;
}

asSpanMethod = null;
return false;
}

private BoundExpression? TryConvertToSpanOrReadOnlySpan(BoundExpression expression)
Copy link
Member

Choose a reason for hiding this comment

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

It feels like this is another method that would be better served by following the standard TryGet pattern.

{
var type = expression.Type;
Debug.Assert(type is not null);

if (!TryGetSpanConversion(type, out var asSpanMethod))
{
return null;
}

return asSpanMethod is null ? expression : CallAsSpanMethod(expression, asSpanMethod);
}

private BoundExpression CallAsSpanMethod(BoundExpression spreadExpression, MethodSymbol asSpanMethod)
{
if (asSpanMethod is MethodSymbol { MethodKind: MethodKind.Constructor } constructor)
{
return _factory.New(constructor, spreadExpression);
}
else if (asSpanMethod is MethodSymbol { IsStatic: true, ParameterCount: 1 })
{
return _factory.Call(receiver: null, asSpanMethod, spreadExpression);
}
else
{
return _factory.Call(spreadExpression, asSpanMethod);
}
}

/// <summary>
/// Verifies presence of methods necessary for the CopyTo optimization
/// without performing mutating actions e.g. appending to side effects or locals builders.
/// </summary>
private (MethodSymbol spanSliceMethod, BoundExpression spreadElementAsSpan, MethodSymbol getLengthMethod, MethodSymbol copyToMethod)? TryPrepareCopyToOptimization(
BoundCollectionExpressionSpreadElement spreadElement,
BoundExpression rewrittenSpreadOperand)
{
// Cannot use CopyTo when spread element has non-identity conversion to target element type.
// Could do a covariant conversion of ReadOnlySpan in future: https://github.com/dotnet/roslyn/issues/71106
if (spreadElement.IteratorBody is not BoundExpressionStatement expressionStatement || expressionStatement.Expression is BoundConversion)
return null;

if (_factory.WellKnownMethod(WellKnownMember.System_Span_T__Slice_Int_Int, isOptional: true) is not { } spanSliceMethod)
return null;

if (TryConvertToSpanOrReadOnlySpan(rewrittenSpreadOperand) is not { } spreadOperandAsSpan)
return null;

if ((tryGetSpanMethodsForSpread(WellKnownType.System_ReadOnlySpan_T, WellKnownMember.System_ReadOnlySpan_T__get_Length, WellKnownMember.System_ReadOnlySpan_T__CopyTo_Span_T)
?? tryGetSpanMethodsForSpread(WellKnownType.System_Span_T, WellKnownMember.System_Span_T__get_Length, WellKnownMember.System_Span_T__CopyTo_Span_T))
is not (var getLengthMethod, var copyToMethod))
{
return null;
}

return (spanSliceMethod, spreadOperandAsSpan, getLengthMethod, copyToMethod);

// gets either Span or ReadOnlySpan methods for operating on the source spread element.
(MethodSymbol getLengthMethod, MethodSymbol copyToMethod)? tryGetSpanMethodsForSpread(
WellKnownType wellKnownSpanType,
WellKnownMember getLengthMember,
WellKnownMember copyToMember)
{
if (spreadOperandAsSpan.Type!.OriginalDefinition.Equals(this._compilation.GetWellKnownType(wellKnownSpanType))
&& _factory.WellKnownMethod(getLengthMember, isOptional: true) is { } getLengthMethod
&& _factory.WellKnownMethod(copyToMember, isOptional: true) is { } copyToMethod)
{
return (getLengthMethod!, copyToMethod!);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need suppressions on this line?

}

return null;
}
}

private void PerformCopyToOptimization(
ArrayBuilder<BoundExpression> sideEffects,
ArrayBuilder<BoundLocal> localsBuilder,
BoundLocal indexTemp,
BoundExpression spanTemp,
BoundExpression rewrittenSpreadOperand,
MethodSymbol spanSliceMethod,
BoundExpression spreadOperandAsSpan,
MethodSymbol getLengthMethod,
MethodSymbol copyToMethod)
{
// before:
// ..e1 // in [e0, ..e1]
//
// after (roughly):
// var e1Span = e1.AsSpan();
// e1Span.CopyTo(destinationSpan.Slice(indexTemp, e1Span.Length);
// indexTemp += e1Span.Length;

Debug.Assert((object)spreadOperandAsSpan != rewrittenSpreadOperand || spreadOperandAsSpan is BoundLocal { LocalSymbol.SynthesizedKind: SynthesizedLocalKind.LoweringTemp });
if ((object)spreadOperandAsSpan != rewrittenSpreadOperand)
{
spreadOperandAsSpan = _factory.StoreToTemp(spreadOperandAsSpan, out var assignmentToTemp);
sideEffects.Add(assignmentToTemp);
localsBuilder.Add((BoundLocal)spreadOperandAsSpan);
}

// e1Span.CopyTo(destinationSpan.Slice(indexTemp, e1Span.Length);
var spreadLength = _factory.Call(spreadOperandAsSpan, getLengthMethod.AsMember((NamedTypeSymbol)spreadOperandAsSpan.Type!));
var targetSlice = _factory.Call(spanTemp, spanSliceMethod.AsMember((NamedTypeSymbol)spanTemp.Type!), indexTemp, spreadLength);
sideEffects.Add(_factory.Call(spreadOperandAsSpan, copyToMethod.AsMember((NamedTypeSymbol)spreadOperandAsSpan.Type!), targetSlice));

// indexTemp += e1Span.Length;
sideEffects.Add(new BoundAssignmentOperator(rewrittenSpreadOperand.Syntax, indexTemp, _factory.Binary(BinaryOperatorKind.Addition, indexTemp.Type, indexTemp, spreadLength), isRef: false, indexTemp.Type));
}

/// <summary>
/// Create and populate an list from a collection expression.
/// The collection may or may not have a known length.
Expand Down Expand Up @@ -722,11 +932,20 @@ private BoundExpression CreateAndPopulateList(BoundCollectionExpression node, Ty
_factory.Binary(BinaryOperatorKind.Addition, indexTemp.Type, indexTemp, _factory.Literal(1)),
isRef: false,
indexTemp.Type));
},
(sideEffects, spanTemp, spreadElement, rewrittenSpreadOperand) =>
{
if (TryPrepareCopyToOptimization(spreadElement, rewrittenSpreadOperand) is not var (spanSliceMethod, spreadElementAsSpan, getLengthMethod, copyToMethod))
return false;

PerformCopyToOptimization(sideEffects, localsBuilder, indexTemp, spanTemp, rewrittenSpreadOperand, spanSliceMethod, spreadElementAsSpan, getLengthMethod, copyToMethod);
return true;
});
}
else
{
var addMethod = ((MethodSymbol)_factory.WellKnownMember(WellKnownMember.System_Collections_Generic_List_T__Add)).AsMember(collectionType);
var addMethod = _factory.WellKnownMethod(WellKnownMember.System_Collections_Generic_List_T__Add).AsMember(collectionType);
var addRangeMethod = _factory.WellKnownMethod(WellKnownMember.System_Collections_Generic_List_T__AddRange, isOptional: true)?.AsMember(collectionType);
AddCollectionExpressionElements(
elements,
listTemp,
Expand All @@ -738,6 +957,24 @@ private BoundExpression CreateAndPopulateList(BoundCollectionExpression node, Ty
// list.Add(element);
expressions.Add(
_factory.Call(listTemp, addMethod, rewrittenValue));
},
(sideEffects, spanTemp, spreadElement, rewrittenSpreadOperand) =>
{
if (addRangeMethod is null)
return false;

var type = rewrittenSpreadOperand.Type!;

var useSiteInfo = GetNewCompoundUseSiteInfo();
var conversion = _compilation.Conversions.ClassifyConversionFromType(type, addRangeMethod.Parameters[0].Type, isChecked: false, ref useSiteInfo);
_diagnostics.Add(rewrittenSpreadOperand.Syntax, useSiteInfo);
if (conversion.IsImplicit && conversion.IsReference)
{
sideEffects.Add(_factory.Call(listTemp, addRangeMethod, MakeConversionNode(rewrittenSpreadOperand, type, @checked: false, markAsChecked: true)));
return true;
}

return false;
});
}

Expand Down Expand Up @@ -782,7 +1019,8 @@ private void AddCollectionExpressionElements(
ArrayBuilder<BoundLocal> rewrittenExpressions,
int numberIncludingLastSpread,
ArrayBuilder<BoundExpression> sideEffects,
Action<ArrayBuilder<BoundExpression>, BoundExpression, BoundExpression> addElement)
Action<ArrayBuilder<BoundExpression>, BoundExpression, BoundExpression> addElement,
Func<ArrayBuilder<BoundExpression>, BoundExpression, BoundCollectionExpressionSpreadElement, BoundExpression, bool>? tryOptimizeSpreadElement = null)
{
for (int i = 0; i < elements.Length; i++)
{
Expand All @@ -793,6 +1031,9 @@ private void AddCollectionExpressionElements(

if (element is BoundCollectionExpressionSpreadElement spreadElement)
{
if (tryOptimizeSpreadElement?.Invoke(sideEffects, rewrittenReceiver, spreadElement, rewrittenExpression) == true)
continue;

var rewrittenElement = MakeCollectionExpressionSpreadElement(
spreadElement,
rewrittenExpression,
Expand Down
Loading