Do not explicitly pass type to VNForMapStore
#61882
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The code was trying to obtain the value of the location to store to from the updated map for the element. This is simply incorrect for all but the trivial case of an empty field sequence.
It must always be equal to the type of the map being updated, not having redundancy eliminates the possibility for mistakes.
ghost
added
the
community-contribution
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsNext prerequisite in preparation for typing maps with placeholder. Separate as there are significant diffs. First, there is a fix for how type mismatch was accounted for in Which is that we will no longer pass explicit types to There are significant diffs from this, because this way we fix type mismatches arising in code such as the following: private static int Problem(StructWithIndex a, StructWithIndex b)
{
a.Index = 1;
b = a;
return b.Index;
}
struct StructWithIndex
{
public int Index;
public int Value;
}(Struct promotion must be disabled for this sample code) Previously, when we numbered ***** BB01, STMT00000(before)
N003 ( 5, 6) [000004] -A--G---R--- * ASG int
N002 ( 3, 4) [000003] U------N---- +--* LCL_FLD int V00 arg0 ud:1->2[+0] Fseq[Index]
N001 ( 1, 1) [000002] ------------ \--* CNS_INT int 1
N001 [000002] CNS_INT 1 => $41 {IntCns 1}
VNApplySelectors:
VNForHandle(Index) is $100, fieldType is int
VNForMapSelect($80, $100):int returns $140 {$80[$100]}
VNApplySelectors:
VNForHandle(Index) is $100, fieldType is int
VNForMapSelect($80, $100):int returns $140 {$80[$100]}
N002 [000003] LCL_FLD V00 arg0 ud:1->2[+0] Fseq[Index] => $140 {$80[$100]}
VNApplySelectorsAssign:
VNForHandle(Index) is $100, fieldType is int
VNForMapStore($80, $100, $41):int in BB01 returns $180 {$80[$100 := $41]}
VNApplySelectorsAssign:
VNForHandle(Index) is $100, fieldType is int
VNForMapStore($80, $100, $41):int in BB01 returns $180 {$80[$100 := $41]}
N002 [000003] LCL_FLD V00 arg0 ud:1->2[+0] Fseq[Index] => $180 {$80[$100 := $41]}
N003 [000004] ASG => $41 {IntCns 1}This is because This lead to mismatch on copy: ***** BB01, STMT00001(before)
N003 ( 7, 5) [000008] -A------R--- * ASG struct (copy)
N002 ( 3, 2) [000006] D------N---- +--* LCL_VAR struct<StructWithIndex, 8> V01 arg1 d:2
N001 ( 3, 2) [000005] ------------ \--* LCL_VAR struct<StructWithIndex, 8> V00 arg0 u:2 (last use)
N001 [000005] LCL_VAR V00 arg0 u:2 (last use) => $180 {$80[$100 := $41]}
*** Mismatched types in VNApplySelectorsTypeCheck (indType is TYP_STRUCT)
*** Mismatched types in VNApplySelectorsTypeCheck (indType is TYP_STRUCT)
*** Mismatched types in VNApplySelectorsAssignTypeCoerce (indType is TYP_STRUCT)
*** Mismatched types in VNApplySelectorsAssignTypeCoerce (indType is TYP_STRUCT)
Tree [000008] assigned VN to local var V01/2: <l:$1c3 {MemOpaque:NotInLoop}, c:$1c2 {MemOpaque:NotInLoop}>
N003 [000008] ASG => $VN.VoidAnd we lost track of the value. After this change, we no longer do, and this enables some nice improvements. Win-x64 diffsbenchmarks.run.windows.x64.checked.mch:Detail diffscoreclr_tests.pmi.windows.x64.checked.mch:Detail diffslibraries.pmi.windows.x64.checked.mch:Detail diffslibraries_tests.pmi.windows.x64.checked.mch:Detail diffsAll regressions I checked appear to be of the usual CSE-related flavor.
|
SingleAccretion
changed the title
VNForMapStore
|
@dotnet/jit-contrib |
|
/azp run Fuzzlyn, runtime-coreclr outerloop |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Fuzzlyn failures appear to be known. The outerloop is almost green... except for one suspicious failure on Looking at the pipeline's recent history, the configuration in question has a higher than average number of failures, but not with these symptoms. It appears the exception is coming from the XUnit process, and that should not be using the runtime under test. So based on that I think it is reasonable to request a retry here and see what happens. |
Done.
There's actually an interesting win x86 failure there, but it's not related to your PR (reproduces on main as well). Been trying to come up with an isolated example that triggers it, but for some reason this is difficult. It reproduces only when executed through Fuzzlyn for some reason. |
|
Looks like the retry passed. |
jakobbotsch
approved these changes
Nov 22, 2021
SingleAccretion
deleted the
Improve-Handling-Of-Type-Mismatch-In-VN-Part-Four
branch
This was referenced Nov 29, 2021
ghost
locked as resolved and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Next prerequisite in preparation for typing maps with placeholder types. Separate as there are significant diffs.
First, there is a fix for how type mismatch was accounted for in
fgValueNumberArrIndexAssign- we need to get the type of the last field. Note that at that pointfldSeqonly contains real fields. This fix is needed to minimize diffs for the second change.Which is that we will no longer pass explicit types to
VNForMapStoreand instead derive them frommap, as it reduces the possibility for mismatches and mistakes.There are significant diffs from this, because this way we fix type mismatches arising in code such as the following:
(Struct promotion must be disabled for this sample code)
Previously, when we numbered
a.Index = 1, we gave the whole struct aMapStoreVN ofTYP_INT:This is because
VNApplySelectorsAssigngave the last map it returned a type of the field, which is not correct.This lead to mismatch on copy:
And we lost track of the value. After this change, we no longer do, and this enables some nice improvements.
Win-x64 diffs
benchmarks.run.windows.x64.checked.mch:
Detail diffs
coreclr_tests.pmi.windows.x64.checked.mch:
Detail diffs
libraries.pmi.windows.x64.checked.mch:
Detail diffs
libraries_tests.pmi.windows.x64.checked.mch:
Detail diffs
All regressions I checked appear to be of the usual CSE-related flavor.
Overall diffs.
Part of #58312.