Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add explicit signatures for Span indexer - #21048

Merged
jkotas merged 2 commits into
dotnet:masterfrom
jkotas:span-signatures
Nov 16, 2018
Merged

jkotas merged 2 commits into
dotnet:masterfrom
jkotas:span-signatures

Conversation

@jkotas

@jkotas jkotas commented Nov 16, 2018

Copy link
Copy Markdown
Member

Fixes #21032 and #20958

@jkotas

jkotas commented Nov 16, 2018

Copy link
Copy Markdown
Member Author

Now that the Span indexers are recognized properly again, we are hitting assert during crossgen that sneaked in in the meantime:

Assert failure(PID 2976 [0x00000ba0], Thread: 3816 [0x0ee8]): Assertion failed '((regMask & emitThisGCrefRegs) && (ins == INS_add)) || ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))' in 'System.Globalization.CharUnicodeInfo:InternalGetNumericValue(int):double' (IL size 175)

@stephentoub

Copy link
Copy Markdown
Member

Thanks, @jkotas. Do we have any sense for what kind of throughput impact https://github.com/dotnet/coreclr/issues/21032 has on various workloads? I'm wondering if it's worth trying to get this change in for Preview 1, since I believe (but could be wrong) that coreclr was already snapshotted for the release.

@AndyAyersMS AndyAyersMS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Wish I had seen that CC on the original PR.

LGTM. Thanks.

@AndyAyersMS

Copy link
Copy Markdown
Member

On very simple loops indexing via span it was around 20%...

image

@CarolEidt CarolEidt left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for handling this. I was just starting to dig into this, and it would have taken me quite some time to come up to speed on the signature building.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants