Skip to content

Commit

Permalink
Revert "Do not copy nullable value type receiver of a constrained cal…
Browse files Browse the repository at this point in the history 
…l." (#66407)

Reverts #66311 due to dotnet/runtime#80429 (comment)
  • Loading branch information
@AlekseyTs
AlekseyTs authored Jan 13, 2023
2 parents a13836f + 6c097b6 commit 1dd4753
Show file tree
Hide file tree
Showing 22 changed files with 2,984 additions and 3,419 deletions.
15 changes: 0 additions & 15 deletions src/Compilers/CSharp/Portable/Binder/Binder.ValueChecks.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4580,21 +4580,6 @@ internal static bool HasHome(
stackLocalsOpt));
goto case BoundKind.ConditionalReceiver;

case BoundKind.ComplexReceiver:
Debug.Assert(HasHome(
((BoundComplexReceiver)expression).ValueTypeReceiver,
addressKind,
containingSymbol,
peVerifyCompatEnabled,
stackLocalsOpt));
Debug.Assert(HasHome(
((BoundComplexReceiver)expression).ReferenceTypeReceiver,
addressKind,
containingSymbol,
peVerifyCompatEnabled,
stackLocalsOpt));
goto case BoundKind.ConditionalReceiver;

case BoundKind.ConditionalReceiver:
//ConditionalReceiver is a noop from Emit point of view. - it represents something that has already been pushed.
//We should never need a temp for it.
Expand Down
14 changes: 2 additions & 12 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1714,8 +1714,8 @@
<Field Name="Id" Type="int"/>
</Node>

<!-- This node represents a complex receiver for a conditional access.
At runtime, when its type is a non-nullable value type, ValueTypeReceiver should be used as a receiver.
<!-- This node represents a complex receiver for a call, or a conditional access.
At runtime, when its type is a value type, ValueTypeReceiver should be used as a receiver.
Otherwise, ReferenceTypeReceiver should be used.
This kind of receiver is created only by SpillSequenceSpiller rewriter.
-->
Expand All @@ -1725,16 +1725,6 @@
<Field Name="ReferenceTypeReceiver" Type="BoundExpression" Null="disallow"/>
</Node>

<!-- This node represents a complex receiver for a call.
At runtime, when its type is a value type (including nullable value type), ValueTypeReceiver should be used as a receiver.
Otherwise, ReferenceTypeReceiver should be used.
-->
<Node Name="BoundComplexReceiver" Base="BoundExpression">
<Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>
<Field Name="ValueTypeReceiver" Type="BoundExpression" Null="disallow"/>
<Field Name="ReferenceTypeReceiver" Type="BoundExpression" Null="disallow"/>
</Node>

