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.

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

Refer directly to static data when ReadOnlySpan wraps arrays of bytes. #24621

Merged
merged 6 commits into from
Mar 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
122 changes: 122 additions & 0 deletions src/Compilers/CSharp/Portable/CodeGen/EmitArrayInitializer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -347,5 +347,127 @@ private static bool IsMultidimensionalInitializer(ImmutableArray<BoundExpression

return inits.Length != 0 && inits[0].Kind == BoundKind.ArrayInitialization;
}

private bool TryEmitReadonlySpanAsBlobWrapper(NamedTypeSymbol spanType, BoundExpression wrappedExpression, bool used, bool inPlace)
{
ImmutableArray<byte> data = default;
int elementCount = -1;
TypeSymbol elementType = null;

if (!_module.SupportsPrivateImplClass)
{
return false;
}

var ctor = ((MethodSymbol)this._module.Compilation.GetWellKnownTypeMember(WellKnownMember.System_ReadOnlySpan_T__ctor));
if (ctor == null)
{
return false;
}

if (wrappedExpression is BoundArrayCreation ac)
Copy link
Member

@jcouv jcouv Mar 5, 2018

Choose a reason for hiding this comment

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

Perhaps invert this logic:
if (wrappedExpression.Kind() != BoundKind.ArrayCreation) { return false; }
Then elementCount, data and elementType can be declared once they are needed and initialized. #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

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

The code started with support for arrays and string. Now, because of endiannes it was reduced to supporting just array, but once we have a necessary runtime helper, we would definitely want to support strings.
Then bailing out on negative condition would not be convenient.
With all other parts being equal, positive check is easier to scale to multiple types in the future, that is why we use a positive check here.


In reply to: 172329090 [](ancestors = 172329090)

{
var arrayType = (ArrayTypeSymbol)ac.Type;
elementType = arrayType.ElementType.EnumUnderlyingType();

// NB: we cannot use this approach for element types larger than one byte
// the issue is that metadata stores blobs in little-endian format
// so anything that is larger than one byte will be incorrect on a big-endian machine
// With additional runtime support it might be possible, but not yet.
// See: https://github.com/dotnet/corefx/issues/26948 for more details
if (elementType.SpecialType.SizeInBytes() != 1)
{
return false;
}

elementCount = TryGetRawDataForArrayInit(ac.InitializerOpt, out data);
}

if (elementCount < 0)
{
return false;
}

if (!inPlace && !used)
{
// emitting a value that no one will see
return true;
}

if (elementCount == 0)
{
if (inPlace)
{
_builder.EmitOpCode(ILOpCode.Initobj);
EmitSymbolToken(spanType, wrappedExpression.Syntax);
}
else
{
EmitDefaultValue(spanType, used, wrappedExpression.Syntax);
}
}
else
{
if (EnablePEVerifyCompat())
{
return false;
}

_builder.EmitArrayBlockFieldRef(data, elementType, wrappedExpression.Syntax, _diagnostics);
_builder.EmitIntConstant(elementCount);

if (inPlace)
{
// consumes target ref, data ptr and size, pushes nothing
_builder.EmitOpCode(ILOpCode.Call, stackAdjustment: -3);
}
else
{
// consumes data ptr and size, pushes the instance
_builder.EmitOpCode(ILOpCode.Newobj, stackAdjustment: -1);
}

EmitSymbolToken(ctor.AsMember(spanType), wrappedExpression.Syntax, optArgList: null);
}

return true;
}

/// <summary>
/// Returns a byte blob that matches serialized content of single array initializer.
/// returns -1 if the initializer is null or not an array of literals
/// </summary>
private int TryGetRawDataForArrayInit(BoundArrayInitialization initializer, out ImmutableArray<byte> data)
{
data = default;

if (initializer == null)
{
return -1;
}

var initializers = initializer.Initializers;
if (initializers.Any(init => init.ConstantValue == null))
{
return -1;
}

var elementCount = initializers.Length;
if (elementCount == 0)
{
data = ImmutableArray<byte>.Empty;
return 0;
}

var writer = new BlobBuilder(initializers.Length * 4);

foreach (var init in initializer.Initializers)
{
init.ConstantValue.Serialize(writer);
}

data = writer.ToImmutableArray();
return elementCount;
}
}
}
34 changes: 32 additions & 2 deletions src/Compilers/CSharp/Portable/CodeGen/EmitConversion.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,48 @@ private void EmitConversionExpression(BoundConversion conversion, bool used)
return;
}

var operand = conversion.Operand;

if (conversion.ConversionKind.IsUserDefinedConversion())
{
EmitSpecialUserDefinedConversion(conversion, used, operand);
return;
}

if (!used && !conversion.ConversionHasSideEffects())
{
EmitExpression(conversion.Operand, false); // just do expr side effects
EmitExpression(operand, false); // just do expr side effects
return;
}

EmitExpression(conversion.Operand, true);
EmitExpression(operand, true);
EmitConversion(conversion);

EmitPopIfUnused(used);
}

