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

Jump to bottom

Collection expressions: use inline array for ReadOnlySpan<T> argument to builder method #69227

Merged
merged 14 commits into from
Aug 1, 2023
Merged

Collection expressions: use inline array for ReadOnlySpan<T> argument to builder method #69227

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
d75c020
Collection expressions: use inline array for ReadOnlySpan<T> argument…
cston Jul 18, 2023
56afdf6
Address PROTOTYPE comments
cston Jul 27, 2023
c5002c0
Address feedback
cston Jul 28, 2023
b47e209
Address feedback
cston Jul 28, 2023
aefb966
Rename WellKnownMember
cston Jul 28, 2023
4cfe4e1
Run tests on all platforms
cston Jul 28, 2023
75ead79
Remove unneeded WellKnownMember
cston Jul 31, 2023
d36040b
Misc.
cston Jul 31, 2023
99717d6
Address feedback
cston Jul 31, 2023
d4c2081
Make inline array type a top-level type
cston Jul 31, 2023
c419b51
Break lines
cston Jul 31, 2023
60e248e
Fix build
cston Jul 31, 2023
adae5cd
Address feedback
cston Aug 1, 2023
6fc3312
Add back deleted test
cston Aug 1, 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
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Binder/Binder_Expressions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4729,7 +4729,7 @@ private BoundExpression BindCollectionExpression(CollectionExpressionSyntax synt
{
builder.Add(bindElement(element, diagnostics));
}
return new BoundUnconvertedCollectionExpression(syntax, builder.ToImmutableAndFree(), this);
return new BoundUnconvertedCollectionExpression(syntax, builder.ToImmutableAndFree());

BoundExpression bindElement(CollectionElementSyntax syntax, BindingDiagnosticBag diagnostics)
{
Expand Down
1 change: 0 additions & 1 deletion src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1877,7 +1877,6 @@
<Field Name="Type" Type="TypeSymbol?" Override="true" Null="always"/>
<!-- Collection expression elements. -->
<Field Name="Elements" Type="ImmutableArray&lt;BoundExpression&gt;"/>
<Field Name="Binder" Type="Binder" Null="disallow"/>
</Node>

<Node Name="BoundCollectionExpression" Base="BoundExpression">
Expand Down
22 changes: 22 additions & 0 deletions src/Compilers/CSharp/Portable/Emitter/Model/PEModuleBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1970,6 +1970,28 @@ internal MethodSymbol EnsureInlineArrayAsSpanExists(SyntaxNode syntaxNode, Named
diagnostics);
}

internal NamedTypeSymbol EnsureInlineArrayTypeExists(SyntaxNode syntaxNode, SyntheticBoundNodeFactory factory, int arrayLength, DiagnosticBag diagnostics)
{
Debug.Assert(Compilation.Assembly.RuntimeSupportsInlineArrayTypes);

string typeName = PrivateImplementationDetails.GetInlineArrayTypeName(arrayLength);
var privateImplClass = GetPrivateImplClass(syntaxNode, diagnostics);
var typeAdapter = privateImplClass.GetType(typeName);
Copy link
Member

@jcouv jcouv Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

GetType

nit: consider calling this TryGetType since it can fail to return a type. Then maybe consider using a TryGet API pattern (return bool, out parameter). #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

I went with GetType() to match the existing GetMethod() method used in the other Ensure methods here.


if (typeAdapter is null)
Copy link
Member

@jcouv jcouv Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do we have a test with two collection expressions of the same size? (the InlineArray type will be re-used/shared) #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

CollectionBuilder_InlineArrayTypes

{
var attributeConstructor = (MethodSymbol)factory.SpecialMember(SpecialMember.System_Runtime_CompilerServices_InlineArrayAttribute__ctor);
Debug.Assert(attributeConstructor is { });

var typeSymbol = new SynthesizedInlineArrayTypeSymbol(SourceModule, typeName, arrayLength, attributeConstructor);
privateImplClass.TryAddSynthesizedType(typeSymbol.GetCciAdapter());
typeAdapter = privateImplClass.GetType(typeName)!;
Copy link
Member

@jcouv jcouv Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

!

Do we need this suppression? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

We're dereferencing typeAdapter below.

Copy link
Member

Choose a reason for hiding this comment

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

nit: consider doing Debug.Assert(typeAdapter!.Name == typeName); instead. That assertion should also ensure the nullable state of typeAdapter is not-null.

}

Debug.Assert(typeAdapter.Name == typeName);
return (NamedTypeSymbol)typeAdapter.GetInternalSymbol()!;
}

internal MethodSymbol EnsureInlineArrayAsReadOnlySpanExists(SyntaxNode syntaxNode, NamedTypeSymbol spanType, NamedTypeSymbol intType, DiagnosticBag diagnostics)
{
Debug.Assert(intType.SpecialType == SpecialType.System_Int32);
Expand Down
18 changes: 7 additions & 11 deletions src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

130 changes: 108 additions & 22 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 @@ -18,26 +18,35 @@ internal sealed partial class LocalRewriter
Debug.Assert(!_inExpressionLambda);
Debug.Assert(node.Type is { });

var collectionTypeKind = ConversionsBase.GetCollectionExpressionTypeKind(_compilation, node.Type, out var elementType);
switch (collectionTypeKind)
var previousSyntax = _factory.Syntax;
_factory.Syntax = node.Syntax;
try
{
case CollectionExpressionTypeKind.CollectionInitializer:
return VisitCollectionInitializerCollectionExpression(node, node.Type);
case CollectionExpressionTypeKind.Array:
case CollectionExpressionTypeKind.Span:
case CollectionExpressionTypeKind.ReadOnlySpan:
Debug.Assert(elementType is { });
return VisitArrayOrSpanCollectionExpression(node, node.Type, elementType);
case CollectionExpressionTypeKind.CollectionBuilder:
return VisitCollectionBuilderCollectionExpression(node);
case CollectionExpressionTypeKind.ListInterface:
return VisitListInterfaceCollectionExpression(node);
default:
throw ExceptionUtilities.UnexpectedValue(collectionTypeKind);
var collectionTypeKind = ConversionsBase.GetCollectionExpressionTypeKind(_compilation, node.Type, out var elementType);
switch (collectionTypeKind)
{
case CollectionExpressionTypeKind.CollectionInitializer:
return VisitCollectionInitializerCollectionExpression(node, node.Type);
case CollectionExpressionTypeKind.Array:
case CollectionExpressionTypeKind.Span:
case CollectionExpressionTypeKind.ReadOnlySpan:
Debug.Assert(elementType is { });
return VisitArrayOrSpanCollectionExpression(node, node.Type, TypeWithAnnotations.Create(elementType));
case CollectionExpressionTypeKind.CollectionBuilder:
return VisitCollectionBuilderCollectionExpression(node);
case CollectionExpressionTypeKind.ListInterface:
return VisitListInterfaceCollectionExpression(node);
default:
throw ExceptionUtilities.UnexpectedValue(collectionTypeKind);
}
}
finally
{
_factory.Syntax = previousSyntax;
}
}

private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpression node, TypeSymbol collectionType, TypeSymbol elementType)
private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpression node, TypeSymbol collectionType, TypeWithAnnotations elementType)
{
Debug.Assert(!_inExpressionLambda);

Expand All @@ -50,8 +59,8 @@ private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpr
Debug.Assert(collectionType.Name is "Span" or "ReadOnlySpan");
// We're constructing a Span<T> or ReadOnlySpan<T> rather than T[].
var spanType = (NamedTypeSymbol)collectionType;
Debug.Assert(elementType.Equals(spanType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0].Type, TypeCompareKind.AllIgnoreOptions));
arrayType = ArrayTypeSymbol.CreateSZArray(_compilation.Assembly, TypeWithAnnotations.Create(elementType));
Debug.Assert(elementType.Equals(spanType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0], TypeCompareKind.AllIgnoreOptions));
arrayType = ArrayTypeSymbol.CreateSZArray(_compilation.Assembly, elementType);
spanConstructor = ((MethodSymbol)_compilation.GetWellKnownTypeMember(
collectionType.Name == "Span" ? WellKnownMember.System_Span_T__ctor_Array : WellKnownMember.System_ReadOnlySpan_T__ctor_Array)!).AsMember(spanType);
}
Expand All @@ -64,7 +73,7 @@ private BoundExpression VisitArrayOrSpanCollectionExpression(BoundCollectionExpr
// The array initializer includes at least one spread element, so we'll create an intermediate List<T> instance.
// https://github.com/dotnet/roslyn/issues/68785: Avoid intermediate List<T> if all spread elements have Length property.
// https://github.com/dotnet/roslyn/issues/68785: Emit Enumerable.TryGetNonEnumeratedCount() and avoid intermediate List<T> at runtime.
var listType = _compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_List_T).Construct(elementType);
var listType = _compilation.GetWellKnownType(WellKnownType.System_Collections_Generic_List_T).Construct(ImmutableArray.Create(elementType));
var listToArray = ((MethodSymbol)_compilation.GetWellKnownTypeMember(WellKnownMember.System_Collections_Generic_List_T__ToArray)!).AsMember(listType);
var list = VisitCollectionInitializerCollectionExpression(node, collectionType);
array = _factory.Call(list, listToArray);
Expand Down Expand Up @@ -159,16 +168,35 @@ private BoundExpression VisitCollectionBuilderCollectionExpression(BoundCollecti
Debug.Assert(!_inExpressionLambda);
Debug.Assert(node.Type is { });

var syntax = node.Syntax;
var elements = node.Elements;
var constructMethod = node.CollectionBuilderMethod;

Debug.Assert(constructMethod is { });
Debug.Assert(constructMethod.ReturnType.Equals(node.Type, TypeCompareKind.AllIgnoreOptions));

var spanType = (NamedTypeSymbol)constructMethod.Parameters[0].Type;
Debug.Assert(spanType.OriginalDefinition.Equals(_compilation.GetWellKnownType(WellKnownType.System_ReadOnlySpan_T), TypeCompareKind.AllIgnoreOptions));

var span = VisitArrayOrSpanCollectionExpression(node, spanType, spanType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0].Type);
return new BoundCall(
node.Syntax,
var elementType = spanType.TypeArgumentsWithAnnotationsNoUseSiteDiagnostics[0];
var locals = ArrayBuilder<LocalSymbol>.GetInstance();
var sideEffects = ArrayBuilder<BoundExpression>.GetInstance();
BoundExpression span;

if (elements.Length > 0
Copy link
Member

@jcouv jcouv Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Should there be any kind of upper limit/heuristic? #Closed

Copy link
Member Author

@cston cston Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For now, we don't think there should be a limit (see notes), because the elements are already explicit in the source and because the work around to use the heap is to use C# 11 syntax.

&& !elements.Any(i => i is BoundCollectionExpressionSpreadElement)
Copy link
Member

Choose a reason for hiding this comment

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

I am a bit worried, from a usability perspective, about the potential for a perf cliff if you include a spread, and whether there's going to be advice of "yeah, for perf you shouldn't use spreads". Think we may want to push on the "Give me a span, on the stack if possible" API for allowing us to make this more of a gradient.

&& _compilation.Assembly.RuntimeSupportsInlineArrayTypes
&& (!constructMethod.ReturnType.IsRefLikeType || constructMethod.Parameters[0].EffectiveScope == ScopedKind.ScopedValue))
Copy link
Member

@jcouv jcouv Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Did we add tests to cover those conditions, or were those getting hit by existing scenarios? #Closed

Copy link
Member Author

Choose a reason for hiding this comment

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

Covered by CollectionBuilder_RefStructCollection.

{
span = CreateAndPopulateInlineArray(syntax, elementType, elements, locals, sideEffects);
}
else
{
span = VisitArrayOrSpanCollectionExpression(node, spanType, elementType);
}

var call = new BoundCall(
syntax,
receiverOpt: null,
method: constructMethod,
arguments: ImmutableArray.Create(span),
Expand All @@ -181,6 +209,64 @@ private BoundExpression VisitCollectionBuilderCollectionExpression(BoundCollecti
defaultArguments: default,
resultKind: LookupResultKind.Viable,
type: constructMethod.ReturnType);

return new BoundSequence(
syntax,
locals.ToImmutableAndFree(),
sideEffects.ToImmutableAndFree(),
call,
call.Type);
}

private BoundExpression CreateAndPopulateInlineArray(
SyntaxNode syntax,
TypeWithAnnotations elementType,
ImmutableArray<BoundExpression> elements,
ArrayBuilder<LocalSymbol> locals,
ArrayBuilder<BoundExpression> sideEffects)
{
Debug.Assert(elements.Length > 0);
Debug.Assert(_factory.ModuleBuilderOpt is { });
Debug.Assert(_diagnostics.DiagnosticBag is { });
Debug.Assert(_compilation.Assembly.RuntimeSupportsInlineArrayTypes);

int arrayLength = elements.Length;
var inlineArrayType = _factory.ModuleBuilderOpt.EnsureInlineArrayTypeExists(syntax, _factory, arrayLength, _diagnostics.DiagnosticBag).Construct(ImmutableArray.Create(elementType));
Debug.Assert(inlineArrayType.HasInlineArrayAttribute(out int inlineArrayLength) && inlineArrayLength == arrayLength);

var intType = _factory.SpecialType(SpecialType.System_Int32);
Copy link
Member

Choose a reason for hiding this comment

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

Do we have a test where type System.Int32 is missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

No. I believe we're handling that correctly here, and I don't think we'd get this far without an error in that case.

Copy link
Member

Choose a reason for hiding this comment

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

We have tests for various other missing types, but not this one. Let's cover it too.

MethodSymbol elementRef = _factory.ModuleBuilderOpt.EnsureInlineArrayElementRefExists(syntax, intType, _diagnostics.DiagnosticBag).
Construct(ImmutableArray.Create(TypeWithAnnotations.Create(inlineArrayType), elementType));

// Create an inline array and assign to a local.
// var tmp = new __InlineArrayN<T>();
Copy link
Member

@jcouv jcouv Jul 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

T

nit: ElementType instead of T (also below)? #Closed

BoundAssignmentOperator assignmentToTemp;
BoundLocal inlineArrayLocal = _factory.StoreToTemp(new BoundDefaultExpression(syntax, inlineArrayType), out assignmentToTemp, isKnownToReferToTempIfReferenceType: true);
sideEffects.Add(assignmentToTemp);
locals.Add(inlineArrayLocal.LocalSymbol);

// Populate the inline array.
// InlineArrayElementRef<__InlineArrayN<T>, T>(ref tmp, 0) = element0;
// InlineArrayElementRef<__InlineArrayN<T>, T>(ref tmp, 1) = element1;
// ...
for (int i = 0; i < arrayLength; i++)
{
var element = VisitExpression(elements[i]);
var call = _factory.Call(null, elementRef, inlineArrayLocal, _factory.Literal(i), useStrictArgumentRefKinds: true);
Copy link
Member

@jcouv jcouv Jul 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Please add pseudo-code comments corresponding to what is generated #Closed

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding. It makes things much easier to follow :-)

var assignment = new BoundAssignmentOperator(syntax, call, element, type: call.Type) { WasCompilerGenerated = true };
sideEffects.Add(assignment);
}

// Get a span to the inline array.
// ... InlineArrayAsReadOnlySpan<__InlineArrayN<T>, T>(in tmp, N)
var inlineArrayAsReadOnlySpan = _factory.ModuleBuilderOpt.EnsureInlineArrayAsReadOnlySpanExists(syntax, _factory.WellKnownType(WellKnownType.System_ReadOnlySpan_T), intType, _diagnostics.DiagnosticBag).
Construct(ImmutableArray.Create(TypeWithAnnotations.Create(inlineArrayType), elementType));
return _factory.Call(
receiver: null,
inlineArrayAsReadOnlySpan,
inlineArrayLocal,
_factory.Literal(arrayLength),
useStrictArgumentRefKinds: true);
}

private BoundExpression MakeCollectionExpressionSpreadElement(BoundCollectionExpressionSpreadElement initializer)
Expand Down
Loading