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

Avoid creating result temp for switch-expressions #69194

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from
Draft

Avoid creating result temp for switch-expressions #69194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1c24d15
Avoid creating result temp for is-expressions
alrz Jul 17, 2023
ef864e8
Avoid creating result temp for switch-expressions
alrz Jul 24, 2023
8700a94
Simplify
alrz Aug 9, 2023
80b45e7
Reset state
alrz Feb 27, 2024
5f1f000
Revert
alrz Feb 27, 2024
3cd0617
Update baseline
alrz Feb 27, 2024
edd02ad
Fix formatting
alrz Feb 27, 2024
ffe56a2
Join for end state
alrz Feb 27, 2024
e0e0f87
Move
alrz Feb 27, 2024
755afc6
Fixup
alrz Feb 27, 2024
92d93d8
Merge remote-tracking branch 'origin/main' into lowered-switch
alrz Mar 22, 2024
6d5275d
Revert formatting
alrz Mar 22, 2024
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
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundLoweredIsPatternExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,12 @@ private partial void Validate()
Debug.Assert(this.Statements is [.., BoundGotoStatement or BoundSwitchDispatch]);
}
}

partial class BoundLoweredSwitchExpression
{
private partial void Validate()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It feels like a BoundNode_Validate.cs file could be used to gather all these in one place.

{
Debug.Assert(this.Statements is [.., BoundGotoStatement or BoundSwitchDispatch]);
Debug.Assert(!this.SwitchArms.IsEmpty);
}
}
12 changes: 12 additions & 0 deletions src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1525,6 +1525,18 @@
<Field Name="WasTargetTyped" Type="bool" />
</Node>

<Node Name="BoundLoweredSwitchExpression" Base="BoundExpression" HasValidate="true">
<Field Name="Type" Type="TypeSymbol" Override="true" Null="disallow"/>
<Field Name="Statements" Type="ImmutableArray&lt;BoundStatement&gt;"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these statements? Either use more descriptive name, or consider adding a comment

<Field Name="SwitchArms" Type="ImmutableArray&lt;BoundLoweredSwitchExpressionArm&gt;"/>
</Node>

<Node Name="BoundLoweredSwitchExpressionArm" Base="BoundNode">
<Field Name="Locals" Type="ImmutableArray&lt;LocalSymbol&gt;"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it will be preferred to not introduce new nodes with locals that survive lowering. Can we somehow represent what we want with combination of existing nodes?

Copy link
Contributor Author

@alrz alrz Mar 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These are the same locals in BoundScope.Locals

roslyn/src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_SwitchExpression.cs

Lines 114 to 116 in 37bdfe3

// Note the language scope of the locals, even though they are included for the purposes of
// lifetime analysis in the enclosing scope.
result.Add(new BoundScope(arm.Syntax, arm.Locals, statements));

BoundScope is rewritten in MethodToClassRewriter, otherwise DefineScopeLocals is used on locals. To cover the first part, I think we could use this existing node in the spiller (I suppose that's where this is relevant) and insert BoundScope instead of rewriting the node itself with assignments, if that sounds right to you I can try it in the next iteration.

<Field Name="Statements" Type="ImmutableArray&lt;BoundStatement&gt;"/>
<Field Name="Value" Type="BoundExpression" Null="disallow"/>
</Node>

<Node Name="BoundDecisionDag" Base="BoundNode">
<Field Name="RootNode" Type="BoundDecisionDagNode" Null="disallow"/>
</Node>
Expand Down
73 changes: 53 additions & 20 deletions src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,10 @@ private void EmitExpressionCore(BoundExpression expression, bool used)
EmitRefValueOperator((BoundRefValueOperator)expression, used);
break;

case BoundKind.LoweredSwitchExpression:
EmitLoweredSwitchExpression((BoundLoweredSwitchExpression)expression, used);
break;

case BoundKind.LoweredIsPatternExpression:
EmitLoweredIsPatternExpression((BoundLoweredIsPatternExpression)expression, used);
break;
Expand Down Expand Up @@ -366,6 +370,42 @@ private void EmitExpressionCore(BoundExpression expression, bool used)
}
}

private void EmitLoweredSwitchExpression(BoundLoweredSwitchExpression node, bool used)
{
EmitSideEffects(node.Statements);

var doneLabel = new object();
for (int i = 0, n = node.SwitchArms.Length; i < n; i++)
{
var switchArm = node.SwitchArms[i];
if (!switchArm.Locals.IsEmpty)
{
_builder.OpenLocalScope();
DefineScopeLocals(switchArm.Locals);
}

EmitSideEffects(switchArm.Statements);
EmitExpression(switchArm.Value, used);
if (used)
{
EmitStaticCastIfNeeded(node, switchArm.Value);
}

if (!switchArm.Locals.IsEmpty)
{
_builder.CloseLocalScope();
}

_builder.EmitBranch(ILOpCode.Br, doneLabel);
if (used && i != n - 1)
{
_builder.AdjustStack(-1);
}
}

_builder.MarkLabel(doneLabel);
}

