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 |
|
@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. | |
| ``` #Resolved |
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). |
|
@jcouv, @RikkiGibson, @333fred I made some changes since you last look at the PR. I am still investigating |
This reverts commit ba1129f.
Relaxed 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 states and emit smaller IL.
I changed my mind about doing this specific rewrite. Especially that given the new equality implementation the new type evaluation might be considered equal to the one we are replacing, which makes their result temps equal and the temp remapping would be circular, causing an exception. Rather than complicating the logic, I decided to not do the rewrite because it does not result in complete elimination anyway. In reply to: 3843329071 Refers to: src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder.cs:2673 in 090043b. [](commit_id = 090043b, deletion_comment = True) |
Fixes #82063