Skip to content

Removed JIT (uint) cast workaround to elide bound-checks in CoreLib #67448

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jkotas merged 13 commits into from
Jul 12, 2022
Merged

Removed JIT (uint) cast workaround to elide bound-checks in CoreLib #67448

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
cf4ccd4
Removed (uint) cast workaround in CoreLib, annotated missing places w…
gfoidl Apr 1, 2022
af9b2d6
Fixed the bug I introduced in TimeSpanParse.TimeSpanTokenizer
gfoidl Apr 1, 2022
19b8e00
Use uint-cast on both sided for clarity
gfoidl Apr 2, 2022
33cd7a1
Added missing namespace (oops)
gfoidl Apr 2, 2022
20842e6
Use uint-division in BitHelper as the JIT can produce more efficient …
gfoidl Apr 4, 2022
da31c0b
Don't have struct local in ValueStringBuilder due to hitting JIT opti…
gfoidl Apr 4, 2022
7e816a5
BitHelper with uint division and without Math.DivRem for even better …
gfoidl Apr 4, 2022
d9f4e01
BitHelper needs local Span due to regression in jit-diff otherwise
gfoidl Apr 4, 2022
034dd89
Fixed dumb copy & paste failure in BitHelper.IsMarked
gfoidl Apr 5, 2022
76d6d2a
Merge branch 'main' into remove-bound-check-hack
gfoidl May 23, 2022
186fb58
Added Debug.Asserts to BitHelper's method that take bitPosition
gfoidl Jul 11, 2022
6afa6c2
BitHelper: comment for workaround to tracking issue
gfoidl Jul 12, 2022
26f459f
Update src/libraries/System.Private.CoreLib/src/System/Collections/Ge…
gfoidl Jul 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions src/libraries/Common/src/System/Collections/Generic/BitHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Runtime.CompilerServices;

