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

Share pattern-matching temps when they are the same type. #19378

Merged
merged 6 commits into from
May 11, 2017
Merged

Share pattern-matching temps when they are the same type. #19378

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4eb82b1
Share pattern-matching temps when they are the same type.
gafter May 9, 2017
11f360d
Update test to account for changed code in pattern-matching
gafter May 10, 2017
e088442
Merge branch 'master' of https://github.com/dotnet/roslyn into master…
gafter May 10, 2017
300a47e
Eliminate some unneeded parameters.
gafter May 10, 2017
c63b016
Add a test demonstrating that temps differing by tuple names or dynam…
gafter May 10, 2017
716d6df
Rename a test.
gafter May 10, 2017
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
5 changes: 3 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/PatternSwitchBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ internal override BoundStatement BindSwitchExpressionAndSections(SwitchStatement
BoundPatternSwitchLabel defaultLabel;
bool isComplete;
ImmutableArray<BoundPatternSwitchSection> switchSections =
BindPatternSwitchSections(boundSwitchExpression, node.Sections, originalBinder, out defaultLabel, out isComplete, diagnostics);
BindPatternSwitchSections(node, boundSwitchExpression, node.Sections, originalBinder, out defaultLabel, out isComplete, diagnostics);
var locals = GetDeclaredLocalsForScope(node);
var functions = GetDeclaredLocalFunctionsForScope(node);
return new BoundPatternSwitchStatement(
Expand Down Expand Up @@ -101,6 +101,7 @@ internal override void BindPatternSwitchLabelForInference(CasePatternSwitchLabel
/// the decision tree.
/// </summary>
private ImmutableArray<BoundPatternSwitchSection> BindPatternSwitchSections(
SyntaxNode syntax,
Copy link
Contributor

@AlekseyTs AlekseyTs May 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

SyntaxNode syntax [](start = 12, length = 17)

It looks like there is only one call-site. Consider making this a SwitchStatementSyntax and removing the two next parameters (boundSwitchExpression is coming from a property, sections are available from SwitchStatementSyntax). #Closed

Copy link
Member Author

@gafter gafter May 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes; even this syntax parameter is unneeded (it is available in the property SwitchSyntax).

BoundExpression boundSwitchExpression,
SyntaxList<SwitchSectionSyntax> sections,
Binder originalBinder,
Expand All @@ -115,7 +116,7 @@ private ImmutableArray<BoundPatternSwitchSection> BindPatternSwitchSections(

// Bind match sections
var boundPatternSwitchSectionsBuilder = ArrayBuilder<BoundPatternSwitchSection>.GetInstance();
SubsumptionDiagnosticBuilder subsumption = new SubsumptionDiagnosticBuilder(ContainingMemberOrLambda, this.Conversions, boundSwitchExpression);
SubsumptionDiagnosticBuilder subsumption = new SubsumptionDiagnosticBuilder(ContainingMemberOrLambda, syntax, this.Conversions, boundSwitchExpression);
foreach (var sectionSyntax in sections)
{
boundPatternSwitchSectionsBuilder.Add(BindPatternSwitchSection(
Expand Down
9 changes: 5 additions & 4 deletions src/Compilers/CSharp/Portable/Binder/SubsumptionDiagnosticBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@ internal class SubsumptionDiagnosticBuilder : DecisionTreeBuilder
private readonly DecisionTree _subsumptionTree;

internal SubsumptionDiagnosticBuilder(Symbol enclosingSymbol,
Conversions conversions,
BoundExpression expression)
: base(enclosingSymbol, conversions)
SyntaxNode syntax,
Conversions conversions,
BoundExpression expression)
: base(enclosingSymbol, syntax, conversions)
{
_subsumptionTree = DecisionTree.Create(expression, expression.Type, enclosingSymbol);
_subsumptionTree = CreateEmptyDecisionTree(expression);
}

/// <summary>
Expand Down
30 changes: 0 additions & 30 deletions src/Compilers/CSharp/Portable/BoundTree/DecisionTree.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,36 +78,6 @@ public DecisionTree(BoundExpression expression, TypeSymbol type, LocalSymbol tem
Debug.Assert(this.Type != null);
}

/// <summary>
/// Create a fresh decision tree for the given input expression of the given type.
/// </summary>
public static DecisionTree Create(BoundExpression expression, TypeSymbol type, Symbol enclosingSymbol)
{
Debug.Assert(expression.Type == type);
LocalSymbol temp = null;
if (expression.ConstantValue == null)
{
// Unless it is a constant, the decision tree acts on a copy of the input expression.
// We create a temp to represent that copy. Lowering will assign into this temp.
temp = new SynthesizedLocal(enclosingSymbol as MethodSymbol, type, SynthesizedLocalKind.PatternMatching, expression.Syntax, false, RefKind.None);
expression = new BoundLocal(expression.Syntax, temp, null, type);
}

if (type.CanContainNull() || type.SpecialType == SpecialType.None)
{
// We need the ByType decision tree to separate null from non-null values.
// Note that, for the purpose of the decision tree (and subsumption), we
// ignore the fact that the input may be a constant, and therefore always
// or never null.
return new ByType(expression, type, temp);
}
else
{
// If it is a (e.g. builtin) value type, we can switch on its (constant) values.
return new ByValue(expression, type, temp);
}
}

/// <summary>
/// A decision tree node that branches based on (1) whether the input value is null, (2) the runtime
/// type of the input expression, and finally (3) a default decision tree if nothing in the previous
Expand Down
50 changes: 48 additions & 2 deletions src/Compilers/CSharp/Portable/BoundTree/DecisionTreeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,63 @@ internal abstract class DecisionTreeBuilder
protected readonly Symbol _enclosingSymbol;
protected readonly Conversions _conversions;
protected HashSet<DiagnosticInfo> _useSiteDiagnostics = new HashSet<DiagnosticInfo>();
private Dictionary<TypeSymbol, LocalSymbol> localByType = new Dictionary<TypeSymbol, LocalSymbol>();
Copy link
Contributor

Choose a reason for hiding this comment

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

new Dictionary<TypeSymbol, LocalSymbol>(); [](start = 66, length = 42)

Should this dictionary be sensitive to dynamic/object, tuple names and custom modifier differences?

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, it definitely should not.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it definitely should not.

Please add a test targeting this scenario.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added TestIgnoreDynamicVsObjectAndTupleElementNames_03.

Copy link
Member Author

Choose a reason for hiding this comment

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

My "No" above should be "Yes".


protected DecisionTreeBuilder(
Symbol enclosingSymbol,
SyntaxNode syntax,
Conversions conversions)
{
this._enclosingSymbol = enclosingSymbol;
this.Syntax = syntax;
this._conversions = conversions;
}

protected SyntaxNode Syntax { private get; set; }

private LocalSymbol PatternMatchingTemp(TypeSymbol type)
{
LocalSymbol temp;
if (!localByType.TryGetValue(type, out temp))
{
temp = new SynthesizedLocal(_enclosingSymbol as MethodSymbol, type, SynthesizedLocalKind.PatternMatching, Syntax);
Copy link
Member

Choose a reason for hiding this comment

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

_enclosingSymbol as MethodSymbol [](start = 44, length = 32)

Can this be null? If not use explicit cast.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the parameter is "optional" in the constructor.

While it would be possible to change this to a cast now, that would be an error when we introduce a match expression, because in that case the enclosing symbol could be a field (for a field initializer). I'd rather leave this as is so it is future-proof.

localByType.Add(type, temp);
}

return temp;
}

/// <summary>
/// Create a fresh decision tree for the given input expression of the given type.
/// </summary>
protected DecisionTree CreateEmptyDecisionTree(BoundExpression expression)
{
var type = expression.Type;

LocalSymbol temp = null;
if (expression.ConstantValue == null)
{
// Unless it is a constant, the decision tree acts on a copy of the input expression.
// We create a temp to represent that copy. Lowering will assign into this temp.
temp = PatternMatchingTemp(type);
expression = new BoundLocal(expression.Syntax, temp, null, type);
}

if (type.CanContainNull() || type.SpecialType == SpecialType.None)
{
// We need the ByType decision tree to separate null from non-null values.
// Note that, for the purpose of the decision tree (and subsumption), we
// ignore the fact that the input may be a constant, and therefore always
// or never null.
return new DecisionTree.ByType(expression, type, temp);
}
else
{
// If it is a (e.g. builtin) value type, we can switch on its (constant) values.
return new DecisionTree.ByValue(expression, type, temp);
}
}

protected DecisionTree AddToDecisionTree(DecisionTree decisionTree, SyntaxNode sectionSyntax, BoundPatternSwitchLabel label)
{
var pattern = label.Pattern;
Expand Down Expand Up @@ -237,7 +283,7 @@ private DecisionTree AddByValue(DecisionTree.ByType byType, BoundConstantPattern
if (forType == null)
{
var type = value.Value.Type;
var localSymbol = new SynthesizedLocal(_enclosingSymbol as MethodSymbol, type, SynthesizedLocalKind.PatternMatching, Syntax, false, RefKind.None);
var localSymbol = PatternMatchingTemp(type);
var narrowedExpression = new BoundLocal(Syntax, localSymbol, null, type);
forType = new DecisionTree.ByValue(narrowedExpression, value.Value.Type.TupleUnderlyingTypeOrSelf(), localSymbol);
byType.TypeAndDecision.Add(new KeyValuePair<TypeSymbol, DecisionTree>(value.Value.Type, forType));
Expand Down Expand Up @@ -338,7 +384,7 @@ private DecisionTree AddByType(DecisionTree.ByType byType, TypeSymbol type, Deci

if (result == null)
{
var localSymbol = new SynthesizedLocal(_enclosingSymbol as MethodSymbol, type, SynthesizedLocalKind.PatternMatching, Syntax, false, RefKind.None);
var localSymbol = PatternMatchingTemp(type);
var expression = new BoundLocal(Syntax, localSymbol, null, type);
result = makeDecision(expression, type);
Debug.Assert(result.Temp == null);
Expand Down
19 changes: 4 additions & 15 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_PatternSwitchStatement.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ private class PatternSwitchLocalRewriter : DecisionTreeBuilder
private ArrayBuilder<BoundStatement> _loweredDecisionTree = ArrayBuilder<BoundStatement>.GetInstance();

private PatternSwitchLocalRewriter(LocalRewriter localRewriter, BoundPatternSwitchStatement node)
: base(localRewriter._factory.CurrentMethod, localRewriter._factory.Compilation.Conversions)
: base(localRewriter._factory.CurrentMethod, node.Syntax, localRewriter._factory.Compilation.Conversions)
{
this._localRewriter = localRewriter;
this._factory = localRewriter._factory;
Expand All @@ -59,15 +59,9 @@ private BoundStatement MakeLoweredForm(BoundPatternSwitchStatement node)
var expression = _localRewriter.VisitExpression(node.Expression);
var result = ArrayBuilder<BoundStatement>.GetInstance();

// if the expression is "too complex", we copy it to a temp.
Copy link
Contributor

Choose a reason for hiding this comment

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

// if the expression is "too complex", we copy it to a temp. [](start = 16, length = 60)

Could you please explain why this is no longer needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Lowering starts by lowering to a decision tree, which starts by calling CreateEmptyDecisionTree, which contains code equivalent to this deleted code. The existing generated code always contained two temps instead of one.

        /// <summary>
        /// Create a fresh decision tree for the given input expression of the given type.
        /// </summary>
        protected DecisionTree CreateEmptyDecisionTree(BoundExpression expression)
        {
            var type = expression.Type;

            LocalSymbol temp = null;
            if (expression.ConstantValue == null)
            {
                // Unless it is a constant, the decision tree acts on a copy of the input expression.
                // We create a temp to represent that copy. Lowering will assign into this temp.
                temp = PatternMatchingTemp(type);
                expression = new BoundLocal(expression.Syntax, temp, null, type);
            }

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation.

LocalSymbol initialTemp = null;
if (expression.ConstantValue == null)
{
initialTemp = _factory.SynthesizedLocal(expression.Type, expression.Syntax);
result.Add(_factory.Assignment(_factory.Local(initialTemp), expression));
expression = _factory.Local(initialTemp);

// EnC: We need to insert a hidden sequence point to handle function remapping in case
// EnC: We need to insert a hidden sequence point to handle function remapping in case
// the containing method is edited while methods invoked in the expression are being executed.
if (!node.WasCompilerGenerated && _localRewriter.Instrument)
{
Expand All @@ -85,7 +79,6 @@ private BoundStatement MakeLoweredForm(BoundPatternSwitchStatement node)
}
// at this point the end of result is unreachable.

_declaredTemps.AddOptional(initialTemp);
_declaredTemps.AddRange(node.InnerLocals);

// output the sections of code
Expand Down Expand Up @@ -148,7 +141,7 @@ private DecisionTree LowerToDecisionTree(
BoundExpression loweredExpression,
BoundPatternSwitchStatement node)
{
var loweredDecisionTree = DecisionTree.Create(loweredExpression, loweredExpression.Type, _enclosingSymbol);
var loweredDecisionTree = CreateEmptyDecisionTree(loweredExpression);
BoundPatternSwitchLabel defaultLabel = null;
SyntaxNode defaultSection = null;
foreach (var section in node.SwitchSections)
Expand Down Expand Up @@ -223,15 +216,11 @@ private void LowerDecisionTree(BoundExpression expression, DecisionTree decision
_loweredDecisionTree.Add(_factory.Assignment(decisionTree.Expression, convertedExpression));
}

// If the temp is not yet in the declared temp set, add it now
if (_declaredTempSet.Add(decisionTree.Temp))
{
_declaredTemps.Add(decisionTree.Temp);
}
else
{
// we should only attempt to declare each temp once.
throw ExceptionUtilities.Unreachable;
}
}

switch (decisionTree.Kind)
Expand Down
Loading