JIT: handle some side effects in box pattern match

[AndyAyersMS]

Handle simple side effects in the box; br pattern based optimization.

Fixes#1713.

[AndyAyersMS]

cc @dotnet/jit-contrib @jkotas

Seems like an important enough case to warrant handling in the jit, despite the small number of impacted methods. Does not hit outside of SPC.

TIER0

PMI CodeSize Diffs for System.Private.CoreLib.dll [tier0] for x64 default jit
Summary of Code Size diffs:
(Lower is better)
Total bytes of diff: -60 (-0.00% of base)
diff is an improvement.
Top file improvements (bytes):
-60 : System.Private.CoreLib.dasm (-0.00% of base)
1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged.
Top method improvements (bytes):
-21 (-6.86% of base) : System.Private.CoreLib.dasm - GenericEqualityComparer`1:IndexOf(ref,Vector`1,int,int):int:this
-21 (-6.82% of base) : System.Private.CoreLib.dasm - GenericEqualityComparer`1:LastIndexOf(ref,Vector`1,int,int):int:this
-9 (-3.23% of base) : System.Private.CoreLib.dasm - AsyncMethodBuilderCore:Start(byref)
-5 (-0.21% of base) : System.Private.CoreLib.dasm - StringBuilder:AppendJoinCore(long,int,ref):StringBuilder:this (7 methods)
-2 (-0.65% of base) : System.Private.CoreLib.dasm - ObjectEqualityComparer`1:IndexOf(ref,Vector`1,int,int):int:this
-2 (-0.65% of base) : System.Private.CoreLib.dasm - ObjectEqualityComparer`1:LastIndexOf(ref,Vector`1,int,int):int:this
Top method improvements (percentages):
-21 (-6.86% of base) : System.Private.CoreLib.dasm - GenericEqualityComparer`1:IndexOf(ref,Vector`1,int,int):int:this
-21 (-6.82% of base) : System.Private.CoreLib.dasm - GenericEqualityComparer`1:LastIndexOf(ref,Vector`1,int,int):int:this
-9 (-3.23% of base) : System.Private.CoreLib.dasm - AsyncMethodBuilderCore:Start(byref)
-2 (-0.65% of base) : System.Private.CoreLib.dasm - ObjectEqualityComparer`1:IndexOf(ref,Vector`1,int,int):int:this
-2 (-0.65% of base) : System.Private.CoreLib.dasm - ObjectEqualityComparer`1:LastIndexOf(ref,Vector`1,int,int):int:this
-5 (-0.21% of base) : System.Private.CoreLib.dasm - StringBuilder:AppendJoinCore(long,int,ref):StringBuilder:this (7 methods)
6 total methods with Code Size differences (6 improved, 0 regressed), 21938 unchanged.

TIER1

Tried a couple of experiments to mitigate the size regressions, but no luck. They are small.