namespace System.Collections.Generic
{
internal ref struct BitHelper
Expand All @@ -19,19 +21,22 @@ internal BitHelper(Span<int> span, bool clear)

internal void MarkBit(int bitPosition)
{
Copy link
Member

@tannergooding tannergooding Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Can we add a Debug.Assert(bitPosition >= 0)? Same for the other two methods here.

The previous logic, would've done say -1 / 32 == 0 and now will do uint.MaxValue / 32 = 134217727

Copy link
Member

@tannergooding tannergooding Jul 11, 2022

Choose a reason for hiding this comment

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

It might even be better to just keep this one "as is" since its really doing the same thing, just with more casts now (that is comparing uint to uint)

int bitArrayIndex = bitPosition / IntSize;
if ((uint)bitArrayIndex < (uint)_span.Length)
(int bitArrayIndex, int position) = Math.DivRem(bitPosition, IntSize);
Copy link
Member

@jkotas jkotas Apr 2, 2022

Choose a reason for hiding this comment

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

There is a lot of cruft that the JIT has to optimize out now. Is this code with DivRem as good as it was before this change?

Copy link
Member Author

@gfoidl gfoidl Apr 3, 2022

Choose a reason for hiding this comment

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

The division is done only once, thus less machine code is created.
But for "Final local variable assignments" I see that JIT has more work.
Diffs show(ed) an improvement by looking at that change in isolation.

I assume that

runtime/src/libraries/Common/src/System/Collections/Generic/BitHelper.cs

Line 26 in 33cd7a1

if ((uint)bitArrayIndex < (uint)span.Length)
will always be true, so saving the extra division should be beneficial.

asm (x64) current main 
; Assembly listing for method System.Collections.Generic.BitHelper:IsMarked(int):bool:this
; Emitting BLENDED_CODE for X64 CPU with SSE2 - Windows
; ReadyToRun compilation
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 this         [V00,T01] (  4,  3.50)   byref  ->  rcx         this single-def
;  V01 arg1         [V01,T00] (  7,  5.50)     int  ->  rdx         single-def
;  V02 loc0         [V02,T02] (  3,  2.50)     int  ->  rax        
;# V03 OutArgs      [V03    ] (  1,  1   )  lclBlk ( 0) [rsp+00H]   "OutgoingArgSpace"
;* V04 tmp1         [V04    ] (  0,  0   )   byref  ->  zero-ref    single-def "Span.get_Item ptrToSpan"
;* V05 tmp2         [V05    ] (  0,  0   )   byref  ->  zero-ref    "Inlining Arg"
;  V06 cse0         [V06,T03] (  2,  2   )     int  ->   r8         "CSE - aggressive"
;
; Lcl frame size = 0

G_M54322_IG01:
						;; size=0 bbWeight=0.50 PerfScore 0.00
G_M54322_IG02:
       mov      eax, edx
       sar      eax, 31
       and      eax, 31
       add      eax, edx
       sar      eax, 5
       mov      r8d, dword ptr [rcx+8]
       cmp      eax, r8d
       jae      SHORT G_M54322_IG05
						;; size=22 bbWeight=1    PerfScore 5.00
G_M54322_IG03:
       mov      rcx, bword ptr [rcx]
       mov      eax, eax
       mov      eax, dword ptr [rcx+4*rax]
       mov      ecx, edx
       sar      ecx, 31
       and      ecx, 31
       add      ecx, edx
       and      ecx, -32
       sub      edx, ecx
       bt       eax, edx
       setb     al
       movzx    rax, al
						;; size=32 bbWeight=0.50 PerfScore 3.88
G_M54322_IG04:
       ret      
						;; size=1 bbWeight=0.50 PerfScore 0.50
G_M54322_IG05:
       xor      eax, eax
						;; size=2 bbWeight=0.50 PerfScore 0.12
G_M54322_IG06:
       ret      
						;; size=1 bbWeight=0.50 PerfScore 0.50

; Total bytes of code 58, prolog size 0, PerfScore 15.80, instruction count 23, allocated bytes for code 58 (MethodHash=300a2bcd) for method System.Collections.Generic.BitHelper:IsMarked(int):bool:this
; ============================================================

; Assembly listing for method System.Collections.Generic.BitHelper:MarkBit(int):this
; Emitting BLENDED_CODE for X64 CPU with SSE2 - Windows
; ReadyToRun compilation
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 this         [V00,T01] (  4,  3.50)   byref  ->  rcx         this single-def
;  V01 arg1         [V01,T00] (  7,  5.50)     int  ->  rdx         single-def
;  V02 loc0         [V02,T03] (  3,  2.50)     int  ->  rax        
;# V03 OutArgs      [V03    ] (  1,  1   )  lclBlk ( 0) [rsp+00H]   "OutgoingArgSpace"
;* V04 tmp1         [V04    ] (  0,  0   )   byref  ->  zero-ref    single-def "Span.get_Item ptrToSpan"
;  V05 tmp2         [V05,T02] (  3,  3   )   byref  ->  rax         single-def "dup spill"
;* V06 tmp3         [V06    ] (  0,  0   )   byref  ->  zero-ref    "Inlining Arg"
;  V07 cse0         [V07,T04] (  2,  2   )     int  ->   r8         "CSE - aggressive"
;
; Lcl frame size = 0

G_M49794_IG01:
						;; size=0 bbWeight=1    PerfScore 0.00
G_M49794_IG02:
       mov      eax, edx
       sar      eax, 31
       and      eax, 31
       add      eax, edx
       sar      eax, 5
       mov      r8d, dword ptr [rcx+8]
       cmp      eax, r8d
       jae      SHORT G_M49794_IG04
						;; size=22 bbWeight=1    PerfScore 5.00
G_M49794_IG03:
       mov      rcx, bword ptr [rcx]
       mov      eax, eax
       lea      rax, bword ptr [rcx+4*rax]
       mov      ecx, edx
       sar      ecx, 31
       and      ecx, 31
       add      ecx, edx
       and      ecx, -32
       sub      edx, ecx
       mov      r8d, 1
       mov      ecx, edx
       shl      r8d, cl
       or       dword ptr [rax], r8d
						;; size=38 bbWeight=0.50 PerfScore 5.00
G_M49794_IG04:
       ret      
						;; size=1 bbWeight=1    PerfScore 1.00

; Total bytes of code 61, prolog size 0, PerfScore 17.10, instruction count 22, allocated bytes for code 61 (MethodHash=de493d7d) for method System.Collections.Generic.BitHelper:MarkBit(int):this
; ============================================================
asm (x64) PR 
; Assembly listing for method System.Collections.Generic.BitHelper:IsMarked(int):bool:this
; Emitting BLENDED_CODE for X64 CPU with SSE2 - Windows
; ReadyToRun compilation
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 3 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 this         [V00,T01] (  4,  4   )   byref  ->  rcx         this single-def
;  V01 arg1         [V01,T00] (  5,  5   )     int  ->  rdx         single-def
;  V02 loc0         [V02,T03] (  3,  2.50)     int  ->  rax        
;  V03 loc1         [V03,T07] (  2,  1.50)     int  ->  rdx        
;* V04 loc2         [V04    ] (  0,  0   )  struct (16) zero-ref    ld-addr-op
;# V05 OutArgs      [V05    ] (  1,  1   )  lclBlk ( 0) [rsp+00H]   "OutgoingArgSpace"
;* V06 tmp1         [V06    ] (  0,  0   )  struct ( 8) zero-ref    "dup spill"
;  V07 tmp2         [V07,T02] (  3,  3   )     int  ->  rax         "Inline stloc first use temp"
;* V08 tmp3         [V08    ] (  0,  0   )  struct ( 8) zero-ref    "NewObj constructor temp"
;* V09 tmp4         [V09    ] (  0,  0   )     int  ->  zero-ref    "Inlining Arg"
;  V10 tmp5         [V10,T06] (  2,  1.50)   byref  ->   r8         single-def V04._pointer(offs=0x00) P-INDEP "field V04._pointer (fldOffset=0x0)"
;  V11 tmp6         [V11,T04] (  2,  2   )     int  ->  rcx         V04._length(offs=0x08) P-INDEP "field V04._length (fldOffset=0x8)"
;* V12 tmp7         [V12    ] (  0,  0   )     int  ->  zero-ref    V06.Item1(offs=0x00) P-INDEP "field V06.Item1 (fldOffset=0x0)"
;* V13 tmp8         [V13    ] (  0,  0   )     int  ->  zero-ref    V06.Item2(offs=0x04) P-INDEP "field V06.Item2 (fldOffset=0x4)"
;* V14 tmp9         [V14    ] (  0,  0   )     int  ->  zero-ref    V08.Item1(offs=0x00) P-INDEP "field V08.Item1 (fldOffset=0x0)"
;  V15 tmp10        [V15,T05] (  2,  2   )     int  ->  rdx         V08.Item2(offs=0x04) P-INDEP "field V08.Item2 (fldOffset=0x4)"
;
; Lcl frame size = 0

G_M54322_IG01:
						;; size=0 bbWeight=0.50 PerfScore 0.00
G_M54322_IG02:
       mov      eax, edx
       sar      eax, 31
       and      eax, 31
       add      eax, edx
       sar      eax, 5
       mov      r8d, eax
       shl      r8d, 5
       sub      edx, r8d
       mov      r8, bword ptr [rcx]
       mov      ecx, dword ptr [rcx+8]
       cmp      eax, ecx
       jae      SHORT G_M54322_IG05
						;; size=33 bbWeight=1    PerfScore 8.00
G_M54322_IG03:
       mov      eax, eax
       mov      eax, dword ptr [r8+4*rax]
       bt       eax, edx
       setb     al
       movzx    rax, al
						;; size=15 bbWeight=0.50 PerfScore 2.00
G_M54322_IG04:
       ret      
						;; size=1 bbWeight=0.50 PerfScore 0.50
G_M54322_IG05:
       xor      eax, eax
						;; size=2 bbWeight=0.50 PerfScore 0.12
G_M54322_IG06:
       ret      
						;; size=1 bbWeight=0.50 PerfScore 0.50

; Total bytes of code 52, prolog size 0, PerfScore 16.33, instruction count 20, allocated bytes for code 52 (MethodHash=300a2bcd) for method System.Collections.Generic.BitHelper:IsMarked(int):bool:this
; ============================================================

; Assembly listing for method System.Collections.Generic.BitHelper:MarkBit(int):this
; Emitting BLENDED_CODE for X64 CPU with SSE2 - Windows
; ReadyToRun compilation
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 3 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 this         [V00,T01] (  4,  4   )   byref  ->  rcx         this single-def
;  V01 arg1         [V01,T00] (  5,  5   )     int  ->  rdx         single-def
;  V02 loc0         [V02,T04] (  3,  2.50)     int  ->  rax        
;  V03 loc1         [V03,T08] (  2,  1.50)     int  ->  rdx        
;* V04 loc2         [V04    ] (  0,  0   )  struct (16) zero-ref    ld-addr-op
;# V05 OutArgs      [V05    ] (  1,  1   )  lclBlk ( 0) [rsp+00H]   "OutgoingArgSpace"
;* V06 tmp1         [V06    ] (  0,  0   )  struct ( 8) zero-ref    "dup spill"
;  V07 tmp2         [V07,T02] (  3,  3   )   byref  ->  rax         single-def "dup spill"
;  V08 tmp3         [V08,T03] (  3,  3   )     int  ->  rax         "Inline stloc first use temp"
;* V09 tmp4         [V09    ] (  0,  0   )  struct ( 8) zero-ref    "NewObj constructor temp"
;* V10 tmp5         [V10    ] (  0,  0   )     int  ->  zero-ref    "Inlining Arg"
;  V11 tmp6         [V11,T07] (  2,  1.50)   byref  ->   r8         single-def V04._pointer(offs=0x00) P-INDEP "field V04._pointer (fldOffset=0x0)"
;  V12 tmp7         [V12,T05] (  2,  2   )     int  ->  rcx         V04._length(offs=0x08) P-INDEP "field V04._length (fldOffset=0x8)"
;* V13 tmp8         [V13    ] (  0,  0   )     int  ->  zero-ref    V06.Item1(offs=0x00) P-INDEP "field V06.Item1 (fldOffset=0x0)"
;* V14 tmp9         [V14    ] (  0,  0   )     int  ->  zero-ref    V06.Item2(offs=0x04) P-INDEP "field V06.Item2 (fldOffset=0x4)"
;* V15 tmp10        [V15    ] (  0,  0   )     int  ->  zero-ref    V09.Item1(offs=0x00) P-INDEP "field V09.Item1 (fldOffset=0x0)"
;  V16 tmp11        [V16,T06] (  2,  2   )     int  ->  rdx         V09.Item2(offs=0x04) P-INDEP "field V09.Item2 (fldOffset=0x4)"
;
; Lcl frame size = 0

G_M49794_IG01:
						;; size=0 bbWeight=1    PerfScore 0.00
G_M49794_IG02:
       mov      eax, edx
       sar      eax, 31
       and      eax, 31
       add      eax, edx
       sar      eax, 5
       mov      r8d, eax
       shl      r8d, 5
       sub      edx, r8d
       mov      r8, bword ptr [rcx]
       mov      ecx, dword ptr [rcx+8]
       cmp      eax, ecx
       jae      SHORT G_M49794_IG04
						;; size=33 bbWeight=1    PerfScore 8.00
G_M49794_IG03:
       mov      ecx, eax
       lea      rax, bword ptr [r8+4*rcx]
       mov      r8d, 1
       mov      ecx, edx
       shl      r8d, cl
       or       dword ptr [rax], r8d
						;; size=20 bbWeight=0.50 PerfScore 3.12
G_M49794_IG04:
       ret      
						;; size=1 bbWeight=1    PerfScore 1.00

; Total bytes of code 54, prolog size 0, PerfScore 17.53, instruction count 19, allocated bytes for code 54 (MethodHash=de493d7d) for method System.Collections.Generic.BitHelper:MarkBit(int):this
; ============================================================

Copy link
Member

@jkotas jkotas Apr 3, 2022

Choose a reason for hiding this comment

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

This division does not need to be signed. The input is always non-negative. Would it be better to cast the input to uint before the division? Then it is going to be just a single shift and a single mask without the additional instructions to get the sign right.

Copy link
Member Author

@gfoidl gfoidl Apr 3, 2022

Choose a reason for hiding this comment

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

Good point 👍🏻. The divisor is constant too, so that becomes a cheap operation -- like you said.
(Will update later, tomorrow -- being on mobile now).

Span<int> span = _span;
if ((uint)bitArrayIndex < (uint)span.Length)
{
_span[bitArrayIndex] |= (1 << (bitPosition % IntSize));
span[bitArrayIndex] |= 1 << position;
}
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal bool IsMarked(int bitPosition)
{
int bitArrayIndex = bitPosition / IntSize;
(int bitArrayIndex, int position) = Math.DivRem(bitPosition, IntSize);
Span<int> span = _span;
Copy link
Member

@jkotas jkotas Jul 11, 2022

Choose a reason for hiding this comment

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

This one can use a tracking issue. The JIT should be smart enough to make it unnecessary to cache the Span here.

Copy link
Member Author

@gfoidl gfoidl Jul 12, 2022

Choose a reason for hiding this comment

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

Filed #72004 for this.

return
(uint)bitArrayIndex < (uint)_span.Length &&
(_span[bitArrayIndex] & (1 << (bitPosition % IntSize))) != 0;
(uint)bitArrayIndex < (uint)span.Length &&
(span[bitArrayIndex] & (1 << position)) != 0;
}

/// <summary>How many ints must be allocated to represent n bits. Returns (n+31)/32, but avoids overflow.</summary>
Expand Down
10 changes: 6 additions & 4 deletions src/libraries/Common/src/System/Text/ValueStringBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,10 @@ public void Insert(int index, string? s)
public void Append(char c)
{
int pos = _pos;
if ((uint)pos < (uint)_chars.Length)
Span<char> chars = _chars;
Copy link
Member

@jkotas jkotas Apr 2, 2022

Choose a reason for hiding this comment

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

Are all codegen deltas from this change possitive?

Append methods are often called a lot from a single caller. Extra aggresive inlined struct local may push us over JIT optimization limits.

Copy link
Member Author

@gfoidl gfoidl Apr 3, 2022

Choose a reason for hiding this comment

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

Before the bound-check didn't get elided, that change does it.
I think the JIT missed the optimization here, especially as it's a ref struct thus _chars can't be changed in between the length-check and the index-access.

The jit-diff shows for this change in isolation an improvement, only one method regresses. It's System.Number:NumberToStringFormat, and unfortunately a huge method, so I have troubles by manually checking the cause for this.

jit-diff for only this change 
Found 2 files with textual diffs.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 3188507
Total bytes of diff: 3186045
Total bytes of delta: -2462 (-0.08 % of base)
Total relative delta: -2.60
    diff is an improvement.
    relative diff is an improvement.


Top file improvements (bytes):
       -2462 : System.Private.CoreLib.dasm (-0,08 % of base)

1 total files with Code Size differences (1 improved, 0 regressed), 0 unchanged.

Top method regressions (bytes):
         498 (7,61 % of base) : System.Private.CoreLib.dasm - System.Number:NumberToStringFormat(byref,byref,System.ReadOnlySpan`1[System.Char],System.Globalization.NumberFormatInfo)

Top method improvements (bytes):
        -538 (-6,38 % of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector256`1:ToString(System.String,System.IFormatProvider):System.String:this (12 methods)
        -538 (-6,61 % of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector64`1:ToString(System.String,System.IFormatProvider):System.String:this (12 methods)
        -534 (-7,16 % of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1:ToString(System.String,System.IFormatProvider):System.String:this (12 methods)
        -115 (-7,65 % of base) : System.Private.CoreLib.dasm - StringSerializer:GetSerializedString(System.TimeZoneInfo):System.String
         -84 (-8,85 % of base) : System.Private.CoreLib.dasm - System.Number:NumberToString(byref,byref,ushort,int,System.Globalization.NumberFormatInfo)
         -75 (-11,06 % of base) : System.Private.CoreLib.dasm - StringSerializer:SerializeTransitionTime(System.TimeZoneInfo+TransitionTime,byref)
         -63 (-6,98 % of base) : System.Private.CoreLib.dasm - System.ApplicationId:ToString():System.String:this
         -60 (-3,72 % of base) : System.Private.CoreLib.dasm - System.Reflection.AssemblyNameFormatter:ComputeDisplayName(System.String,System.Version,System.String,System.Byte[],int,int):System.String
         -59 (-8,31 % of base) : System.Private.CoreLib.dasm - System.PasteArguments:AppendArgument(byref,System.String)
         -56 (-10,63 % of base) : System.Private.CoreLib.dasm - System.Number:FormatGeneral(byref,byref,int,System.Globalization.NumberFormatInfo,ushort,bool)
         -48 (-7,92 % of base) : System.Private.CoreLib.dasm - System.Reflection.RuntimeMethodInfo:ToString():System.String:this
         -45 (-8,46 % of base) : System.Private.CoreLib.dasm - System.Reflection.AssemblyNameFormatter:AppendQuoted(byref,System.String)
         -43 (-5,75 % of base) : System.Private.CoreLib.dasm - System.Reflection.CustomAttributeData:ToString():System.String:this
         -41 (-9,60 % of base) : System.Private.CoreLib.dasm - RTDynamicMethod:ToString():System.String:this
         -34 (-1,72 % of base) : System.Private.CoreLib.dasm - System.Reflection.CustomAttributeTypedArgument:ToString(bool):System.String:this
         -33 (-7,60 % of base) : System.Private.CoreLib.dasm - System.Reflection.RuntimePropertyInfo:ToString():System.String:this
         -32 (-4,65 % of base) : System.Private.CoreLib.dasm - System.IO.Enumeration.FileSystemName:TranslateWin32Expression(System.String):System.String
         -28 (-2,64 % of base) : System.Private.CoreLib.dasm - System.Number:FormatFixed(byref,byref,int,System.Int32[],System.String,System.String)
         -27 (-2,85 % of base) : System.Private.CoreLib.dasm - System.Net.WebUtility:HtmlDecode(System.ReadOnlySpan`1[System.Char],byref)
         -27 (-3,13 % of base) : System.Private.CoreLib.dasm - System.String:Join(System.String,System.Collections.Generic.IEnumerable`1[System.String]):System.String
         -26 (-3,16 % of base) : System.Private.CoreLib.dasm - System.Globalization.CalendarData:NormalizeDatePattern(System.String):System.String
         -26 (-7,10 % of base) : System.Private.CoreLib.dasm - System.Reflection.RuntimeConstructorInfo:ToString():System.String:this
         -22 (-6,73 % of base) : System.Private.CoreLib.dasm - System.Number:FormatExponent(byref,System.Globalization.NumberFormatInfo,int,ushort,int,bool)
         -20 (-2,62 % of base) : System.Private.CoreLib.dasm - System.PasteArguments:Paste(System.Collections.Generic.IEnumerable`1[System.String],bool):System.String
         -19 (-2,94 % of base) : System.Private.CoreLib.dasm - System.IO.Path:Combine(System.String[]):System.String
         -19 (-4,51 % of base) : System.Private.CoreLib.dasm - System.Number:FormatPercent(byref,byref,int,System.Globalization.NumberFormatInfo)
         -18 (-3,75 % of base) : System.Private.CoreLib.dasm - System.Reflection.MethodBase:AppendParameters(byref,System.Type[],int)
         -18 (-2,69 % of base) : System.Private.CoreLib.dasm - System.String:ReplaceLineEndings(System.String):System.String:this
         -18 (-0,89 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:AppendFormatHelper(System.IFormatProvider,System.String,System.ParamsArray):this
         -17 (-9,24 % of base) : System.Private.CoreLib.dasm - StringSerializer:SerializeSubstitute(System.String,byref)
         -17 (-4,67 % of base) : System.Private.CoreLib.dasm - System.Number:FormatScientific(byref,byref,int,System.Globalization.NumberFormatInfo,ushort)
         -16 (-1,75 % of base) : System.Private.CoreLib.dasm - StringSerializer:GetNextStringValue():System.String:this
         -16 (-1,50 % of base) : System.Private.CoreLib.dasm - System.IO.Path:GetRelativePath(System.String,System.String,int):System.String
         -16 (-3,86 % of base) : System.Private.CoreLib.dasm - System.Number:FormatCurrency(byref,byref,int,System.Globalization.NumberFormatInfo)
         -16 (-1,81 % of base) : System.Private.CoreLib.dasm - System.String:JoinCore(System.ReadOnlySpan`1[System.Char],System.Collections.Generic.IEnumerable`1[System.__Canon]):System.String
         -15 (-2,58 % of base) : System.Private.CoreLib.dasm - System.IO.Path:Join(System.String[]):System.String
         -15 (-3,08 % of base) : System.Private.CoreLib.dasm - System.String:JoinCore(System.ReadOnlySpan`1[System.Char],System.Object[]):System.String
         -15 (-3,80 % of base) : System.Private.CoreLib.dasm - System.TypeNameParser:EscapeTypeName(System.String):System.String
         -13 (-1,89 % of base) : System.Private.CoreLib.dasm - System.Net.WebUtility:HtmlEncode(System.ReadOnlySpan`1[System.Char],byref)
         -12 (-4,07 % of base) : System.Private.CoreLib.dasm - System.DateTimeParse:TryParseQuoteString(System.ReadOnlySpan`1[System.Char],int,byref,byref):bool
         -12 (-1,92 % of base) : System.Private.CoreLib.dasm - System.IO.PathInternal:RemoveRelativeSegments(System.ReadOnlySpan`1[System.Char],int,byref):bool
         -11 (-2,26 % of base) : System.Private.CoreLib.dasm - System.IO.PathInternal:NormalizeDirectorySeparators(System.String):System.String
         -11 (-0,94 % of base) : System.Private.CoreLib.dasm - System.Reflection.AssemblyNameParser:GetNextToken(byref):int:this
         -11 (-12,50 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:Append(System.String):this
         -11 (-16,42 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:Append(ushort):this
         -10 (-11,76 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:GrowAndAppend(ushort):this
          -9 (-1,65 % of base) : System.Private.CoreLib.dasm - System.String:Concat(System.Collections.Generic.IEnumerable`1[System.__Canon]):System.String
          -9 (-1,87 % of base) : System.Private.CoreLib.dasm - System.String:Concat(System.Collections.Generic.IEnumerable`1[System.String]):System.String
          -9 (-2,56 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:AppendSpanFormattable(double,System.String,System.IFormatProvider):this
          -9 (-3,31 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:AppendSpanFormattable(int,System.String,System.IFormatProvider):this
          -9 (-3,33 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:AppendSpanFormattable(System.__Canon,System.String,System.IFormatProvider):this
          -9 (-3,19 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:AppendSpanFormattable(System.DateTime,System.String,System.IFormatProvider):this
          -6 (-2,00 % of base) : System.Private.CoreLib.dasm - System.Number:FormatNumber(byref,byref,int,System.Globalization.NumberFormatInfo)

Top method regressions (percentages):
         498 (7,61 % of base) : System.Private.CoreLib.dasm - System.Number:NumberToStringFormat(byref,byref,System.ReadOnlySpan`1[System.Char],System.Globalization.NumberFormatInfo)

Top method improvements (percentages):
         -11 (-16,42 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:Append(ushort):this
         -11 (-12,50 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:Append(System.String):this
         -10 (-11,76 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:GrowAndAppend(ushort):this
         -75 (-11,06 % of base) : System.Private.CoreLib.dasm - StringSerializer:SerializeTransitionTime(System.TimeZoneInfo+TransitionTime,byref)
         -56 (-10,63 % of base) : System.Private.CoreLib.dasm - System.Number:FormatGeneral(byref,byref,int,System.Globalization.NumberFormatInfo,ushort,bool)
         -41 (-9,60 % of base) : System.Private.CoreLib.dasm - RTDynamicMethod:ToString():System.String:this
         -17 (-9,24 % of base) : System.Private.CoreLib.dasm - StringSerializer:SerializeSubstitute(System.String,byref)
         -84 (-8,85 % of base) : System.Private.CoreLib.dasm - System.Number:NumberToString(byref,byref,ushort,int,System.Globalization.NumberFormatInfo)
         -45 (-8,46 % of base) : System.Private.CoreLib.dasm - System.Reflection.AssemblyNameFormatter:AppendQuoted(byref,System.String)
         -59 (-8,31 % of base) : System.Private.CoreLib.dasm - System.PasteArguments:AppendArgument(byref,System.String)
         -48 (-7,92 % of base) : System.Private.CoreLib.dasm - System.Reflection.RuntimeMethodInfo:ToString():System.String:this
        -115 (-7,65 % of base) : System.Private.CoreLib.dasm - StringSerializer:GetSerializedString(System.TimeZoneInfo):System.String
         -33 (-7,60 % of base) : System.Private.CoreLib.dasm - System.Reflection.RuntimePropertyInfo:ToString():System.String:this
        -534 (-7,16 % of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector128`1:ToString(System.String,System.IFormatProvider):System.String:this (12 methods)
         -26 (-7,10 % of base) : System.Private.CoreLib.dasm - System.Reflection.RuntimeConstructorInfo:ToString():System.String:this
         -63 (-6,98 % of base) : System.Private.CoreLib.dasm - System.ApplicationId:ToString():System.String:this
         -22 (-6,73 % of base) : System.Private.CoreLib.dasm - System.Number:FormatExponent(byref,System.Globalization.NumberFormatInfo,int,ushort,int,bool)
        -538 (-6,61 % of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector64`1:ToString(System.String,System.IFormatProvider):System.String:this (12 methods)
        -538 (-6,38 % of base) : System.Private.CoreLib.dasm - System.Runtime.Intrinsics.Vector256`1:ToString(System.String,System.IFormatProvider):System.String:this (12 methods)
         -43 (-5,75 % of base) : System.Private.CoreLib.dasm - System.Reflection.CustomAttributeData:ToString():System.String:this
         -17 (-4,67 % of base) : System.Private.CoreLib.dasm - System.Number:FormatScientific(byref,byref,int,System.Globalization.NumberFormatInfo,ushort)
         -32 (-4,65 % of base) : System.Private.CoreLib.dasm - System.IO.Enumeration.FileSystemName:TranslateWin32Expression(System.String):System.String
         -19 (-4,51 % of base) : System.Private.CoreLib.dasm - System.Number:FormatPercent(byref,byref,int,System.Globalization.NumberFormatInfo)
         -12 (-4,07 % of base) : System.Private.CoreLib.dasm - System.DateTimeParse:TryParseQuoteString(System.ReadOnlySpan`1[System.Char],int,byref,byref):bool
         -16 (-3,86 % of base) : System.Private.CoreLib.dasm - System.Number:FormatCurrency(byref,byref,int,System.Globalization.NumberFormatInfo)
         -15 (-3,80 % of base) : System.Private.CoreLib.dasm - System.TypeNameParser:EscapeTypeName(System.String):System.String
         -18 (-3,75 % of base) : System.Private.CoreLib.dasm - System.Reflection.MethodBase:AppendParameters(byref,System.Type[],int)
         -60 (-3,72 % of base) : System.Private.CoreLib.dasm - System.Reflection.AssemblyNameFormatter:ComputeDisplayName(System.String,System.Version,System.String,System.Byte[],int,int):System.String
          -9 (-3,33 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:AppendSpanFormattable(System.__Canon,System.String,System.IFormatProvider):this
          -9 (-3,31 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:AppendSpanFormattable(int,System.String,System.IFormatProvider):this
          -9 (-3,19 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:AppendSpanFormattable(System.DateTime,System.String,System.IFormatProvider):this
         -26 (-3,16 % of base) : System.Private.CoreLib.dasm - System.Globalization.CalendarData:NormalizeDatePattern(System.String):System.String
         -27 (-3,13 % of base) : System.Private.CoreLib.dasm - System.String:Join(System.String,System.Collections.Generic.IEnumerable`1[System.String]):System.String
         -15 (-3,08 % of base) : System.Private.CoreLib.dasm - System.String:JoinCore(System.ReadOnlySpan`1[System.Char],System.Object[]):System.String
         -19 (-2,94 % of base) : System.Private.CoreLib.dasm - System.IO.Path:Combine(System.String[]):System.String
         -27 (-2,85 % of base) : System.Private.CoreLib.dasm - System.Net.WebUtility:HtmlDecode(System.ReadOnlySpan`1[System.Char],byref)
         -18 (-2,69 % of base) : System.Private.CoreLib.dasm - System.String:ReplaceLineEndings(System.String):System.String:this
         -28 (-2,64 % of base) : System.Private.CoreLib.dasm - System.Number:FormatFixed(byref,byref,int,System.Int32[],System.String,System.String)
         -20 (-2,62 % of base) : System.Private.CoreLib.dasm - System.PasteArguments:Paste(System.Collections.Generic.IEnumerable`1[System.String],bool):System.String
         -15 (-2,58 % of base) : System.Private.CoreLib.dasm - System.IO.Path:Join(System.String[]):System.String
          -9 (-2,56 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:AppendSpanFormattable(double,System.String,System.IFormatProvider):this
         -11 (-2,26 % of base) : System.Private.CoreLib.dasm - System.IO.PathInternal:NormalizeDirectorySeparators(System.String):System.String
          -6 (-2,00 % of base) : System.Private.CoreLib.dasm - System.Number:FormatNumber(byref,byref,int,System.Globalization.NumberFormatInfo)
         -12 (-1,92 % of base) : System.Private.CoreLib.dasm - System.IO.PathInternal:RemoveRelativeSegments(System.ReadOnlySpan`1[System.Char],int,byref):bool
         -13 (-1,89 % of base) : System.Private.CoreLib.dasm - System.Net.WebUtility:HtmlEncode(System.ReadOnlySpan`1[System.Char],byref)
          -9 (-1,87 % of base) : System.Private.CoreLib.dasm - System.String:Concat(System.Collections.Generic.IEnumerable`1[System.String]):System.String
         -16 (-1,81 % of base) : System.Private.CoreLib.dasm - System.String:JoinCore(System.ReadOnlySpan`1[System.Char],System.Collections.Generic.IEnumerable`1[System.__Canon]):System.String
         -16 (-1,75 % of base) : System.Private.CoreLib.dasm - StringSerializer:GetNextStringValue():System.String:this
         -34 (-1,72 % of base) : System.Private.CoreLib.dasm - System.Reflection.CustomAttributeTypedArgument:ToString(bool):System.String:this
          -9 (-1,65 % of base) : System.Private.CoreLib.dasm - System.String:Concat(System.Collections.Generic.IEnumerable`1[System.__Canon]):System.String
         -16 (-1,50 % of base) : System.Private.CoreLib.dasm - System.IO.Path:GetRelativePath(System.String,System.String,int):System.String
         -11 (-0,94 % of base) : System.Private.CoreLib.dasm - System.Reflection.AssemblyNameParser:GetNextToken(byref):int:this
         -18 (-0,89 % of base) : System.Private.CoreLib.dasm - System.Text.ValueStringBuilder:AppendFormatHelper(System.IFormatProvider,System.String,System.ParamsArray):this

54 total methods with Code Size differences (53 improved, 1 regressed), 25071 unchanged.

--------------------------------------------------------------------------------

Besides the elimination of the bound-checks I see a difference in codegen at the end of that method.

G_M27859_IG04:
       add      rsp, 40
       ret      

G_M27859_IG05:
-      movzx    rdx, dx
-      lea      rax, [(reloc)]
G_M27859_IG05:
+      movzx    rdx, dx
+      call     [System.Text.ValueStringBuilder:GrowAndAppend(ushort):this]
+      nop   

G_M27859_IG06:
       add      rsp, 40
-      tail.jmp [rax]System.Text.ValueStringBuilder:GrowAndAppend(ushort):this

-G_M27859_IG07:
-      call     [CORINFO_HELP_RNGCHKFAIL]
-      int3

So a standard call instead of the tail-call.
When looking at the whole System.Private.CoreLib.dasm (as generated by jit-diff diff) shows that ValueStringBuilder.Append(ushort) gets inlined, so GrowAndAppend boils down a call anyway.

asm (x64) current main 
; Assembly listing for method System.Text.ValueStringBuilder:Append(ushort):this
; Emitting BLENDED_CODE for X64 CPU with SSE2 - Windows
; ReadyToRun compilation
; optimized code
; rsp based frame
; fully interruptible
; No PGO data
; 0 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 this         [V00,T00] (  7,  5.50)   byref  ->  rcx         this single-def
;  V01 arg1         [V01,T01] (  4,  3   )  ushort  ->  rdx         single-def
;  V02 loc0         [V02,T02] (  5,  3.50)     int  ->  rax        
;  V03 OutArgs      [V03    ] (  1,  1   )  lclBlk (32) [rsp+00H]   "OutgoingArgSpace"
;  V04 tmp1         [V04,T03] (  3,  3   )   byref  ->   r8         single-def "Span.get_Item ptrToSpan"
;* V05 tmp2         [V05    ] (  0,  0   )   byref  ->  zero-ref    "Inlining Arg"
;
; Lcl frame size = 40

G_M27859_IG01:
       sub      rsp, 40
						;; size=4 bbWeight=0    PerfScore 0.00
G_M27859_IG02:
       mov      eax, dword ptr [rcx+8]
       cmp      eax, dword ptr [rcx+24]
       jae      SHORT G_M27859_IG05
						;; size=8 bbWeight=1    PerfScore 6.00
G_M27859_IG03:
       lea      r8, bword ptr [rcx+16]
       cmp      eax, dword ptr [r8+8]
       jae      SHORT G_M27859_IG07
       mov      r8, bword ptr [r8]
       mov      r9d, eax
       mov      word  ptr [r8+2*r9], dx
       inc      eax
       mov      dword ptr [rcx+8], eax
						;; size=26 bbWeight=0.50 PerfScore 4.50
G_M27859_IG04:
       add      rsp, 40
       ret      
						;; size=5 bbWeight=0.50 PerfScore 0.62
G_M27859_IG05:
       movzx    rdx, dx
       lea      rax, [(reloc)]
						;; size=10 bbWeight=0.50 PerfScore 0.38
G_M27859_IG06:
       add      rsp, 40
       tail.jmp [rax]System.Text.ValueStringBuilder:GrowAndAppend(ushort):this
						;; size=7 bbWeight=0.50 PerfScore 1.12
G_M27859_IG07:
       call     [CORINFO_HELP_RNGCHKFAIL]
       int3     
						;; size=7 bbWeight=0    PerfScore 0.00

; Total bytes of code 67, prolog size 4, PerfScore 19.33, instruction count 20, allocated bytes for code 67 (MethodHash=b9d9932c) for method System.Text.ValueStringBuilder:Append(ushort):this
; ============================================================
asm (x64) PR 
; Assembly listing for method System.Text.ValueStringBuilder:Append(ushort):this
; Emitting BLENDED_CODE for X64 CPU with SSE2 - Windows
; ReadyToRun compilation
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 this         [V00,T00] (  6,  5   )   byref  ->  rcx         this single-def
;  V01 arg1         [V01,T02] (  4,  3   )  ushort  ->  rdx         single-def
;  V02 loc0         [V02,T03] (  4,  3   )     int  ->  rax        
;* V03 loc1         [V03    ] (  0,  0   )  struct (16) zero-ref    ld-addr-op
;  V04 OutArgs      [V04    ] (  1,  1   )  lclBlk (32) [rsp+00H]   "OutgoingArgSpace"
;  V05 tmp1         [V05,T05] (  2,  1.50)   byref  ->   r9         single-def V03._pointer(offs=0x00) P-INDEP "field V03._pointer (fldOffset=0x0)"
;  V06 tmp2         [V06,T04] (  2,  2   )     int  ->   r8         V03._length(offs=0x08) P-INDEP "field V03._length (fldOffset=0x8)"
;  V07 tmp3         [V07,T01] (  3,  6   )   byref  ->   r8         single-def "BlockOp address local"
;
; Lcl frame size = 40

G_M27859_IG01:
       sub      rsp, 40
						;; size=4 bbWeight=0.50 PerfScore 0.12
G_M27859_IG02:
       mov      eax, dword ptr [rcx+8]
       lea      r8, bword ptr [rcx+16]
       mov      r9, bword ptr [r8]
       mov      r8d, dword ptr [r8+8]
       cmp      eax, r8d
       jae      SHORT G_M27859_IG05
						;; size=19 bbWeight=1    PerfScore 7.75
G_M27859_IG03:
       mov      r8d, eax
       mov      word  ptr [r9+2*r8], dx
       inc      eax
       mov      dword ptr [rcx+8], eax
						;; size=13 bbWeight=0.50 PerfScore 1.25
G_M27859_IG04:
       add      rsp, 40
       ret      
						;; size=5 bbWeight=0.50 PerfScore 0.62
G_M27859_IG05:
       movzx    rdx, dx
       call     [System.Text.ValueStringBuilder:GrowAndAppend(ushort):this]
       nop      
						;; size=10 bbWeight=0.50 PerfScore 1.75
G_M27859_IG06:
       add      rsp, 40
       ret      
						;; size=5 bbWeight=0.50 PerfScore 0.62

; Total bytes of code 56, prolog size 4, PerfScore 17.73, instruction count 18, allocated bytes for code 56 (MethodHash=b9d9932c) for method System.Text.ValueStringBuilder:Append(ushort):this
; ============================================================

Copy link
Member

@jkotas jkotas Apr 3, 2022

Choose a reason for hiding this comment

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

It's System.Number:NumberToStringFormat, and unfortunately a huge method

We care about number formatting performance quite a bit. It would be useful to understand the performance impact of this change on number formatting.

Copy link
Member Author

@gfoidl gfoidl Apr 4, 2022

Choose a reason for hiding this comment

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

NumberToStringFormat-asm.zip

Compared the asm block by block, and starting at G_M25536_IG74 I see more stack-work (spilling?) happening.
Tracked back to C# code this is at

runtime/src/libraries/System.Private.CoreLib/src/System/Number.Formatting.cs

Lines 2076 to 2077 in c830e33

if (number.IsNegative && (section == 0) && (number.Scale != 0))
sb.Append(info.NegativeSign);
, so the first time the ValueStringBuilder is used in that method.

I guess

Extra aggresive inlined struct local may push us over JIT optimization limits.

is the case here.

In conclusion I think it's the best to revert the change in ValueStringBuilder, and fix the JIT for

I think the JIT missed the optimization here, especially as it's a ref struct thus _chars can't be changed in between the length-check and the index-access.

so we get the best of both worlds.
If JIT is fixed we should get the improvements in the jit-diff anyway without regressing System.Number:NumberToStringFormat.

Hm, I don't find any issue for this missed optimization, but believe to remember that this was already mentioned somewhere else.

if ((uint)pos < (uint)chars.Length)
{
_chars[pos] = c;
chars[pos] = c;
_pos = pos + 1;
}
else
Expand All @@ -190,9 +191,10 @@ public void Append(string? s)
}

int pos = _pos;
if (s.Length == 1 && (uint)pos < (uint)_chars.Length) // very common case, e.g. appending strings from NumberFormatInfo like separators, percent symbols, etc.
Span<char> chars = _chars;
if (s.Length == 1 && (uint)pos < (uint)chars.Length) // very common case, e.g. appending strings from NumberFormatInfo like separators, percent symbols, etc.
{
_chars[pos] = s[0];
chars[pos] = s[0];
_pos = pos + 1;
}
else
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Boolean.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ public bool TryFormat(Span<char> destination, out int charsWritten)
{
if (m_value)
{
if ((uint)destination.Length > 3) // uint cast, per https://github.com/dotnet/runtime/issues/10596
if (destination.Length > 3)
{
ulong true_val = BitConverter.IsLittleEndian ? 0x65007500720054ul : 0x54007200750065ul; // "True"
MemoryMarshal.Write<ulong>(MemoryMarshal.AsBytes(destination), ref true_val);
Expand All @@ -109,7 +109,7 @@ public bool TryFormat(Span<char> destination, out int charsWritten)
}
else
{
if ((uint)destination.Length > 4)
if (destination.Length > 4)
{
ulong fals_val = BitConverter.IsLittleEndian ? 0x73006C00610046ul : 0x460061006C0073ul; // "Fals"
MemoryMarshal.Write<ulong>(MemoryMarshal.AsBytes(destination), ref fals_val);
Expand Down
3 changes: 1 addition & 2 deletions src/libraries/System.Private.CoreLib/src/System/Buffers/StandardFormat.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,7 @@ internal int Format(Span<char> destination)
int count = 0;
char symbol = Symbol;

if (symbol != default &&
(uint)destination.Length == FormatStringLength) // to eliminate bounds checks
if (symbol != default && destination.Length == FormatStringLength)
{
destination[0] = symbol;
count = 1;
Expand Down
4 changes: 2 additions & 2 deletions ...ies/System.Private.CoreLib/src/System/Buffers/Text/Utf8Formatter/Utf8Formatter.Boolean.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ public static bool TryFormat(bool value, Span<byte> destination, out int bytesWr
{
// This check can't be performed earlier because we need to throw if an invalid symbol is
// provided, even if the buffer is too small.
if ((uint)4 >= (uint)destination.Length)
if (destination.Length <= 4)
{
goto BufferTooSmall;
}
Expand All @@ -75,7 +75,7 @@ public static bool TryFormat(bool value, Span<byte> destination, out int bytesWr
}
else if (symbol == 'l')
{
if ((uint)4 >= (uint)destination.Length)
if (destination.Length <= 4)
{
goto BufferTooSmall;
}
Expand Down
4 changes: 1 addition & 3 deletions ...ries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Formatter/Utf8Formatter.Date.L.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ public static partial class Utf8Formatter
//
private static bool TryFormatDateTimeL(DateTime value, Span<byte> destination, out int bytesWritten)
{
// Writing the check in this fashion elides all bounds checks on 'buffer'
// for the remainder of the method.
if ((uint)28 >= (uint)destination.Length)
if (destination.Length <= 28)
{
bytesWritten = 0;
return false;
Expand Down
4 changes: 1 addition & 3 deletions ...ries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Formatter/Utf8Formatter.Date.R.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ public static partial class Utf8Formatter
//
private static bool TryFormatDateTimeR(DateTime value, Span<byte> destination, out int bytesWritten)
{
// Writing the check in this fashion elides all bounds checks on 'buffer'
// for the remainder of the method.
if ((uint)28 >= (uint)destination.Length)
if (destination.Length <= 28)
{
bytesWritten = 0;
return false;
Expand Down
35 changes: 17 additions & 18 deletions src/libraries/System.Private.CoreLib/src/System/Char.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,7 +508,7 @@ public static bool IsControl(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
Copy link
Member

@tannergooding tannergooding Jul 11, 2022

Choose a reason for hiding this comment

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

These ones seem to be touching code "stylistically", we probably should exclude it.

Copy link
Member Author

@gfoidl gfoidl Jul 11, 2022

Choose a reason for hiding this comment

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

W/o the superfluos paranthesis it's easier to read, thus I changed it.

Copy link
Member

@tannergooding tannergooding Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't disagree, but its also a "stylistic" change and so makes it harder to review the thing we actually care about. The typical PR policy is to not include such things and leave them to a separate PR, especially when its the only or primary change to a given file.

some operand swapping, etc. was done to get proper codegen. By reverting these to only have a mechanical remove of the uint-casts, we give up some (runtime) benefits.

We should have issues tracking these. We really shouldn't be getting codegen differences between x < y and y > x, especially for the cases where one input is constant.

{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -523,7 +523,7 @@ public static bool IsDigit(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -543,7 +543,7 @@ public static bool IsLetter(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -564,7 +564,7 @@ public static bool IsLetterOrDigit(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -584,8 +584,7 @@ public static bool IsLower(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}

if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand Down Expand Up @@ -627,7 +626,7 @@ public static bool IsNumber(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand Down Expand Up @@ -660,7 +659,7 @@ public static bool IsPunctuation(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand Down Expand Up @@ -705,7 +704,7 @@ public static bool IsSeparator(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -730,7 +729,7 @@ public static bool IsSurrogate(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand Down Expand Up @@ -762,7 +761,7 @@ public static bool IsSymbol(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -782,7 +781,7 @@ public static bool IsUpper(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -802,7 +801,7 @@ public static bool IsWhiteSpace(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -828,7 +827,7 @@ public static UnicodeCategory GetUnicodeCategory(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -852,7 +851,7 @@ public static double GetNumericValue(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -874,7 +873,7 @@ public static bool IsHighSurrogate(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -896,7 +895,7 @@ public static bool IsLowSurrogate(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand All @@ -913,7 +912,7 @@ public static bool IsSurrogatePair(string s, int index)
{
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.s);
}
if (((uint)index) >= ((uint)s.Length))
if ((uint)index >= (uint)s.Length)
{
ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.index);
}
Expand Down
5 changes: 3 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Collections/Generic/ValueListBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,10 @@ public ref T this[int index]
public void Append(T item)
{
int pos = _pos;
if ((uint)pos < (uint)_span.Length)
Span<T> span = _span;
if ((uint)pos < (uint)span.Length)
{
_span[pos] = item;
span[pos] = item;
_pos = pos + 1;
}
else
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Decimal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ public static int[] GetBits(decimal d)
/// <exception cref="ArgumentException">The destination span was not long enough to store the binary representation.</exception>
public static int GetBits(decimal d, Span<int> destination)
{
if ((uint)destination.Length <= 3)
if (destination.Length <= 3)
{
ThrowHelper.ThrowArgumentException_DestinationTooShort();
}
Expand All @@ -582,7 +582,7 @@ public static int GetBits(decimal d, Span<int> destination)
/// <returns>true if the decimal's binary representation was written to the destination; false if the destination wasn't long enough.</returns>
public static bool TryGetBits(decimal d, Span<int> destination, out int valuesWritten)
{
if ((uint)destination.Length <= 3)
if (destination.Length <= 3)
{
valuesWritten = 0;
return false;
Expand Down
10 changes: 5 additions & 5 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/DateTimeParse.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4671,7 +4671,7 @@ private static bool ParseFormatR(ReadOnlySpan<char> source, ref ParsingInfo pars
// Tue, 03 Jan 2017 08:08:05 GMT

// The format is exactly 29 characters.
if ((uint)source.Length != 29)
if (source.Length != 29)
{
result.SetBadDateTimeFailure();
return false;
Expand Down Expand Up @@ -4868,7 +4868,7 @@ private static bool ParseFormatO(ReadOnlySpan<char> source, ref DateTimeResult r
// 2017-06-12T05:30:45.7680000-7:00 (special-case of one-digit offset hour)
// 2017-06-12T05:30:45.7680000-07:00

if ((uint)source.Length < 27 ||
if (source.Length < 27 ||
source[4] != '-' ||
source[7] != '-' ||
source[10] != 'T' ||
Expand Down Expand Up @@ -4989,7 +4989,7 @@ private static bool ParseFormatO(ReadOnlySpan<char> source, ref DateTimeResult r
return false;
}

if ((uint)source.Length > 27)
if (source.Length > 27)
{
char offsetChar = source[27];
switch (offsetChar)
Expand All @@ -5007,7 +5007,7 @@ private static bool ParseFormatO(ReadOnlySpan<char> source, ref DateTimeResult r
case '-':
int offsetHours, colonIndex;

if ((uint)source.Length == 33)
if (source.Length == 33)
{
uint oh1 = (uint)(source[28] - '0'), oh2 = (uint)(source[29] - '0');

Expand All @@ -5020,7 +5020,7 @@ private static bool ParseFormatO(ReadOnlySpan<char> source, ref DateTimeResult r
offsetHours = (int)(oh1 * 10 + oh2);
colonIndex = 30;
}
else if ((uint)source.Length == 32) // special-case allowed for compat: only one offset hour digit
else if (source.Length == 32) // special-case allowed for compat: only one offset hour digit
{
offsetHours = source[28] - '0';

Expand Down
Loading