Use unsigned casts for array/Span indexers #57970

pentp · 2021-08-23T20:07:29Z

Array and Span length is never negative and after bounds checking index is also guaranteed to be non-negative. Unsigned casts get rid of quite a few movsxd instructions and replace the rest with cheaper mov variants.

PMI diffs for System.Private.CoreLib show only 8 regressions, of which only a couple are because of CSE misses, most are from loop alignment changes.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 5517665
Total bytes of diff: 5513593
Total bytes of delta: -4072 (-0,07% of base)
Total relative delta: -10,86
    diff is an improvement.
    relative diff is an improvement.

Detailed diffs for win-x64

Top method regressions (bytes):
          20 ( 1,56% of base) : System.Private.CoreLib.dasm - PathHelper:TryExpandShortFileName(byref,String):String
           9 ( 1,29% of base) : System.Private.CoreLib.dasm - GregorianCalendarHelper:GetYearOffset(int,int,bool):int:this
           5 ( 0,90% of base) : System.Private.CoreLib.dasm - Encoder:GetBytes(long,int,long,int,bool):int:this
           4 ( 0,71% of base) : System.Private.CoreLib.dasm - Decoder:GetChars(long,int,long,int,bool):int:this
           3 ( 0,78% of base) : System.Private.CoreLib.dasm - String:<GetNonRandomizedHashCodeOrdinalIgnoreCase>g__GetNonRandomizedHashCodeOrdinalIgnoreCaseSlow|47_0(String):int
           1 ( 0,27% of base) : System.Private.CoreLib.dasm - Decoder:GetCharCount(long,int,bool):int:this
           1 ( 0,26% of base) : System.Private.CoreLib.dasm - Encoder:GetByteCount(long,int,bool):int:this
           1 ( 0,16% of base) : System.Private.CoreLib.dasm - RuntimeModule:ResolveSignature(int):ref:this

