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

BoundDecisionDag.Rewrite - Avoid capturing the replacement map #58137

Merged
merged 1 commit into from
Dec 8, 2021
Merged

BoundDecisionDag.Rewrite - Avoid capturing the replacement map #58137

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 10 additions & 12 deletions src/Compilers/CSharp/Portable/BoundTree/BoundDecisionDag.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ public ImmutableArray<BoundDecisionDagNode> TopologicallySortedNodes
/// takes as its input the node to be rewritten and a function that returns the previously computed
/// rewritten node for successor nodes.
/// </summary>
public BoundDecisionDag Rewrite(Func<BoundDecisionDagNode, Func<BoundDecisionDagNode, BoundDecisionDagNode>, BoundDecisionDagNode> makeReplacement)
public BoundDecisionDag Rewrite(Func<BoundDecisionDagNode, IReadOnlyDictionary<BoundDecisionDagNode, BoundDecisionDagNode>, BoundDecisionDagNode> makeReplacement)
{
// First, we topologically sort the nodes of the dag so that we can translate the nodes bottom-up.
// This will avoid overflowing the compiler's runtime stack which would occur for a large switch
Expand All @@ -95,14 +95,12 @@ public BoundDecisionDag Rewrite(Func<BoundDecisionDagNode, Func<BoundDecisionDag
// a node's successors before the node, the replacement should always be in the cache when we need it.
var replacement = PooledDictionary<BoundDecisionDagNode, BoundDecisionDagNode>.GetInstance();

Func<BoundDecisionDagNode, BoundDecisionDagNode> getReplacementForChild = n => replacement[n];

// Loop backwards through the topologically sorted nodes to translate them, so that we always visit a node after its successors
for (int i = sortedNodes.Length - 1; i >= 0; i--)
{
BoundDecisionDagNode node = sortedNodes[i];
Debug.Assert(!replacement.ContainsKey(node));
BoundDecisionDagNode newNode = makeReplacement(node, getReplacementForChild);
BoundDecisionDagNode newNode = makeReplacement(node, replacement);
replacement.Add(node, newNode);
}

Expand All @@ -113,18 +111,18 @@ public BoundDecisionDag Rewrite(Func<BoundDecisionDagNode, Func<BoundDecisionDag
}

/// <summary>
/// A trivial node replacement function for use with <see cref="Rewrite(Func{BoundDecisionDagNode, Func{BoundDecisionDagNode, BoundDecisionDagNode}, BoundDecisionDagNode})"/>.
/// A trivial node replacement function for use with <see cref="Rewrite(Func{BoundDecisionDagNode, IReadOnlyDictionary{BoundDecisionDagNode, BoundDecisionDagNode}, BoundDecisionDagNode})"/>.
/// </summary>
public static BoundDecisionDagNode TrivialReplacement(BoundDecisionDagNode dag, Func<BoundDecisionDagNode, BoundDecisionDagNode> replacement)
public static BoundDecisionDagNode TrivialReplacement(BoundDecisionDagNode dag, IReadOnlyDictionary<BoundDecisionDagNode, BoundDecisionDagNode> replacement)
{
switch (dag)
{
case BoundEvaluationDecisionDagNode p:
return p.Update(p.Evaluation, replacement(p.Next));
return p.Update(p.Evaluation, replacement[p.Next]);
case BoundTestDecisionDagNode p:
return p.Update(p.Test, replacement(p.WhenTrue), replacement(p.WhenFalse));
return p.Update(p.Test, replacement[p.WhenTrue], replacement[p.WhenFalse]);
case BoundWhenDecisionDagNode p:
return p.Update(p.Bindings, p.WhenExpression, replacement(p.WhenTrue), (p.WhenFalse != null) ? replacement(p.WhenFalse) : null);
return p.Update(p.Bindings, p.WhenExpression, replacement[p.WhenTrue], (p.WhenFalse != null) ? replacement[p.WhenFalse] : null);
case BoundLeafDecisionDagNode p:
return p;
default:
Expand All @@ -149,17 +147,17 @@ public BoundDecisionDag SimplifyDecisionDagIfConstantInput(BoundExpression input
return Rewrite(makeReplacement);

// Make a replacement for a given node, using the precomputed replacements for its successors.
BoundDecisionDagNode makeReplacement(BoundDecisionDagNode dag, Func<BoundDecisionDagNode, BoundDecisionDagNode> replacement)
BoundDecisionDagNode makeReplacement(BoundDecisionDagNode dag, IReadOnlyDictionary<BoundDecisionDagNode, BoundDecisionDagNode> replacement)
{
if (dag is BoundTestDecisionDagNode p)
{
// This is the key to the optimization. The result of a top-level test might be known if the input is constant.
switch (knownResult(p.Test))
{
case true:
return replacement(p.WhenTrue);
return replacement[p.WhenTrue];
case false:
return replacement(p.WhenFalse);
return replacement[p.WhenFalse];
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter.PatternLocalRewriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -623,7 +623,7 @@ void storeToTemp(BoundDagTemp temp, BoundExpression expr)
}
}

BoundDecisionDagNode makeReplacement(BoundDecisionDagNode node, Func<BoundDecisionDagNode, BoundDecisionDagNode> replacement)
static BoundDecisionDagNode makeReplacement(BoundDecisionDagNode node, IReadOnlyDictionary<BoundDecisionDagNode, BoundDecisionDagNode> replacement)
Copy link
Member Author

Choose a reason for hiding this comment

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

Could be inlined as lambda for caching (this is the tuple input optimization which is common compared to rewrite for constant input)

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO when it comes to whether to use a lambda or to convert a local function to delegate, let's use whichever form we prefer to read, with the assumption that eventually the method group conversion to delegate caching will come about.

{
switch (node)
{
Expand All @@ -635,7 +635,7 @@ eval.Field is var field &&
field.TupleElementIndex is int i)
{
// The elements of an input tuple were evaluated beforehand, so don't need to be evaluated now.
return replacement(evalNode.Next);
return replacement[evalNode.Next];
}

// Since we are performing an optimization whose precondition is that the original
Expand Down