private void EmitSpecialUserDefinedConversion(BoundConversion conversion, bool used, BoundExpression operand)
{
var typeTo = (NamedTypeSymbol)conversion.Type;

Debug.Assert((operand.Type.IsArray()) &&
this._module.Compilation.IsReadOnlySpanType(typeTo),
"only special kinds of conversions involving ReadOnlySpan may be handled in emit");

if (!TryEmitReadonlySpanAsBlobWrapper(typeTo, operand, used, inPlace: false))
{
// there are several reasons that could prevent us from emitting a wrapper
// in such case we just emit the operand and then invoke the conversion method
EmitExpression(operand, used);
if (used)
{
// consumes 1 argument (array) and produces one result (span)
_builder.EmitOpCode(ILOpCode.Call, stackAdjustment: 0);
EmitSymbolToken(conversion.SymbolOpt, conversion.Syntax, optArgList: null);
}
}
}

private void EmitConversion(BoundConversion conversion)
{
switch (conversion.ConversionKind)
Expand Down
48 changes: 37 additions & 11 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1900,27 +1900,39 @@ private void EmitObjectCreationExpression(BoundObjectCreationExpression expressi
}
else
{
// check if need to construct at all
if (!used && ConstructorNotSideEffecting(constructor))
{
// creating nullable has no side-effects, so we will just evaluate the arguments
// ctor has no side-effects, so we will just evaluate the arguments
foreach (var arg in expression.Arguments)
{
EmitExpression(arg, used: false);
}

return;
}
else

// ReadOnlySpan may just refer to the blob, if possible.
if (this._module.Compilation.IsReadOnlySpanType(expression.Type) &&
expression.Arguments.Length == 1)
{
EmitArguments(expression.Arguments, constructor.Parameters, expression.ArgumentRefKindsOpt);
if (TryEmitReadonlySpanAsBlobWrapper((NamedTypeSymbol)expression.Type, expression.Arguments[0], used, inPlace: false))
{
return;
}
}

var stackAdjustment = GetObjCreationStackBehavior(expression);
_builder.EmitOpCode(ILOpCode.Newobj, stackAdjustment);
// none of the above cases, so just create an instance
EmitArguments(expression.Arguments, constructor.Parameters, expression.ArgumentRefKindsOpt);

// for variadic ctors emit expanded ctor token
EmitSymbolToken(constructor, expression.Syntax,
constructor.IsVararg ? (BoundArgListOperator)expression.Arguments[expression.Arguments.Length - 1] : null);
var stackAdjustment = GetObjCreationStackBehavior(expression);
_builder.EmitOpCode(ILOpCode.Newobj, stackAdjustment);

EmitPopIfUnused(used);
}
// for variadic ctors emit expanded ctor token
EmitSymbolToken(constructor, expression.Syntax,
constructor.IsVararg ? (BoundArgListOperator)expression.Arguments[expression.Arguments.Length - 1] : null);

EmitPopIfUnused(used);
}
}

Expand Down Expand Up @@ -2124,9 +2136,24 @@ private void InPlaceInit(BoundExpression target, bool used)

private void InPlaceCtorCall(BoundExpression target, BoundObjectCreationExpression objCreation, bool used)
{
Debug.Assert(TargetIsNotOnHeap(target), "in-place construction target should not be on heap");

var temp = EmitAddress(target, AddressKind.Writeable);
Debug.Assert(temp == null, "in-place ctor target should not create temps");

// ReadOnlySpan may just refer to the blob, if possible.
if (this._module.Compilation.IsReadOnlySpanType(objCreation.Type) && objCreation.Arguments.Length == 1)
{
if (TryEmitReadonlySpanAsBlobWrapper((NamedTypeSymbol)objCreation.Type, objCreation.Arguments[0], used, inPlace: true))
{
if (used)
{
EmitExpression(target, used: true);
}
return;
}
}

var constructor = objCreation.Constructor;
EmitArguments(objCreation.Arguments, constructor.Parameters, objCreation.ArgumentRefKindsOpt);
// -2 to adjust for consumed target address and not produced value.
Expand All @@ -2138,7 +2165,6 @@ private void InPlaceCtorCall(BoundExpression target, BoundObjectCreationExpressi

if (used)
{
Debug.Assert(TargetIsNotOnHeap(target), "cannot read-back the target since it could have been modified");
EmitExpression(target, used: true);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1014,8 +1014,14 @@ private BoundExpression RewriteUserDefinedConversion(
}
}

// do not rewrite user defined conversion
// - in expression trees or
// - special situations when converting from array to ReadOnlySpan, which has special support in codegen
bool doNotLowerToCall = _inExpressionLambda ||
(rewrittenOperand.Type.IsArray()) && _compilation.IsReadOnlySpanType(rewrittenType);

BoundExpression result =
_inExpressionLambda
doNotLowerToCall
? BoundConversion.Synthesized(syntax, rewrittenOperand, conversion, false, true, default(ConstantValue), rewrittenType)
: (BoundExpression)BoundCall.Synthesized(syntax, null, conversion.Method, rewrittenOperand);
Debug.Assert(result.Type == rewrittenType);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,11 @@ internal bool IsExceptionType(TypeSymbol type, ref HashSet<DiagnosticInfo> useSi
return IsEqualOrDerivedFromWellKnownClass(type, WellKnownType.System_Exception, ref useSiteDiagnostics);
}

internal bool IsReadOnlySpanType(TypeSymbol type)
{
return type.OriginalDefinition == GetWellKnownType(WellKnownType.System_ReadOnlySpan_T);
}

internal bool IsEqualOrDerivedFromWellKnownClass(TypeSymbol type, WellKnownType wellKnownType, ref HashSet<DiagnosticInfo> useSiteDiagnostics)
{
Debug.Assert(wellKnownType == WellKnownType.System_Attribute ||
Expand Down
Loading