Skip to content

Rewrite selection for fields and always normalize SIMD types in VN - #61370

Merged
jakobbotsch merged 1 commit into
dotnet:mainfrom
SingleAccretion:Improve-Handling-Of-Type-Mismatch-In-VN-Part-Three
Nov 15, 2021
Merged

Rewrite selection for fields and always normalize SIMD types in VN#61370
jakobbotsch merged 1 commit into
dotnet:mainfrom
SingleAccretion:Improve-Handling-Of-Type-Mismatch-In-VN-Part-Three

Conversation

@SingleAccretion

@SingleAccretionSingleAccretion commented Nov 9, 2021

Copy link
Copy Markdown
Contributor

This is mirroring the changes made in #61113 in preparation for typing the maps with placeholders, meaning that ApplySelectors is no longer suitable when selecting the first field.

There are small diffs (win-x64, win-x86, win-arm64) with this change, all are coming from additional CSEs.

Notes on the diffs

The first kind is from the SIMD type normalization:

privatestaticVector128<int>Problem(StructWithVtors[]a){a[0].Vtor=Vector128<int>.Zero;returna[0].Vtor;}structStructWithVtors{publicVector128<int>Vtor;}

We are now able to VN return a[0].Vtor properly, as we no longer run into a type mismatch check where indType is some SIMD type while arrElemFldType is TYP_STRUCT, since it is effectively obtained from VNApplySelectorsAssign.

The second kind of diffs (show up on x86) are due to the new code not using this to get the first field's type:

var_types firstFieldType = vnStore->TypeOfVN(fldMapVN);

It was creating artificial mismatches in cases when we see maps of TYP_REF:

*****BB02, STMT00010(before)
N007 ( 11, 12) [000079] -A-XG---R---*ASG int N006 ( 1, 1) [000078] D------N----+--*LCL_VAR int V04 tmp2 d:2N005 ( 11, 12) [000048] ---XG----U--\--*CAST_ovfl int <- ulong
N004 ( 4, 4) [000047] ---XG-------\--*IND long N003 ( 2, 2) [000137] -------N----\--*ADD byref N001 ( 1, 1) [000046] ------------+--*LCL_VAR ref V00this u:1N002 ( 1, 1) [000136] ------------\--*CNS_INT int 4 field offset Fseq[hackishFieldName]
N001 [000046] LCL_VARV00this u:1=> $80 {InitVal($40)}
N002 [000136] CNS_INT4 field offset Fseq[hackishFieldName] => $45 {IntCns4}
N003 [000137] ADD=> $281 {ADD($45, $80)}
VNApplySelectors:VNForHandle(hackishFieldName) is $182, fieldType is long
AX2: $182!= $183==> select([$2c9]store($2c8, $183, $8d), $182) ==> select($2c8, $182) remaining budget is 99.
AX1: select([$345]store($2c8, $182, $8c), $182) ==> $8c.
==>Not updating loop memory dependence of [000047], memory $345 not defined in a loop
VNForMapSelect($2c9, $182):long returns $8c {MemOpaque:L00}
VNForMapSelect($8c, $80):ref returns $220 {$8c[$80]}
***Mismatched types in VNApplySelectorsTypeCheck (reading beyond the end)

I was concerned about the new normalization code impacting TP, but it seems that the redundant VNForMapSelects removed in #61113, combined with the redundant handle lookups removed in this PR actually result in a ~0.1% TP improvement (according to my noisy PIN over SPMI-ing the benchmarks collection a few times, anyway):

 N006 [000000] LCL_VAR V00 this u:1 => $80 {InitVal($40)}
N007 [000096] CNS_INT 12 field offset Fseq[mt] => $44 {IntCns 12}
N008 [000097] ADD => $280 {ADD($44, $80)}
- VNApplySelectors:
VNForHandle(mt) is $181, fieldType is ref
VNForMapSelect($81, $181):ref returns $203 {$81[$181]}
- VNForMapSelect($203, $80):ref returns $204 {$203[$80]}
VNForMapStore($203, $80, $240):ref in BB01 returns $2c0 {$203[$80 := $240]}
- VNApplySelectorsAssign:- VNForHandle(mt) is $181, fieldType is ref
VNForMapStore($81, $181, $2c0):ref in BB01 returns $2c1 {$81[$181 := $2c0]}
fgCurMemoryVN[GcHeap] assigned for StoreField at [000005] to VN: $2c1.

Part of #58312.

@SingleAccretionSingleAccretion added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 9, 2021
@ghost

ghost commented Nov 9, 2021

Copy link
Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

