Skip to content
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

Jump to bottom

Support loop cloning of byte array accesses #48894

Merged
merged 2 commits into from
Mar 2, 2021
Merged

Support loop cloning of byte array accesses #48894

merged 2 commits into from
Mar 2, 2021

Conversation

BruceForstall
Copy link
Member

Loop cloning does pattern matching on morphed array index expressions, looking
for expressions that include bounds checks that can potentially
be removed. It wasn't matching array index expressions for byte arrays,
which don't have an index scaling expression. This change simply
matches that pattern.

A few spmi asmdiff results (these are all size regressions because loops get cloned when they previously didn't):

Benchmarks:

Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 3683
Total bytes of diff: 4244
Total bytes of delta: 561 (15.23% of base)
    diff is a regression.
Detail diffs 


Top file regressions (bytes):
         119 : 11905.dasm (51.07% of base)
          90 : 13427.dasm (21.33% of base)
          78 : 21965.dasm (36.62% of base)
          74 : 2662.dasm (30.33% of base)
          55 : 7472.dasm (50.46% of base)
          40 : 7823.dasm (5.02% of base)
          29 : 17253.dasm (24.37% of base)
          29 : 19312.dasm (24.37% of base)
          25 : 5734.dasm (21.74% of base)
          18 : 14416.dasm (1.50% of base)
           4 : 19533.dasm (3.54% of base)

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

Top method regressions (bytes):
         119 (51.07% of base) : 11905.dasm - System.Xml.XmlConverter:TryParseInt32(System.Byte[],int,int,byref):bool
          90 (21.33% of base) : 13427.dasm - IDEAEncryption:DoIDEAIteration(System.Byte[],System.Byte[],System.Byte[],int,int,System.Char[],System.Char[]):long
          78 (36.62% of base) : 21965.dasm - Utf8Json.Formatters.Int32Formatter:DeserializeFromPropertyName(byref,Utf8Json.IJsonFormatterResolver):int:this
          74 (30.33% of base) : 2662.dasm - Utf8Json.JsonReader:ReadInt64():long:this
          55 (50.46% of base) : 7472.dasm - System.Xml.XmlBufferReader:Equals2(int,int,System.Byte[]):bool:this
          40 ( 5.02% of base) : 7823.dasm - CriticalHelper:ReadMembers(System.Runtime.Serialization.ClassDataContract,System.Reflection.Emit.LocalBuilder):this
          29 (24.37% of base) : 17253.dasm - Microsoft.CodeAnalysis.Collections.ByteSequenceComparer:Equals(System.Collections.Immutable.ImmutableArray`1[Byte],System.Collections.Immutable.ImmutableArray`1[Byte]):bool
          29 (24.37% of base) : 19312.dasm - System.Reflection.Internal.ByteSequenceComparer:Equals(System.Collections.Immutable.ImmutableArray`1[Byte],System.Collections.Immutable.ImmutableArray`1[Byte]):bool
          25 (21.74% of base) : 5734.dasm - SslCredKey:Equals(SslCredKey):bool:this
          18 ( 1.50% of base) : 14416.dasm - BenchmarksGame.KNucleotide_9:loadThreeData(System.IO.Stream)
           4 ( 3.54% of base) : 19533.dasm - System.Reflection.Metadata.Ecma335.MetadataSizes:ReferenceFits(int,System.Reflection.Metadata.Ecma335.TableIndex[]):bool:this

Top method regressions (percentages):
         119 (51.07% of base) : 11905.dasm - System.Xml.XmlConverter:TryParseInt32(System.Byte[],int,int,byref):bool
          55 (50.46% of base) : 7472.dasm - System.Xml.XmlBufferReader:Equals2(int,int,System.Byte[]):bool:this
          78 (36.62% of base) : 21965.dasm - Utf8Json.Formatters.Int32Formatter:DeserializeFromPropertyName(byref,Utf8Json.IJsonFormatterResolver):int:this
          74 (30.33% of base) : 2662.dasm - Utf8Json.JsonReader:ReadInt64():long:this
          29 (24.37% of base) : 17253.dasm - Microsoft.CodeAnalysis.Collections.ByteSequenceComparer:Equals(System.Collections.Immutable.ImmutableArray`1[Byte],System.Collections.Immutable.ImmutableArray`1[Byte]):bool
          29 (24.37% of base) : 19312.dasm - System.Reflection.Internal.ByteSequenceComparer:Equals(System.Collections.Immutable.ImmutableArray`1[Byte],System.Collections.Immutable.ImmutableArray`1[Byte]):bool
          25 (21.74% of base) : 5734.dasm - SslCredKey:Equals(SslCredKey):bool:this
          90 (21.33% of base) : 13427.dasm - IDEAEncryption:DoIDEAIteration(System.Byte[],System.Byte[],System.Byte[],int,int,System.Char[],System.Char[]):long
          40 ( 5.02% of base) : 7823.dasm - CriticalHelper:ReadMembers(System.Runtime.Serialization.ClassDataContract,System.Reflection.Emit.LocalBuilder):this
           4 ( 3.54% of base) : 19533.dasm - System.Reflection.Metadata.Ecma335.MetadataSizes:ReferenceFits(int,System.Reflection.Metadata.Ecma335.TableIndex[]):bool:this
          18 ( 1.50% of base) : 14416.dasm - BenchmarksGame.KNucleotide_9:loadThreeData(System.IO.Stream)

11 total methods with Code Size differences (0 improved, 11 regressed), 0 unchanged.

Tests:


Summary of Code Size diffs:
(Lower is better)

Total bytes of base: 41658
Total bytes of diff: 44436
Total bytes of delta: 2778 (6.67% of base)
    diff is a regression.
Detail diffs 


Top file regressions (bytes):
         346 : 190982.dasm (26.00% of base)
         206 : 237388.dasm (5.86% of base)
         183 : 238375.dasm (10.33% of base)
         120 : 209604.dasm (43.17% of base)
         120 : 209243.dasm (43.17% of base)
         120 : 221455.dasm (43.17% of base)
         120 : 221531.dasm (43.17% of base)
          90 : 191069.dasm (21.33% of base)
          79 : 231438.dasm (77.45% of base)
          70 : 247653.dasm (66.67% of base)
          63 : 226648.dasm (59.43% of base)
          63 : 226740.dasm (50.00% of base)
          63 : 238301.dasm (59.43% of base)
          63 : 238741.dasm (59.43% of base)
          63 : 238370.dasm (59.43% of base)
          63 : 239229.dasm (59.43% of base)
          63 : 217980.dasm (50.00% of base)
          63 : 238704.dasm (59.43% of base)
          63 : 227153.dasm (59.43% of base)
          63 : 238829.dasm (59.43% of base)

Top file improvements (bytes):
         -70 : 213280.dasm (-1.22% of base)
         -66 : 214200.dasm (-1.64% of base)
         -66 : 214228.dasm (-1.39% of base)
         -42 : 214236.dasm (-1.00% of base)
         -42 : 213251.dasm (-0.92% of base)

53 total files with Code Size differences (5 improved, 48 regressed), 0 unchanged.

Top method regressions (bytes):
         346 (26.00% of base) : 190982.dasm - Huffman:DoHuffIteration(System.Byte[],System.Byte[],System.Byte[],int,int,huff_node[]):long
         206 ( 5.86% of base) : 237388.dasm - Co1245GetBytes_double:runTest():bool:this
         183 (10.33% of base) : 238375.dasm - VectorArrayInitTest`1[Byte][System.Byte]:VectorArrayInit(int,System.Random):int
         120 (43.17% of base) : 209604.dasm - TestLibrary.Assert:AreAllEqualUnordered(System.Byte[],System.Byte[])
         120 (43.17% of base) : 209243.dasm - TestLibrary.Assert:AreAllEqualUnordered(System.Byte[],System.Byte[])
         120 (43.17% of base) : 221455.dasm - TestLibrary.Assert:AreAllEqualUnordered(System.Byte[],System.Byte[])
         120 (43.17% of base) : 221531.dasm - TestLibrary.Assert:AreAllEqualUnordered(System.Byte[],System.Byte[])
          90 (21.33% of base) : 191069.dasm - IDEAEncryption:DoIDEAIteration(System.Byte[],System.Byte[],System.Byte[],int,int,System.Char[],System.Char[]):long
          79 (77.45% of base) : 231438.dasm - MarshalBoolArray:TestMethod_CallBackIn(int,System.Boolean[]):bool
          70 (66.67% of base) : 247653.dasm - VectorMathTests.Program:GenerateByteArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 226648.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (50.00% of base) : 226740.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238301.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238741.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238370.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 239229.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (50.00% of base) : 217980.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238704.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 227153.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238829.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]

Top method improvements (bytes):
         -70 (-1.22% of base) : 213280.dasm - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -66 (-1.64% of base) : 214200.dasm - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -66 (-1.39% of base) : 214228.dasm - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -42 (-1.00% of base) : 214236.dasm - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -42 (-0.92% of base) : 213251.dasm - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int

Top method regressions (percentages):
          79 (77.45% of base) : 231438.dasm - MarshalBoolArray:TestMethod_CallBackIn(int,System.Boolean[]):bool
          70 (66.67% of base) : 247653.dasm - VectorMathTests.Program:GenerateByteArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 226648.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238301.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238741.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238370.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 239229.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238704.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 227153.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238829.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238912.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238047.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238617.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 238444.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 239100.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 227047.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (59.43% of base) : 239056.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (50.00% of base) : 226740.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
          63 (50.00% of base) : 217980.dasm - VectorTest:GetRandomArray(int,System.Random):System.Byte[]
         120 (43.17% of base) : 209604.dasm - TestLibrary.Assert:AreAllEqualUnordered(System.Byte[],System.Byte[])

Top method improvements (percentages):
         -66 (-1.64% of base) : 214200.dasm - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -66 (-1.39% of base) : 214228.dasm - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -70 (-1.22% of base) : 213280.dasm - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -42 (-1.00% of base) : 214236.dasm - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int
         -42 (-0.92% of base) : 213251.dasm - IntelHardwareIntrinsicTest.Program:Main(System.String[]):int

53 total methods with Code Size differences (5 improved, 48 regressed), 0 unchanged.

@BruceForstall
Support loop cloning of byte array accesses
787f58d 
Cloning pattern matches morphed array index expressions, looking
for expressions that include bounds checks that can potentially
be removed. It wasn't matching array index expressions for byte arrays,
which don't have an index scaling expression. This change simply
matches that pattern.
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 1, 2021
@BruceForstall
Copy link
Member Author

@AndyAyersMS PTAL
cc @dotnet/jit-contrib

AndyAyersMS
AndyAyersMS reviewed Mar 1, 2021
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

What happens for arrays of structs where there's likely a MUL or more complex tree in there?

@BruceForstall
Copy link
Member Author

What happens for arrays of structs where there's likely a MUL or more complex tree in there?

Arrays of structs are not handled (actually, there is some code to specifically disallow handling them). I opened #48897 to track this.

@BruceForstall
Copy link
Member Author

/azp run runtime-coreclr outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Base automatically changed from master to main March 1, 2021 09:08
kunalspathak
kunalspathak reviewed Mar 1, 2021
View reviewed changes
src/coreclr/jit/optimizer.cpp
{
return false;
// No scale (e.g., byte array).
index = si;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to add some assert to make sure the array is what we expect?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I missed this before merge. There's certainly some question about what additional things could/should be checked in this pattern matching. Types, for example. For this case, the index is checked (lower down) to be a lclvar that is the same as the array bounds check base, so we're good.

AndyAyersMS
AndyAyersMS approved these changes Mar 1, 2021
View reviewed changes
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

LGTM.

@BruceForstall BruceForstall merged commit 0e8efce into dotnet:main Mar 2, 2021
@BruceForstall BruceForstall deleted the CloneByteArray branch March 2, 2021 01:02
@benaadams
Copy link
Member

Any reason the filenames in the diffs are being output as numbers?

Top file regressions (bytes):
         119 : 11905.dasm (51.07% of base)
          90 : 13427.dasm (21.33% of base)
          78 : 21965.dasm (36.62% of base)

@BruceForstall
Copy link
Member Author

@benaadams SuperPMI asm diffs (as opposed to "jit-diff" asm diffs) generates a file per function, with the number being the "method context number" (aka the function number in the MCH file) as the filename. The "per-file" stats hence aren't really interesting. We could think about how to make the jit-analyze output more useful when doing SPMI diffs (for starters, just don't show the per-file section).

@JulieLeeMSFT JulieLeeMSFT mentioned this pull request Mar 26, 2021
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Apr 2, 2021
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Apr 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kunalspathak kunalspathak kunalspathak left review comments

@AndyAyersMS AndyAyersMS AndyAyersMS approved these changes

Assignees

@BruceForstall BruceForstall

Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
6.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@BruceForstall @benaadams @AndyAyersMS @kunalspathak @JulieLeeMSFT