-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Fix Unnecessary sign-extension for (nuint)span.Length
#88136
Merged
Merged
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
f403ba2
unconditionally set isNeverNegative in morph, use that data when opti…
En3Tho d3e96b8
cr, change condition and apply optimization to winX64 only
En3Tho 0bfe3cd
apply suggestions and add lclfld check too
En3Tho 914da44
Run jit-format
En3Tho e73dd3e
ADDED CORINFO_FLG_SPAN
En3Tho d6652bc
Address CR
En3Tho 2dc4601
Pull main and resolve conflicts
En3Tho bfb1395
Redo CR fix after pull main.
En3Tho 5edb8c9
Merge branch 'main' of https://github.com/dotnet/runtime into issue-8…
En3Tho cdb554f
merge pull main & removed corinfo flag. A quick check if these change…
En3Tho 0a62f7a
fix build for ee
En3Tho 52021c3
missed this one
En3Tho d665ba5
marked span & readonlyspan as intrinsics, handled checking
En3Tho 7a6d356
CR: removed IsSpan from GenTreeLclVar and used proposed change in Pro…
En3Tho 1f0431a
CR: forgot to remove redundant SetIsSpan(true) call. Fixed
En3Tho eadf5fe
restored some lclmorph lines to keep diff simpler
En3Tho 5b0b0df
It seems like it affects something after all?
En3Tho 6ce32b2
Optimize spans only when Optimizations are enabled
En3Tho 8e82297
revert OptimizationEnabled check
En3Tho File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default pattern for special casing types in the JIT is to mark the type with
[Intrinsic]
, callisIntrinsicType
on the class handle, and check the name ifisIntrinsicType
returns true. It is how we are special casing the SIMD Vector types. My guess that the default pattern would cheaper overall than computing all type flags just to figure out whether the type is a Span or ReadOnlySpan.If this check is very hot and we cannot afford to use the default pattern for it, it should be a dedicated
isSpanOrReadOnlySpanType
method on JIT/EE interface so that we do not pay the full cost of getting all flags thatgetClassAttributes
returns.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggested to @En3Tho to add
CORINFO_FLG_SPAN
mainly based on the fact that it is on the same level of information asCORINFO_FLG_ARRAY
andCORINFO_FLG_DELEGATE
, but it is reasonable to me to switch this toisSpanOrReadOnlySpanType
. I am not so sure that we want to switchlvaSetStruct
to be doing the same style of recognition as the SIMD recognition we do today.Note that we have been calling
getClassAttribs
unconditionally fromlvaSetStruct
since forever. It was only recently removed in #87967.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Arrays or Delegates are core type system entities that one cannot identify via a simple name check. Span/ReadOnlySpan is just an ordinary type that the JIT is recognizing as intrinsic.
Are you worried about throughput? The name check behind the
isIntrinsicType
early out is fairly cheap overall. We have intentionally switched to allow JIT to identify intrinsics by name, so that the JIT/EE interface does not need to know about all ordinary types and methods that the JIT wants to special case as an intrinsic.If we are worried about throughout, a dedicated
isSpanOrReadOnlySpanType
method would be the way to go.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point.
Yes, I was mainly worried about throughput -- we have been quite careful in the JIT to only do SIMD recognition from the paths where we know it is relevant, instead of, say, a general function like
lvaSetStruct
. But SIMD recognition also has much more to recognize, so if you say that theisIntrinsic
+ metadata lookup is cheap, then I think we should just do it this way and avoid the JIT-EE change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can keep track of it only when optimizing.