This is mirroring the changes made in #61113 in preparation for typing the maps with placeholders, meaning that ApplySelectors is no longer suitable when selecting the first field.

There are small diffs (win-x64, win-x86, win-arm64) with this change, all are coming from additional CSEs.

Notes on the diffs

The first kind is from the SIMD type normalization:

privatestaticVector128<int>Problem(StructWithVtors[]a){a[0].Vtor=Vector128<int>.Zero;returna[0].Vtor;}structStructWithVtors{publicVector128<int>Vtor;}

We are now able to VN return a[0].Vtor properly, as we no longer run into a type mismatch check where indType is some SIMD type while arrElemFldType is TYP_STRUCT, since it is effectively obtained from VNApplySelectorsAssign.

The second kind of diffs (show up on x86) are due to the new code not using this to get the first field's type:

var_types firstFieldType = vnStore->TypeOfVN(fldMapVN);

It was creating artificial mismatches in cases when we see maps of TYP_REF:

*****BB02, STMT00010(before)
N007 ( 11, 12) [000079] -A-XG---R---*ASG int N006 ( 1, 1) [000078] D------N----+--*LCL_VAR int V04 tmp2 d:2N005 ( 11, 12) [000048] ---XG----U--\--*CAST_ovfl int <- ulong
N004 ( 4, 4) [000047] ---XG-------\--*IND long N003 ( 2, 2) [000137] -------N----\--*ADD byref N001 ( 1, 1) [000046] ------------+--*LCL_VAR ref V00this u:1N002 ( 1, 1) [000136] ------------\--*CNS_INT int 4 field offset Fseq[hackishFieldName]
N001 [000046] LCL_VARV00this u:1=> $80 {InitVal($40)}
N002 [000136] CNS_INT4 field offset Fseq[hackishFieldName] => $45 {IntCns4}
N003 [000137] ADD=> $281 {ADD($45, $80)}
VNApplySelectors:VNForHandle(hackishFieldName) is $182, fieldType is long
AX2: $182!= $183==> select([$2c9]store($2c8, $183, $8d), $182) ==> select($2c8, $182) remaining budget is 99.
AX1: select([$345]store($2c8, $182, $8c), $182) ==> $8c.
==>Not updating loop memory dependence of [000047], memory $345 not defined in a loop
VNForMapSelect($2c9, $182):long returns $8c {MemOpaque:L00}
VNForMapSelect($8c, $80):ref returns $220 {$8c[$80]}
***Mismatched types in VNApplySelectorsTypeCheck (reading beyond the end)

I was concerned about the new normalization code impacting TP, but it seems that the redundant VNForMapSelects removed in #61113, combined with the redundant lookups removed in this PR actually result in a ~0.1% TP improvement (according to my noisy PIN over SPMI-ing the benchmarks collection a few times, anyway):

 N006 [000000] LCL_VAR V00 this u:1 => $80 {InitVal($40)}
N007 [000096] CNS_INT 12 field offset Fseq[mt] => $44 {IntCns 12}
N008 [000097] ADD => $280 {ADD($44, $80)}
- VNApplySelectors:
VNForHandle(mt) is $181, fieldType is ref
VNForMapSelect($81, $181):ref returns $203 {$81[$181]}
- VNForMapSelect($203, $80):ref returns $204 {$203[$80]}
VNForMapStore($203, $80, $240):ref in BB01 returns $2c0 {$203[$80 := $240]}
- VNApplySelectorsAssign:- VNForHandle(mt) is $181, fieldType is ref
VNForMapStore($81, $181, $2c0):ref in BB01 returns $2c1 {$81[$181 := $2c0]}
fgCurMemoryVN[GcHeap] assigned for StoreField at [000005] to VN: $2c1.
Author:SingleAccretion
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@SingleAccretion
SingleAccretionforce-pushed the Improve-Handling-Of-Type-Mismatch-In-VN-Part-Three branch 2 times, most recently from d5f48be to edcddb3CompareNovember 9, 2021 16:35
The issue was that VNApplySelectors uses the types
of fields when selecting, but that's not valid for
"the first field" maps.
Mirror how the stores to fields are numbered.
@SingleAccretion
SingleAccretionforce-pushed the Improve-Handling-Of-Type-Mismatch-In-VN-Part-Three branch from edcddb3 to ba83fbaCompareNovember 10, 2021 12:21
@SingleAccretion
SingleAccretion marked this pull request as ready for review November 10, 2021 14:31
@SingleAccretion

Copy link
Copy Markdown
ContributorAuthor

@dotnet/jit-contrib

@jakobbotschjakobbotsch left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. Thanks!

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@SingleAccretion@jakobbotsch@JulieLeeMSFT