Top method improvements (bytes):
        -112 (-1,08% of base) : System.Private.CoreLib.dasm - List`1:ConvertAll(Converter`2):List`1:this (64 methods)
         -70 (-0,64% of base) : System.Private.CoreLib.dasm - DefaultBinder:BindToMethod(int,ref,byref,ref,CultureInfo,ref,byref):MethodBase:this
         -60 (-0,66% of base) : System.Private.CoreLib.dasm - HashSet`1:SymmetricExceptWithEnumerable(IEnumerable`1):this (8 methods)
         -55 (-1,13% of base) : System.Private.CoreLib.dasm - Dictionary`2:Resize(int,bool):this (10 methods)
         -42 (-0,55% of base) : System.Private.CoreLib.dasm - Enumerator:MoveNext():bool:this (74 methods)
         -37 (-1,09% of base) : System.Private.CoreLib.dasm - HashSet`1:Resize(int,bool):this (8 methods)
         -35 (-3,43% of base) : System.Private.CoreLib.dasm - ArraySortHelper`1:SwapIfGreater(Span`1,Comparison`1,int,int) (9 methods)
         -32 (-1,47% of base) : System.Private.CoreLib.dasm - ArraySortHelper`1:DownHeap(Span`1,int,int,Comparison`1) (8 methods)
         -32 (-0,44% of base) : System.Private.CoreLib.dasm - Dictionary`2:OnDeserialization(Object):this (8 methods)
         -31 (-0,41% of base) : System.Private.CoreLib.dasm - HashSet`1:OnDeserialization(Object):this (8 methods)
         -31 (-1,66% of base) : System.Private.CoreLib.dasm - List`1:RemoveAll(Predicate`1):int:this (8 methods)
         -30 (-0,82% of base) : System.Private.CoreLib.dasm - ArraySortHelper`2:DownHeap(Span`1,Span`1,int,int,IComparer`1) (8 methods)
         -30 (-0,50% of base) : System.Private.CoreLib.dasm - ArraySortHelper`2:PickPivotAndPartition(Span`1,Span`1,IComparer`1):int (8 methods)
         -30 (-0,77% of base) : System.Private.CoreLib.dasm - Vector128`1:ToString():String:this (6 methods)
         -30 (-0,77% of base) : System.Private.CoreLib.dasm - Vector256`1:ToString():String:this (6 methods)
         -29 (-0,35% of base) : System.Private.CoreLib.dasm - TlsOverPerCoreLockedStacksArrayPool`1:Rent(int):ref:this (10 methods)
         -28 (-0,46% of base) : System.Private.CoreLib.dasm - HashSet`1:IntersectWithEnumerable(IEnumerable`1):this (8 methods)
         -28 (-1,42% of base) : System.Private.CoreLib.dasm - PerCoreLockedStacks:TryPop():ref:this (8 methods)
         -28 (-0,76% of base) : System.Private.CoreLib.dasm - Vector64`1:ToString():String:this (6 methods)
         -24 (-0,54% of base) : System.Private.CoreLib.dasm - ConfigurableArrayPool`1:Rent(int):ref:this (8 methods)

Top method regressions (percentages):
          20 ( 1,56% of base) : System.Private.CoreLib.dasm - PathHelper:TryExpandShortFileName(byref,String):String
           9 ( 1,29% of base) : System.Private.CoreLib.dasm - GregorianCalendarHelper:GetYearOffset(int,int,bool):int:this
           5 ( 0,90% of base) : System.Private.CoreLib.dasm - Encoder:GetBytes(long,int,long,int,bool):int:this
           3 ( 0,78% of base) : System.Private.CoreLib.dasm - String:<GetNonRandomizedHashCodeOrdinalIgnoreCase>g__GetNonRandomizedHashCodeOrdinalIgnoreCaseSlow|47_0(String):int
           4 ( 0,71% of base) : System.Private.CoreLib.dasm - Decoder:GetChars(long,int,long,int,bool):int:this
           1 ( 0,27% of base) : System.Private.CoreLib.dasm - Decoder:GetCharCount(long,int,bool):int:this
           1 ( 0,26% of base) : System.Private.CoreLib.dasm - Encoder:GetByteCount(long,int,bool):int:this
           1 ( 0,16% of base) : System.Private.CoreLib.dasm - RuntimeModule:ResolveSignature(int):ref:this

Top method improvements (percentages):
         -16 (-8,60% of base) : System.Private.CoreLib.dasm - String:TrimHelper(long,int,int):String:this
         -19 (-6,09% of base) : System.Private.CoreLib.dasm - Utf8Parser:TryParseByteX(ReadOnlySpan`1,byref,byref):bool
         -24 (-5,80% of base) : System.Private.CoreLib.dasm - Path:Populate83FileNameFromRandomBytes(long,int,Span`1)
         -10 (-5,03% of base) : System.Private.CoreLib.dasm - <GetEnumerator>d__17:MoveNext():bool:this
         -14 (-3,98% of base) : System.Private.CoreLib.dasm - Utf8Parser:TryParseUInt64X(ReadOnlySpan`1,byref,byref):bool
          -8 (-3,79% of base) : System.Private.CoreLib.dasm - TraceLoggingDataCollector:EndBufferedArray(int,int)
         -24 (-3,51% of base) : System.Private.CoreLib.dasm - TimeSpanTokenizer:GetNextToken():TimeSpanToken:this
          -1 (-3,45% of base) : System.Private.CoreLib.dasm - TimeSpanTokenizer:get_NextChar():ushort:this
         -35 (-3,43% of base) : System.Private.CoreLib.dasm - ArraySortHelper`1:SwapIfGreater(Span`1,Comparison`1,int,int) (9 methods)
          -6 (-3,41% of base) : System.Private.CoreLib.dasm - List`1:FindLast(Predicate`1):Vector`1:this
          -1 (-3,23% of base) : System.Private.CoreLib.dasm - ParameterModifier:get_Item(int):bool:this
          -1 (-3,23% of base) : System.Private.CoreLib.dasm - ParameterModifier:set_Item(int,bool):this
          -1 (-3,23% of base) : System.Private.CoreLib.dasm - StackFrameHelper:GetILOffset(int):int:this
          -1 (-3,23% of base) : System.Private.CoreLib.dasm - StackFrameHelper:GetOffset(int):int:this
          -1 (-3,12% of base) : System.Private.CoreLib.dasm - ConcreteFormattableString:GetArgument(int):Object:this
          -1 (-3,12% of base) : System.Private.CoreLib.dasm - SZArrayHelper:get_Item(int):int:this
         -16 (-3,08% of base) : System.Private.CoreLib.dasm - Enumerator:get_Current():byref:this (16 methods)
          -1 (-3,03% of base) : System.Private.CoreLib.dasm - DTSubString:get_Item(int):ushort:this
          -1 (-3,03% of base) : System.Private.CoreLib.dasm - SZArrayHelper:get_Item(int):long:this
          -1 (-3,03% of base) : System.Private.CoreLib.dasm - SZArrayHelper:get_Item(int):Nullable`1:this

1334 total methods with Code Size differences (1326 improved, 8 regressed), 24492 unchanged.

ghost · 2021-08-23T20:07:36Z

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

Issue Details

Array and Span length is never negative and after bounds checking index is also guaranteed to be non-negative. Unsigned casts get rid of quite a few movsxd instructions and replace the rest with cheaper mov variants.

PMI diffs for System.Private.CoreLib show only 8 regressions, of which only a couple are because of CSE misses, most are from loop alignment changes.

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 5517665
Total bytes of diff: 5513593
Total bytes of delta: -4072 (-0,07% of base)
Total relative delta: -10,86
    diff is an improvement.
    relative diff is an improvement.

Detailed diffs for win-x64

Top method regressions (bytes):
          20 ( 1,56% of base) : System.Private.CoreLib.dasm - PathHelper:TryExpandShortFileName(byref,String):String
           9 ( 1,29% of base) : System.Private.CoreLib.dasm - GregorianCalendarHelper:GetYearOffset(int,int,bool):int:this
           5 ( 0,90% of base) : System.Private.CoreLib.dasm - Encoder:GetBytes(long,int,long,int,bool):int:this
           4 ( 0,71% of base) : System.Private.CoreLib.dasm - Decoder:GetChars(long,int,long,int,bool):int:this
           3 ( 0,78% of base) : System.Private.CoreLib.dasm - String:<GetNonRandomizedHashCodeOrdinalIgnoreCase>g__GetNonRandomizedHashCodeOrdinalIgnoreCaseSlow|47_0(String):int
           1 ( 0,27% of base) : System.Private.CoreLib.dasm - Decoder:GetCharCount(long,int,bool):int:this
           1 ( 0,26% of base) : System.Private.CoreLib.dasm - Encoder:GetByteCount(long,int,bool):int:this
           1 ( 0,16% of base) : System.Private.CoreLib.dasm - RuntimeModule:ResolveSignature(int):ref:this

Top method improvements (bytes):
        -112 (-1,08% of base) : System.Private.CoreLib.dasm - List`1:ConvertAll(Converter`2):List`1:this (64 methods)
         -70 (-0,64% of base) : System.Private.CoreLib.dasm - DefaultBinder:BindToMethod(int,ref,byref,ref,CultureInfo,ref,byref):MethodBase:this
         -60 (-0,66% of base) : System.Private.CoreLib.dasm - HashSet`1:SymmetricExceptWithEnumerable(IEnumerable`1):this (8 methods)
         -55 (-1,13% of base) : System.Private.CoreLib.dasm - Dictionary`2:Resize(int,bool):this (10 methods)
         -42 (-0,55% of base) : System.Private.CoreLib.dasm - Enumerator:MoveNext():bool:this (74 methods)
         -37 (-1,09% of base) : System.Private.CoreLib.dasm - HashSet`1:Resize(int,bool):this (8 methods)
         -35 (-3,43% of base) : System.Private.CoreLib.dasm - ArraySortHelper`1:SwapIfGreater(Span`1,Comparison`1,int,int) (9 methods)
         -32 (-1,47% of base) : System.Private.CoreLib.dasm - ArraySortHelper`1:DownHeap(Span`1,int,int,Comparison`1) (8 methods)
         -32 (-0,44% of base) : System.Private.CoreLib.dasm - Dictionary`2:OnDeserialization(Object):this (8 methods)
         -31 (-0,41% of base) : System.Private.CoreLib.dasm - HashSet`1:OnDeserialization(Object):this (8 methods)
         -31 (-1,66% of base) : System.Private.CoreLib.dasm - List`1:RemoveAll(Predicate`1):int:this (8 methods)
         -30 (-0,82% of base) : System.Private.CoreLib.dasm - ArraySortHelper`2:DownHeap(Span`1,Span`1,int,int,IComparer`1) (8 methods)
         -30 (-0,50% of base) : System.Private.CoreLib.dasm - ArraySortHelper`2:PickPivotAndPartition(Span`1,Span`1,IComparer`1):int (8 methods)
         -30 (-0,77% of base) : System.Private.CoreLib.dasm - Vector128`1:ToString():String:this (6 methods)
         -30 (-0,77% of base) : System.Private.CoreLib.dasm - Vector256`1:ToString():String:this (6 methods)
         -29 (-0,35% of base) : System.Private.CoreLib.dasm - TlsOverPerCoreLockedStacksArrayPool`1:Rent(int):ref:this (10 methods)
         -28 (-0,46% of base) : System.Private.CoreLib.dasm - HashSet`1:IntersectWithEnumerable(IEnumerable`1):this (8 methods)
         -28 (-1,42% of base) : System.Private.CoreLib.dasm - PerCoreLockedStacks:TryPop():ref:this (8 methods)
         -28 (-0,76% of base) : System.Private.CoreLib.dasm - Vector64`1:ToString():String:this (6 methods)
         -24 (-0,54% of base) : System.Private.CoreLib.dasm - ConfigurableArrayPool`1:Rent(int):ref:this (8 methods)

Top method regressions (percentages):
          20 ( 1,56% of base) : System.Private.CoreLib.dasm - PathHelper:TryExpandShortFileName(byref,String):String
           9 ( 1,29% of base) : System.Private.CoreLib.dasm - GregorianCalendarHelper:GetYearOffset(int,int,bool):int:this
           5 ( 0,90% of base) : System.Private.CoreLib.dasm - Encoder:GetBytes(long,int,long,int,bool):int:this
           3 ( 0,78% of base) : System.Private.CoreLib.dasm - String:<GetNonRandomizedHashCodeOrdinalIgnoreCase>g__GetNonRandomizedHashCodeOrdinalIgnoreCaseSlow|47_0(String):int
           4 ( 0,71% of base) : System.Private.CoreLib.dasm - Decoder:GetChars(long,int,long,int,bool):int:this
           1 ( 0,27% of base) : System.Private.CoreLib.dasm - Decoder:GetCharCount(long,int,bool):int:this
           1 ( 0,26% of base) : System.Private.CoreLib.dasm - Encoder:GetByteCount(long,int,bool):int:this
           1 ( 0,16% of base) : System.Private.CoreLib.dasm - RuntimeModule:ResolveSignature(int):ref:this

Top method improvements (percentages):
         -16 (-8,60% of base) : System.Private.CoreLib.dasm - String:TrimHelper(long,int,int):String:this
         -19 (-6,09% of base) : System.Private.CoreLib.dasm - Utf8Parser:TryParseByteX(ReadOnlySpan`1,byref,byref):bool
         -24 (-5,80% of base) : System.Private.CoreLib.dasm - Path:Populate83FileNameFromRandomBytes(long,int,Span`1)
         -10 (-5,03% of base) : System.Private.CoreLib.dasm - <GetEnumerator>d__17:MoveNext():bool:this
         -14 (-3,98% of base) : System.Private.CoreLib.dasm - Utf8Parser:TryParseUInt64X(ReadOnlySpan`1,byref,byref):bool
          -8 (-3,79% of base) : System.Private.CoreLib.dasm - TraceLoggingDataCollector:EndBufferedArray(int,int)
         -24 (-3,51% of base) : System.Private.CoreLib.dasm - TimeSpanTokenizer:GetNextToken():TimeSpanToken:this
          -1 (-3,45% of base) : System.Private.CoreLib.dasm - TimeSpanTokenizer:get_NextChar():ushort:this
         -35 (-3,43% of base) : System.Private.CoreLib.dasm - ArraySortHelper`1:SwapIfGreater(Span`1,Comparison`1,int,int) (9 methods)
          -6 (-3,41% of base) : System.Private.CoreLib.dasm - List`1:FindLast(Predicate`1):Vector`1:this
          -1 (-3,23% of base) : System.Private.CoreLib.dasm - ParameterModifier:get_Item(int):bool:this
          -1 (-3,23% of base) : System.Private.CoreLib.dasm - ParameterModifier:set_Item(int,bool):this
          -1 (-3,23% of base) : System.Private.CoreLib.dasm - StackFrameHelper:GetILOffset(int):int:this
          -1 (-3,23% of base) : System.Private.CoreLib.dasm - StackFrameHelper:GetOffset(int):int:this
          -1 (-3,12% of base) : System.Private.CoreLib.dasm - ConcreteFormattableString:GetArgument(int):Object:this
          -1 (-3,12% of base) : System.Private.CoreLib.dasm - SZArrayHelper:get_Item(int):int:this
         -16 (-3,08% of base) : System.Private.CoreLib.dasm - Enumerator:get_Current():byref:this (16 methods)
          -1 (-3,03% of base) : System.Private.CoreLib.dasm - DTSubString:get_Item(int):ushort:this
          -1 (-3,03% of base) : System.Private.CoreLib.dasm - SZArrayHelper:get_Item(int):long:this
          -1 (-3,03% of base) : System.Private.CoreLib.dasm - SZArrayHelper:get_Item(int):Nullable`1:this

1334 total methods with Code Size differences (1326 improved, 8 regressed), 24492 unchanged.

Author:	pentp
Assignees:	-
Labels:	`area-CodeGen-coreclr`, `community-contribution`
Milestone:	-

EgorBo · 2021-09-13T17:49:47Z

I guess it does similar to what #52414 did, @BruceForstall had some objections about it.
Also if we could rely on assertprop and optimize things like this we could get this one for free including other patterns.

pentp · 2021-09-14T07:54:11Z

It's somewhat similar, but we don't have to wait for all of these magical features to get some nice wins right now without any hacks.
There's no downside to this - unlike my previous attempt a couple of years ago (and #52414 also maybe), this one has almost no regressions because it applies zero-extension to both arrays (length & indexer) and spans.

jakobbotsch

This looks great to me, let me kick off outerloop and then I'll merge assuming it passes.
FWIW, it doesn't seem like there were any actual objections in #52414.

jakobbotsch · 2021-09-15T11:39:54Z

/azp run runtime-coreclr outerloop

azure-pipelines · 2021-09-15T11:40:17Z

Azure Pipelines successfully started running 1 pipeline(s).

jakobbotsch · 2021-09-16T12:33:49Z

@pentp did you verify that the original test failures were unrelated? I saw one failing job but I couldn't see the test log, so I tried rerunning it, but looks like the build timed out. Can you rebase/merge this change so we can see if we can get a green CI run?

The outerloop failure looks unrelated, it's #58481.

pentp · 2021-09-16T12:40:58Z

I looked at the two failing test runs and I think they were some test infra failures (like sending to helix failed), don't remember exactly. Rebased the commits.

jakobbotsch · 2021-09-16T16:59:25Z

The failure is #11063.

kunalspathak · 2021-09-21T15:38:54Z

Improvements on Linux/x64 - dotnet/perf-autofiling-issues#1482

kunalspathak · 2021-09-21T15:43:24Z

Improvements on windows/x64 - dotnet/perf-autofiling-issues#1490

kunalspathak · 2021-09-21T15:47:54Z

Improvements on windows amd x64 - dotnet/perf-autofiling-issues#1512

EgorBo · 2021-09-23T15:47:19Z

Improvements on win-arm64: dotnet/perf-autofiling-issues#1538

EgorBo · 2021-09-23T15:50:50Z

Improvements on win-x64: dotnet/perf-autofiling-issues#1488

EgorBo · 2021-09-23T15:53:33Z

Improvements on alpine 3.12: dotnet/perf-autofiling-issues#1503

kunalspathak · 2021-10-21T15:16:43Z

Noticed IniArray improved on win/arm64. @DrewScoggins - any idea why this was not caught by autofiler?

ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 23, 2021

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 23, 2021

JulieLeeMSFT assigned pentp Aug 23, 2021

JulieLeeMSFT added this to the 7.0.0 milestone Aug 23, 2021

JulieLeeMSFT requested a review from jakobbotsch September 13, 2021 16:28

jakobbotsch approved these changes Sep 15, 2021

View reviewed changes

pentp added 3 commits September 16, 2021 15:38

Improve TP for Span_get_Item

43eb602

Use unsigned casts for array length/index

cc768d2

Use unsigned cast for Span_get_Item

e45e445

pentp force-pushed the array-index branch from 6071a2c to e45e445 Compare September 16, 2021 12:38

jakobbotsch merged commit e55a569 into dotnet:main Sep 16, 2021

pentp deleted the array-index branch September 16, 2021 17:39

kunalspathak mentioned this pull request Sep 21, 2021

Regression in QuickSortSpan / Perf_Integer.ToByteArray benchmark #59414

Closed

kunalspathak mentioned this pull request Sep 21, 2021

[Perf] Changes at 9/18/2021 1:42:06 AM dotnet/perf-autofiling-issues#1490

Closed

kunalspathak mentioned this pull request Sep 21, 2021

[Perf] Changes at 9/16/2021 5:40:24 PM dotnet/perf-autofiling-issues#1512

Closed

pentp mentioned this pull request Sep 30, 2021

RyuJIT: Index Variable Widening optimization for array accesses #7312

Closed

ghost locked as resolved and limited conversation to collaborators Nov 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use unsigned casts for array/Span indexers #57970

Use unsigned casts for array/Span indexers #57970

pentp commented Aug 23, 2021

ghost commented Aug 23, 2021

EgorBo commented Sep 13, 2021

pentp commented Sep 14, 2021 •

edited

Loading

jakobbotsch left a comment

jakobbotsch commented Sep 15, 2021

azure-pipelines bot commented Sep 15, 2021

jakobbotsch commented Sep 16, 2021

pentp commented Sep 16, 2021

jakobbotsch commented Sep 16, 2021

kunalspathak commented Sep 21, 2021

kunalspathak commented Sep 21, 2021

kunalspathak commented Sep 21, 2021

EgorBo commented Sep 23, 2021

EgorBo commented Sep 23, 2021

EgorBo commented Sep 23, 2021

kunalspathak commented Oct 21, 2021

Use unsigned casts for array/Span indexers #57970

Use unsigned casts for array/Span indexers #57970

Conversation

pentp commented Aug 23, 2021

ghost commented Aug 23, 2021

EgorBo commented Sep 13, 2021

pentp commented Sep 14, 2021 • edited Loading

jakobbotsch left a comment

Choose a reason for hiding this comment

jakobbotsch commented Sep 15, 2021

azure-pipelines bot commented Sep 15, 2021

jakobbotsch commented Sep 16, 2021

pentp commented Sep 16, 2021

jakobbotsch commented Sep 16, 2021

kunalspathak commented Sep 21, 2021

kunalspathak commented Sep 21, 2021

kunalspathak commented Sep 21, 2021

EgorBo commented Sep 23, 2021

EgorBo commented Sep 23, 2021

EgorBo commented Sep 23, 2021

kunalspathak commented Oct 21, 2021

pentp commented Sep 14, 2021 •

edited

Loading