Skip to content

JIT: Remove BB limit from importer_vectorization - #66534

Merged
EgorBo merged 1 commit into
dotnet:mainfrom
EgorBo:remove-bb-limit-vectorization
Mar 14, 2022
Merged

JIT: Remove BB limit from importer_vectorization#66534
EgorBo merged 1 commit into
dotnet:mainfrom
EgorBo:remove-bb-limit-vectorization

Conversation

@EgorBo

@EgorBoEgorBo commented Mar 12, 2022

Copy link
Copy Markdown
Member

#66529 where due to large method size we end up with the following codegen for all loweredValue.SequenceEqual("<literal>".AsSpan()):

movrdx,1FCA7409060	movrdx,[rdx]learcx,[rbp+4C0]call System.MemoryExtensions.AsSpan(System.String)	mov[rbp+10],rbxmov[rbp+18],r14dmovrcx,[rbp+4C0]mov[rbp+20],rcxmovecx,[rbp+4C8]mov[rbp+28],ecxlearcx,[rbp+10]leardx,[rbp+20]call System.MemoryExtensions.SequenceEqual[[System.Char, System.Private.CoreLib]](System.Span`1<Char>, System.ReadOnlySpan`1<Char>)

so let's just unconditionally enable #65288 - surprisingly, it makes code even smaller.

#66529 Benchmark:

MethodToolchainMeanErrorStdDevRatio
SpanBlack\Core_Root_base\corerun.exe49.13 ns0.048 ns0.037 ns3.05
SpanBlack\Core_Root_PR\corerun.exe16.11 ns0.317 ns0.297 ns1.00
SpanLightGoldenrodYellowk\Core_Root_base\corerun.exe65.72 ns0.150 ns0.126 ns2.08
SpanLightGoldenrodYellowk\Core_Root_PR\corerun.exe31.55 ns0.229 ns0.215 ns1.00

2-3x improvement

Codegen diff for GetNamedColorSpan: https://www.diffchecker.com/bFuRMHNP

SPMI Diffs are interesting - https://dev.azure.com/dnceng/public/_build/results?buildId=1660127&view=ms.vss-build-web.run-extensions-tab (I assume they caught all switches over string literals)

benchmarks.run.Linux.x64.checked.mch:
Total bytes of delta: -10162 (-0.06 % of base)
coreclr_tests.pmi.Linux.x64.checked.mch:
Total bytes of delta: -5643 (-0.00 % of base)
libraries.crossgen2.Linux.x64.checked.mch:
Total bytes of delta: 3730 (0.03 % of base)
libraries.pmi.Linux.x64.checked.mch:
Total bytes of delta: -30098 (-0.06 % of base)
libraries_tests.pmi.Linux.x64.checked.mch:
Total bytes of delta: -36095 (-0.03 % of base)

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 12, 2022
@ghostghost assigned EgorBoMar 12, 2022
@ghost

Copy link
Copy Markdown

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

Issue Details

#66529 where due to large method size we end up with the following codegen for all loweredValue.SequenceEqual("<literal>".AsSpan()):

movrdx,1FCA7409060	movrdx,[rdx]learcx,[rbp+4C0]call System.MemoryExtensions.AsSpan(System.String)	mov[rbp+10],rbxmov[rbp+18],r14dmovrcx,[rbp+4C0]mov[rbp+20],rcxmovecx,[rbp+4C8]mov[rbp+28],ecxlearcx,[rbp+10]leardx,[rbp+20]call System.MemoryExtensions.SequenceEqual[[System.Char, System.Private.CoreLib]](System.Span`1<Char>, System.ReadOnlySpan`1<Char>)

so let's just unconditionally enable #65288 - surprisingly, it makes code even smaller.

#66529 Benchmark:

MethodToolchainMeanErrorStdDevRatio
SpanBlack\Core_Root_base\corerun.exe49.13 ns0.048 ns0.037 ns3.05
SpanBlack\Core_Root_PR\corerun.exe16.11 ns0.317 ns0.297 ns1.00
SpanLightGoldenrodYellowk\Core_Root_base\corerun.exe65.72 ns0.150 ns0.126 ns2.08
SpanLightGoldenrodYellowk\Core_Root_PR\corerun.exe31.55 ns0.229 ns0.215 ns1.00

Codegen diff for GetNamedColorSpan: https://www.diffchecker.com/bFuRMHNP

Author:EgorBo
Assignees:EgorBo
Labels:

area-CodeGen-coreclr

Milestone:-

@EgorBo

EgorBo commented Mar 12, 2022

Copy link
Copy Markdown
MemberAuthor

@dotnet/jit-contrib @AndyAyersMS PTAL

@stephentoub

Copy link
Copy Markdown
Member

surprisingly, it makes code even smaller.

Thanks, Egor. I was surprised in the repro when Eric showed it to me last night that even AsSpan wasn't being inlined. Separate from this change, have we done an experiment around what things would look like if we always, unconditionally inlined AsSpan and span's ctors? I wonder if there's a set of critical, core, tiny APIs where we end up falling off a cliff due to inlining budget and we'd be better off with an internal MethodImplOptions.ForceInliningNoReallyThisNeedsToBeInlined setting.

@EgorBo

Copy link
Copy Markdown
MemberAuthor

surprisingly, it makes code even smaller.

Thanks, Egor. I was surprised in the repro when Eric showed it to me last night that even AsSpan wasn't being inlined. Separate from this change, have we done an experiment around what things would look like if we always, unconditionally inlined AsSpan and span's ctors? I wonder if there's a set of critical, core, tiny APIs where we end up falling off a cliff due to inlining budget and we'd be better off with an internal MethodImplOptions.ForceInliningNoReallyThisNeedsToBeInlined setting.

We plan to address it eventually with partial pre-scan to be able to go over-budget, I think there must be already an issue filed for it.

@EgorBo

EgorBo commented Mar 12, 2022

Copy link
Copy Markdown
MemberAuthor

Failures are not related, I suspect it's DST difference + #66540

@kasperk81

Copy link
Copy Markdown
Contributor

652 total methods with Code Size differences (428 improved, 224 regressed), 1 unchanged.

what caused the regressions?

@EgorBo

EgorBo commented Mar 13, 2022

Copy link
Copy Markdown
MemberAuthor

652 total methods with Code Size differences (428 improved, 224 regressed), 1 unchanged.

what caused the regressions?

It seems like it's always a size improvement to unroll SequenceEquals for spans, but it's often a size regression for string objects.
However, I checked few size regressions and all of them were "performance" improvements so I leave it as is.

@jkotas

Copy link
Copy Markdown
Member

I suspect it's DST difference

Opened #66555

@JulieLeeMSFTJulieLeeMSFT added this to the 7.0.0 milestone Mar 14, 2022
@EgorBo

Copy link
Copy Markdown
MemberAuthor

ping @dotnet/jit-contrib simple change

@EgorBo
EgorBo merged commit 3fd6148 into dotnet:mainMar 14, 2022
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
@ghostghost locked as resolved and limited conversation to collaborators Apr 14, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@EgorBo@stephentoub@kasperk81@jkotas@AndyAyersMS@JulieLeeMSFT