PMI CodeSize Diffs for System.Private.CoreLib.dll, framework assemblies for x64 default jit
Summary of Code Size diffs:
(Lower is better)
Total bytes of diff: 74 (0.00% of base)
diff is a regression.
Top file regressions (bytes):
74 : System.Private.CoreLib.dasm (0.00% of base)
1 total files with Code Size differences (0 improved, 1 regressed), 164 unchanged.
Top method regressions (bytes):
29 ( 7.36% of base) : System.Private.CoreLib.dasm - GenericArraySortHelper`1:SwapIfGreater(Span`1,int,int) (5 methods)
28 ( 1.77% of base) : System.Private.CoreLib.dasm - GenericArraySortHelper`2:DownHeap(Span`1,Span`1,int,int,int) (5 methods)
23 ( 2.31% of base) : System.Private.CoreLib.dasm - GenericArraySortHelper`1:DownHeap(Span`1,int,int,int) (5 methods)
9 ( 1.52% of base) : System.Private.CoreLib.dasm - GenericArraySortHelper`2:SwapIfGreaterWithValues(Span`1,Span`1,int,int) (5 methods)
5 ( 0.31% of base) : System.Private.CoreLib.dasm - StringBuilder:AppendJoinCore(long,int,ref):StringBuilder:this (7 methods)
Top method improvements (bytes):
-9 (-5.06% of base) : System.Private.CoreLib.dasm - ObjectEqualityComparer`1:IndexOf(ref,Vector`1,int,int):int:this
-9 (-5.00% of base) : System.Private.CoreLib.dasm - ObjectEqualityComparer`1:LastIndexOf(ref,Vector`1,int,int):int:this
-2 (-0.88% of base) : System.Private.CoreLib.dasm - AsyncMethodBuilderCore:Start(byref)
Top method regressions (percentages):
29 ( 7.36% of base) : System.Private.CoreLib.dasm - GenericArraySortHelper`1:SwapIfGreater(Span`1,int,int) (5 methods)
23 ( 2.31% of base) : System.Private.CoreLib.dasm - GenericArraySortHelper`1:DownHeap(Span`1,int,int,int) (5 methods)
28 ( 1.77% of base) : System.Private.CoreLib.dasm - GenericArraySortHelper`2:DownHeap(Span`1,Span`1,int,int,int) (5 methods)
9 ( 1.52% of base) : System.Private.CoreLib.dasm - GenericArraySortHelper`2:SwapIfGreaterWithValues(Span`1,Span`1,int,int) (5 methods)
5 ( 0.31% of base) : System.Private.CoreLib.dasm - StringBuilder:AppendJoinCore(long,int,ref):StringBuilder:this (7 methods)
Top method improvements (percentages):
-9 (-5.06% of base) : System.Private.CoreLib.dasm - ObjectEqualityComparer`1:IndexOf(ref,Vector`1,int,int):int:this
-9 (-5.00% of base) : System.Private.CoreLib.dasm - ObjectEqualityComparer`1:LastIndexOf(ref,Vector`1,int,int):int:this
-2 (-0.88% of base) : System.Private.CoreLib.dasm - AsyncMethodBuilderCore:Start(byref)
8 total methods with Code Size differences (3 improved, 5 regressed), 244488 unchanged.

[jkotas]

What do the size regressions look like?

[CarolEidt]

LGTM

[AndyAyersMS]

Here is a typical diff, from GenericArraySortHelper`2:DownHeap: we CSE an address rather than a load from that address. Happens twice in this method, partially counteracted by one less register save.

;;; before;;; r11 is a span's pointer field, ecx is the span length, r15 the indexG_M45276_IG05:lear14d,[rax+rbp]lear15d,[r14-1]cmpr15d,ecxjae G_M45276_IG13movsxdr15,r15dmovr15, qword ptr [r11+8*r15]cmpr14d,ecxjae G_M45276_IG13movsxdr14,r14dmovr14, qword ptr [r11+8*r14]cmpr15,r14jl SHORT G_M45276_IG08 ;; bbWeight=2 PerfScore 18.50G_M45276_IG06:cmpr15,r14jle SHORT G_M45276_IG09;;; afterG_M45276_IG05:lear14d,[rax+rbp]lear15d,[r14-1]cmpr15d,ecxjae G_M45276_IG13movsxdr15,r15dlear15, bword ptr [r11+8*r15]cmp dword ptr [r15],r15d // residual null checkcmpr14d,ecxjae G_M45276_IG13movsxdr14,r14dmovr14, qword ptr [r11+8*r14]cmp qword ptr [r15],r14jl SHORT G_M45276_IG08 ;; bbWeight=2 PerfScore 23.00G_M45276_IG06:cmp qword ptr [r15],r14jle SHORT G_M45276_IG09

Without this change, at Tier1, the box;br .. sequence gets handled by the general importer box ops which express the nullcheck as an unconsumed indir; this happens to provide a dominating cse def for the subsequent loads.

So sometimes there's a benefit to a GT_IND based nullcheck, sometimes there's a benefit to GT_NULLCHECK; we can't tell locally which might end up working out better, and we can't normalize to whichever form is better downstream.