Improve elimination of redundant evaluations during pattern matching operation #82142
Conversation
|
@RikkiGibson, @333fred, @jcouv, @dotnet/roslyn-compiler Please review |
1 similar comment
|
@RikkiGibson, @333fred, @jcouv, @dotnet/roslyn-compiler Please review |
src/Compilers/CSharp/Test/Emit3/Semantics/PatternMatchingTests_ListPatterns.cs
Outdated
Show resolved
Hide resolved
|
@RikkiGibson, @333fred, @jcouv, @dotnet/roslyn-compiler Please review |
|
|
||
| if (!dagBuilder._forLowering && lengthValues.Any(BinaryOperatorKind.Equal, lengthValue)) | ||
| { | ||
| dagBuilder._suitableForLowering = false; |
There was a problem hiding this comment.
nit: Consider leaving a comment on why this is not suitable for lowering #Resolved
There was a problem hiding this comment.
This existing existing requirement. I did not add it
| <Field Name="IsExplicitTest" Type="bool"/> | ||
| </Node> | ||
| <Node Name="BoundDagExplicitNullTest" Base="BoundDagTest"> | ||
| <Node Name="BoundDagExplicitNullTest" Base="BoundDagTest" UpdateMethodModifiers="new"> |
There was a problem hiding this comment.
nit: instead of introducing new machinery, consider using a dedicated name like UpdateInput to avoid overloading Update
|
@RikkiGibson, @333fred, @dotnet/roslyn-compiler For a second review |
| // Then the process of removal made another update to that indexer evaluation. | ||
| // This means that the evaluation would somehow take an input that is calculated | ||
| // from itself, only then temps update would update the evaluation. | ||
| // But a circuarity like this is not possible with indexer evaluations. |
There was a problem hiding this comment.
| // But a circuarity like this is not possible with indexer evaluations. | |
| // But a circularity like this is not possible with indexer evaluations. |
333fred
left a comment
333fred
left a comment
There was a problem hiding this comment.
I did not review the tests, I'm leaving that to the other approvals.
| public readonly Tests? ConditionToUseFinalResult; | ||
| public readonly Tests? TempsUpdatedResult; |
There was a problem hiding this comment.
It would be useful to write out some of what was mentioned in the overview meeting on what these fields are useful for, why some of the branches need temps removed and some not.
|
Addressed comments look good. Not going to sign off since I didn't review all the tests and you already have 2 approvals, but there's nothing blocking from me. |
Unified equivalence implementation with equality implementation for BoundDagTemp and BoundDagEvaluation. This reduces scenarios when temp remapping is necessary. This also improves ability to merge equivalent Dag states. This is what `uniquifyState` function is doing. As a result we create a Decision Dag with less amount of states and emit smaller IL.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
4 participants
Fixes #82063