private void EmitLoweredIsPatternExpression(BoundLoweredIsPatternExpression node, bool used, bool sense = true)
{
EmitSideEffects(node.Statements);
Expand Down Expand Up @@ -3814,18 +3854,9 @@ private void EmitConditionalOperator(BoundConditionalOperator expr, bool used)
// it seems that either PEVerify or the runtime/JIT verifier will complain at you if you try to remove
// either of the casts.
//
var mergeTypeOfAlternative = StackMergeType(expr.Alternative);
if (used)
{
if (IsVarianceCast(expr.Type, mergeTypeOfAlternative))
{
EmitStaticCast(expr.Type, expr.Syntax);
mergeTypeOfAlternative = expr.Type;
}
else if (expr.Type.IsInterfaceType() && !TypeSymbol.Equals(expr.Type, mergeTypeOfAlternative, TypeCompareKind.ConsiderEverything2))
{
EmitStaticCast(expr.Type, expr.Syntax);
}
EmitStaticCastIfNeeded(expr, expr.Alternative);
}

_builder.EmitBranch(ILOpCode.Br, doneLabel);
Expand All @@ -3840,21 +3871,23 @@ private void EmitConditionalOperator(BoundConditionalOperator expr, bool used)

if (used)
{
var mergeTypeOfConsequence = StackMergeType(expr.Consequence);
if (IsVarianceCast(expr.Type, mergeTypeOfConsequence))
{
EmitStaticCast(expr.Type, expr.Syntax);
mergeTypeOfConsequence = expr.Type;
}
else if (expr.Type.IsInterfaceType() && !TypeSymbol.Equals(expr.Type, mergeTypeOfConsequence, TypeCompareKind.ConsiderEverything2))
{
EmitStaticCast(expr.Type, expr.Syntax);
}
EmitStaticCastIfNeeded(expr, expr.Consequence);
}

_builder.MarkLabel(doneLabel);
}

private void EmitStaticCastIfNeeded(BoundExpression node, BoundExpression expression)
{
Debug.Assert(node.Type is not null);
TypeSymbol stackMergeType = StackMergeType(expression);
if (IsVarianceCast(node.Type, stackMergeType) ||
(node.Type.IsInterfaceType() && !TypeSymbol.Equals(node.Type, stackMergeType, TypeCompareKind.ConsiderEverything2)))
{
EmitStaticCast(node.Type, node.Syntax);
}
}

/// <summary>
/// Emit code for a null-coalescing operator.
/// </summary>
Expand Down
15 changes: 10 additions & 5 deletions src/Compilers/CSharp/Portable/CodeGen/EmitStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,16 @@ private void EmitScope(BoundScope block)

_builder.OpenLocalScope();

foreach (var local in block.Locals)
DefineScopeLocals(block.Locals);

EmitStatements(block.Statements);

_builder.CloseLocalScope();
}

private void DefineScopeLocals(ImmutableArray<LocalSymbol> locals)
{
foreach (var local in locals)
{
Debug.Assert(local.Name != null);
Debug.Assert(local.SynthesizedKind == SynthesizedLocalKind.UserDefined &&
Expand All @@ -831,10 +840,6 @@ private void EmitScope(BoundScope block)
_builder.AddLocalToScope(_builder.LocalSlotManager.GetLocal(local));
}
}

EmitStatements(block.Statements);

_builder.CloseLocalScope();
}

private void EmitStateMachineScope(BoundStateMachineScope scope)
Expand Down
29 changes: 28 additions & 1 deletion src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ public BoundNode VisitStatement(BoundNode node)
return VisitSideEffect(node);
}

public ImmutableArray<BoundStatement> VisitSideEffects(ImmutableArray<BoundStatement> statements)
public ImmutableArray<T> VisitSideEffects<T>(ImmutableArray<T> statements) where T : BoundNode
{
#if DEBUG
int prevStack = _expectedStackDepth;
Expand Down Expand Up @@ -1467,6 +1467,26 @@ public override BoundNode VisitSwitchDispatch(BoundSwitchDispatch node)
return node.Update(boundExpression, node.Cases, node.DefaultLabel, node.LengthBasedStringSwitchDataOpt);
}

#if DEBUG
public override BoundNode VisitLoweredSwitchExpression(BoundLoweredSwitchExpression node)
{
return node.Update(
VisitSideEffects(node.Statements),
VisitSideEffects(node.SwitchArms),
VisitType(node.Type));
}
#endif

public override BoundNode VisitLoweredSwitchExpressionArm(BoundLoweredSwitchExpressionArm node)
{
var statements = VisitSideEffects(node.Statements);
var value = VisitExpression(node.Value, ExprContext.Value);
PopEvalStack();
_counter++;
EnsureOnlyEvalStack();
Comment on lines +1484 to +1486
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as exception filters which basically throws away the value and fence it against whatever comes after. I wonder if this is more restrictive than what's done for conditional operators? Also, I didn't understand what is being compensated for here since there's no skipped node.

return node.Update(node.Locals, statements, value);
}

public override BoundNode VisitLoweredIsPatternExpression(BoundLoweredIsPatternExpression node)
{
var statements = VisitSideEffects(node.Statements);
Expand Down Expand Up @@ -2300,6 +2320,13 @@ BoundExpression visitArgumentsAndUpdateCall(BoundCall node, BoundExpression? rec
}
#nullable disable

public override BoundNode VisitLoweredSwitchExpressionArm(BoundLoweredSwitchExpressionArm switchArm)
{
var result = base.VisitLoweredSwitchExpressionArm(switchArm);
_nodeCounter++;
return result;
}

public override BoundNode VisitCatchBlock(BoundCatchBlock node)
{
var exceptionSource = node.ExceptionSourceOpt;
Expand Down
17 changes: 17 additions & 0 deletions src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass_Switch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,5 +181,22 @@ private BoundNode VisitSwitchExpression(BoundSwitchExpression node)
SetState(endState);
return node;
}

public override BoundNode VisitLoweredSwitchExpression(BoundLoweredSwitchExpression node)
{
var initialState = this.State.Clone();
var endState = UnreachableState();
VisitStatements(node.Statements);
foreach (var switchArm in node.SwitchArms)
{
SetState(initialState.Clone());
VisitStatements(switchArm.Statements);
VisitRvalue(switchArm.Value);
Join(ref endState, ref this.State);
}

SetState(endState);
return node;
}
}
}
Loading
Loading