<Node Name="BoundMethodGroup" Base="BoundMethodOrPropertyGroup">
<!-- SPEC: A method group is a set of overloaded methods resulting from a member lookup.
SPEC: A method group may have an associated instance expression and
Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/CodeGen/CodeGenerator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,14 +224,14 @@ public void Generate(
hasStackAlloc = _sawStackalloc;
Debug.Assert(_asyncCatchHandlerOffset >= 0);

asyncCatchHandlerOffset = _diagnostics.HasAnyErrors() ? -1 : _builder.GetILOffsetFromMarker(_asyncCatchHandlerOffset);
asyncCatchHandlerOffset = _builder.GetILOffsetFromMarker(_asyncCatchHandlerOffset);

ArrayBuilder<int> yieldPoints = _asyncYieldPoints;
ArrayBuilder<int> resumePoints = _asyncResumePoints;

Debug.Assert((yieldPoints == null) == (resumePoints == null));

if (yieldPoints == null || _diagnostics.HasAnyErrors())
if (yieldPoints == null)
{
asyncYieldPoints = ImmutableArray<int>.Empty;
asyncResumePoints = ImmutableArray<int>.Empty;
Expand Down
4 changes: 0 additions & 4 deletions src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,6 @@ private LocalDefinition EmitAddress(BoundExpression expression, AddressKind addr
EmitComplexConditionalReceiverAddress((BoundComplexConditionalReceiver)expression);
break;

case BoundKind.ComplexReceiver:
EmitComplexReceiverAddress((BoundComplexReceiver)expression);
break;

case BoundKind.Parameter:
return EmitParameterAddress((BoundParameter)expression, addressKind);

Expand Down
164 changes: 14 additions & 150 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -316,10 +316,6 @@ private void EmitExpressionCore(BoundExpression expression, bool used)
EmitComplexConditionalReceiver((BoundComplexConditionalReceiver)expression, used);
break;

case BoundKind.ComplexReceiver:
EmitComplexReceiver((BoundComplexReceiver)expression, used);
break;

case BoundKind.PseudoVariable:
EmitPseudoVariableValue((BoundPseudoVariable)expression, used);
break;
Expand Down Expand Up @@ -369,11 +365,7 @@ private void EmitComplexConditionalReceiver(BoundComplexConditionalReceiver expr

EmitExpression(expression.ReferenceTypeReceiver, used);
_builder.EmitBranch(ILOpCode.Br, doneLabel);

if (used)
{
_builder.AdjustStack(-1);
}
_builder.AdjustStack(-1);

_builder.MarkLabel(whenValueTypeLabel);
EmitExpression(expression.ValueTypeReceiver, used);
Expand Down Expand Up @@ -420,8 +412,7 @@ private void EmitLoweredConditionalAccessExpression(BoundLoweredConditionalAcces
((TypeParameterSymbol)receiverType).EffectiveInterfacesNoUseSiteDiagnostics.IsEmpty) || // This could be a nullable value type, which must be copied in order to not mutate the original value
LocalRewriter.CanChangeValueBetweenReads(receiver, localsMayBeAssignedOrCaptured: false) ||
(receiverType.IsReferenceType && receiverType.TypeKind == TypeKind.TypeParameter) ||
(receiver.Kind == BoundKind.Local && IsStackLocal(((BoundLocal)receiver).LocalSymbol)) ||
(notConstrained && IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker.Analyze(expression));
(receiver.Kind == BoundKind.Local && IsStackLocal(((BoundLocal)receiver).LocalSymbol));

// ===== RECEIVER
if (nullCheckOnCopy)
Expand Down Expand Up @@ -573,60 +564,6 @@ private void EmitLoweredConditionalAccessExpression(BoundLoweredConditionalAcces
}
}

private sealed class IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker : BoundTreeWalkerWithStackGuardWithoutRecursionOnTheLeftOfBinaryOperator
{
private readonly BoundLoweredConditionalAccess _conditionalAccess;
private bool? _result;

private IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker(BoundLoweredConditionalAccess conditionalAccess)
: base()
{
_conditionalAccess = conditionalAccess;
}

public static bool Analyze(BoundLoweredConditionalAccess conditionalAccess)
{
var walker = new IsConditionalConstrainedCallThatMustUseTempForReferenceTypeReceiverWalker(conditionalAccess);
walker.Visit(conditionalAccess.WhenNotNull);
Debug.Assert(walker._result.HasValue);
return walker._result.GetValueOrDefault();
}

public override BoundNode Visit(BoundNode node)
{
if (_result.HasValue)
{
return null;
}

return base.Visit(node);
}

public override BoundNode VisitCall(BoundCall node)
{
if (node.ReceiverOpt is BoundConditionalReceiver { Id: var id } && id == _conditionalAccess.Id)
{
Debug.Assert(!_result.HasValue);
_result = !IsSafeToDereferenceReceiverRefAfterEvaluatingArguments(node.Arguments);
return null;
}

return base.VisitCall(node);
}

public override BoundNode VisitConditionalReceiver(BoundConditionalReceiver node)
{
if (node.Id == _conditionalAccess.Id)
{
Debug.Assert(!_result.HasValue);
_result = false;
return null;
}

return base.VisitConditionalReceiver(node);
}
}

private void EmitConditionalReceiver(BoundConditionalReceiver expression, bool used)
{
Debug.Assert(!expression.Type.IsValueType);
Expand Down Expand Up @@ -1815,21 +1752,25 @@ LocalDefinition emitGenericReceiver(BoundCall call, out CallKind callKind)
// reference to the stack. So, for a class we need to emit a reference to a temporary
// location, rather than to the original location

// We will emit a runtime check for a class, but the check is really a JIT-time
// Struct values are never nulls.
// We will emit a check for such case, but the check is really a JIT-time
// constant since JIT will know if T is a struct or not.

// if (!typeof(T).IsValueType)
// if ((object)default(T) == null)
// {
// temp = receiverRef
// receiverRef = ref temp
// }

object whenValueTypeLabel = null;
object whenNotNullLabel = null;

if (!receiverType.IsReferenceType)
{
// if (!typeof(T).IsValueType)
whenValueTypeLabel = TryEmitIsValueTypeBranch(receiverType, receiver.Syntax);
// if ((object)default(T) == null)
EmitDefaultValue(receiverType, true, receiver.Syntax);
EmitBox(receiverType, receiver.Syntax);
whenNotNullLabel = new object();
_builder.EmitBranch(ILOpCode.Brtrue, whenNotNullLabel);
}

// temp = receiverRef
Expand All @@ -1839,91 +1780,16 @@ LocalDefinition emitGenericReceiver(BoundCall call, out CallKind callKind)
_builder.EmitLocalStore(tempOpt);
_builder.EmitLocalAddress(tempOpt);

if (whenValueTypeLabel is not null)
if (whenNotNullLabel is not null)
{
_builder.MarkLabel(whenValueTypeLabel);
_builder.MarkLabel(whenNotNullLabel);
}
}

return tempOpt;
}
}

private object TryEmitIsValueTypeBranch(TypeSymbol type, SyntaxNode syntax)
{
if (Binder.GetWellKnownTypeMember(_module.Compilation, WellKnownMember.System_Type__GetTypeFromHandle, _diagnostics, syntax: syntax, isOptional: false) is MethodSymbol getTypeFromHandle &&
Binder.GetWellKnownTypeMember(_module.Compilation, WellKnownMember.System_Type__get_IsValueType, _diagnostics, syntax: syntax, isOptional: false) is MethodSymbol getIsValueType)
{
_builder.EmitOpCode(ILOpCode.Ldtoken);
EmitSymbolToken(type, syntax);
_builder.EmitOpCode(ILOpCode.Call, stackAdjustment: 0); // argument off, return value on
EmitSymbolToken(getTypeFromHandle, syntax, null);
_builder.EmitOpCode(ILOpCode.Call, stackAdjustment: 0); // instance off, return value on
EmitSymbolToken(getIsValueType, syntax, null);
var whenValueTypeLabel = new object();
_builder.EmitBranch(ILOpCode.Brtrue, whenValueTypeLabel);

return whenValueTypeLabel;
}

return null;
}

private void EmitComplexReceiver(BoundComplexReceiver expression, bool used)
{
Debug.Assert(!used);
Debug.Assert(!expression.Type.IsReferenceType);
Debug.Assert(!expression.Type.IsValueType);

var receiverType = expression.Type;

// if (!typeof(T).IsValueType)
object whenValueTypeLabel = TryEmitIsValueTypeBranch(receiverType, expression.Syntax);

EmitExpression(expression.ReferenceTypeReceiver, used);
var doneLabel = new object();
_builder.EmitBranch(ILOpCode.Br, doneLabel);

if (whenValueTypeLabel is not null)
{
if (used)
{
_builder.AdjustStack(-1);
}

_builder.MarkLabel(whenValueTypeLabel);
EmitExpression(expression.ValueTypeReceiver, used);
}

_builder.MarkLabel(doneLabel);
}

private void EmitComplexReceiverAddress(BoundComplexReceiver expression)
{
Debug.Assert(!expression.Type.IsReferenceType);
Debug.Assert(!expression.Type.IsValueType);

var receiverType = expression.Type;

// if (!typeof(T).IsValueType)
object whenValueTypeLabel = TryEmitIsValueTypeBranch(receiverType, expression.Syntax);

var receiverTemp = EmitAddress(expression.ReferenceTypeReceiver, AddressKind.ReadOnly);
Debug.Assert(receiverTemp == null);
var doneLabel = new Object();
_builder.EmitBranch(ILOpCode.Br, doneLabel);

if (whenValueTypeLabel is not null)
{
_builder.AdjustStack(-1);
_builder.MarkLabel(whenValueTypeLabel);
// we will not write through this receiver, but it could be a target of mutating calls
EmitReceiverRef(expression.ValueTypeReceiver, AddressKind.Constrained);
}

_builder.MarkLabel(doneLabel);
}

internal static bool IsPossibleReferenceTypeReceiverOfConstrainedCall(BoundExpression receiver)
{
var receiverType = receiver.Type;
Expand All @@ -1945,9 +1811,7 @@ internal static bool ReceiverIsKnownToReferToTempIfReferenceType(BoundExpression

if (receiver is
BoundLocal { LocalSymbol.IsKnownToReferToTempIfReferenceType: true } or
BoundComplexConditionalReceiver or
BoundComplexReceiver or
BoundConditionalReceiver { Type: { IsReferenceType: false, IsValueType: false } })
BoundComplexConditionalReceiver)
{
return true;
}
Expand Down
47 changes: 1 addition & 46 deletions src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Text;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.CSharp.Symbols;
Expand Down Expand Up @@ -1564,43 +1563,6 @@ public override BoundNode VisitComplexConditionalReceiver(BoundComplexConditiona
return node.Update(valueTypeReceiver, referenceTypeReceiver, node.Type);
}

public override BoundNode VisitComplexReceiver(BoundComplexReceiver node)
{
EnsureOnlyEvalStack();

var origStack = StackDepth();

PushEvalStack(null, ExprContext.None);

var cookie = GetStackStateCookie(); // implicit goto here

SetStackDepth(origStack); // consequence is evaluated with original stack

var valueTypeReceiver = (BoundExpression)this.Visit(node.ValueTypeReceiver);

EnsureStackState(cookie); // implicit label here

SetStackDepth(origStack); // alternative is evaluated with original stack

var unwrappedSequence = node.ReferenceTypeReceiver;

while (unwrappedSequence is BoundSequence sequence)
{
unwrappedSequence = sequence.Value;
}

if (unwrappedSequence is BoundLocal { LocalSymbol: { } localSymbol })
{
ShouldNotSchedule(localSymbol);
}

var referenceTypeReceiver = (BoundExpression)this.Visit(node.ReferenceTypeReceiver);

EnsureStackState(cookie); // implicit label here

return node.Update(valueTypeReceiver, referenceTypeReceiver, node.Type);
}

public override BoundNode VisitUnaryOperator(BoundUnaryOperator node)
{
// checked(-x) is emitted as "0 - x"
Expand Down Expand Up @@ -2252,14 +2214,7 @@ internal override SyntaxNode ScopeDesignatorOpt
get { return null; }
}

internal override LocalSymbol WithSynthesizedLocalKindAndSyntax(
SynthesizedLocalKind kind, SyntaxNode syntax
#if DEBUG
,
[CallerLineNumber] int createdAtLineNumber = 0,
[CallerFilePath] string createdAtFilePath = null
#endif
)
internal override LocalSymbol WithSynthesizedLocalKindAndSyntax(SynthesizedLocalKind kind, SyntaxNode syntax)
{
throw new NotImplementedException();
}
Expand Down
14 changes: 0 additions & 14 deletions src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2940,20 +2940,6 @@ public override BoundNode VisitComplexConditionalReceiver(BoundComplexConditiona
return null;
}

public override BoundNode VisitComplexReceiver(BoundComplexReceiver node)
{
var savedState = this.State.Clone();

VisitRvalue(node.ValueTypeReceiver);
Join(ref this.State, ref savedState);

savedState = this.State.Clone();
VisitRvalue(node.ReferenceTypeReceiver);
Join(ref this.State, ref savedState);

return null;
}

public override BoundNode VisitSequence(BoundSequence node)
{
var sideEffects = node.SideEffects;
Expand Down
Loading

0 comments on commit 1dd4753

Please sign in to comment.