-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Collection expression spread optimization #71195
Changes from 16 commits
511526c
76cd2bb
fd3145e
1a3ed67
00a17f6
d295100
853f8aa
38ee84e
63dbe87
3eac1bf
882a643
501d5b1
36fb608
dc7ef72
c93244e
cce39d0
8625ac3
da6ad13
a071343
1b1ad03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -520,6 +521,42 @@ 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 = 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 = CallAsSpanMethod(VisitExpression(spreadExpression), asSpanMethod); | ||
return _factory.Call(rewrittenSpreadExpression, toArrayMethod.AsMember((NamedTypeSymbol)rewrittenSpreadExpression.Type!)); | ||
} | ||
} | ||
|
||
MethodSymbol? tryGetToArrayMethod(TypeSymbol spreadTypeOriginalDefinition, WellKnownType wellKnownType, WellKnownMember wellKnownMember) | ||
{ | ||
if (TypeSymbol.Equals(spreadTypeOriginalDefinition, this._compilation.GetWellKnownType(wellKnownType), TypeCompareKind.AllIgnoreOptions)) | ||
{ | ||
return _factory.WellKnownMethod(wellKnownMember, isOptional: true); | ||
} | ||
|
||
return null; | ||
} | ||
} | ||
|
||
if (numberIncludingLastSpread == 0) | ||
{ | ||
int knownLength = elements.Length; | ||
|
@@ -599,6 +636,20 @@ private BoundExpression CreateAndPopulateArray(BoundCollectionExpression node, A | |
_factory.Binary(BinaryOperatorKind.Addition, indexTemp.Type, indexTemp, _factory.Literal(1)), | ||
isRef: false, | ||
indexTemp.Type)); | ||
}, | ||
(sideEffects, arrayTemp, spreadElement, rewrittenSpreadOperand) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider naming these delegate parameters, especially now that there are multiple of them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, please spell out the types of these parameters. Same goes for the other similar delegates below. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't sure what you meant by name the delegate parameters. Were you suggesting that I declare a named delegate type which has these parameter names? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I mean specify the parameter name for each of these delegate arguments. What is the parameter this delegate is being passed to? |
||
{ | ||
if (TryPrepareCopyToOptimization(spreadElement, rewrittenSpreadOperand) is not var (spanSliceMethod, spreadElementAsSpan, getLengthMethod, copyToMethod)) | ||
return false; | ||
|
||
// https://github.com/dotnet/roslyn/issues/71270 | ||
// Could save the targetSpan to temp in the enclosing scope, but need to make sure we are async-safe etc. | ||
var targetSpan = TryConvertToSpanOrReadOnlySpan(arrayTemp); | ||
if (targetSpan is null) | ||
return false; | ||
|
||
PerformCopyToOptimization(sideEffects, localsBuilder, indexTemp, targetSpan, rewrittenSpreadOperand, spanSliceMethod, spreadElementAsSpan, getLengthMethod, copyToMethod); | ||
return true; | ||
}); | ||
|
||
var locals = localsBuilder.SelectAsArray(l => l.LocalSymbol); | ||
|
@@ -612,6 +663,161 @@ 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
{ | ||
var type = expression.Type; | ||
Debug.Assert(type is not null); | ||
|
||
if (!TryGetSpanConversion(type, out var asSpanMethod)) | ||
{ | ||
return null; | ||
} | ||
|
||
return CallAsSpanMethod(expression, asSpanMethod); | ||
} | ||
|
||
private BoundExpression CallAsSpanMethod(BoundExpression spreadExpression, MethodSymbol? asSpanMethod) | ||
{ | ||
if (asSpanMethod is null) | ||
{ | ||
return spreadExpression; | ||
} | ||
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!); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -722,11 +928,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, | ||
|
@@ -738,6 +953,25 @@ private BoundExpression CreateAndPopulateList(BoundCollectionExpression node, Ty | |
// list.Add(element); | ||
expressions.Add( | ||
_factory.Call(listTemp, addMethod, rewrittenValue)); | ||
}, | ||
(sideEffects, listTemp, 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.IsIdentity || (conversion.IsImplicit && conversion.IsReference)) | ||
{ | ||
conversion.MarkUnderlyingConversionsCheckedRecursive(); | ||
sideEffects.Add(_factory.Call(listTemp, addRangeMethod, rewrittenSpreadOperand)); | ||
return true; | ||
} | ||
|
||
return false; | ||
}); | ||
} | ||
|
||
|
@@ -782,7 +1016,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) | ||
{ | ||
for (int i = 0; i < elements.Length; i++) | ||
{ | ||
|
@@ -793,6 +1028,9 @@ private void AddCollectionExpressionElements( | |
|
||
if (element is BoundCollectionExpressionSpreadElement spreadElement) | ||
{ | ||
if (tryOptimizeSpreadElement.Invoke(sideEffects, rewrittenReceiver, spreadElement, rewrittenExpression)) | ||
continue; | ||
|
||
var rewrittenElement = MakeCollectionExpressionSpreadElement( | ||
spreadElement, | ||
rewrittenExpression, | ||
|
There was a problem hiding this comment.
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 anout
parameter.