-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Add AggressiveInlining
to Char.IsLatin1
#84414
Conversation
* Force the JIT to inline this 17 byte method to elide bounds checks. * Remove useless `uint` casts as `char` is unsigned and `ReadOnlySpan<T>.Length` is never negative.
@@ -81,7 +81,8 @@ namespace System | |||
}; | |||
|
|||
// Return true for all characters below or equal U+00ff, which is ASCII + Latin-1 Supplement. | |||
private static bool IsLatin1(char c) => (uint)c < (uint)Latin1CharInfo.Length; | |||
[MethodImpl(MethodImplOptions.AggressiveInlining)] |
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.
Do you have an example where it's not inlined currently?
casts are indeed not needed (since net8)
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.
https://csharp.godbolt.org/z/fexKr3f4x
However, it looks like IsLatin1
is actually inlined in Char
as it is a struct not a class.
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.
@EgorBo Is it worth keeping the AggressiveInlining
to express intent, or not?
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.
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.
@EgorBo Sorry for not being clear, since Char
is a struct the method is in fact inlined (sharplab.io).
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.
However, not with crossgen2 (godbolt).
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.
@xtqqczze change host type from struct to class
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.
Ah you meant that in this case it's fine - yes. But overall it showcases a real problem inliner is not able to figure out
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.
Filed #84416
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.
Is it worth keeping the AggressiveInlining to express intent, or not?
No, but thanks
uint
casts aschar
is unsigned andReadOnlySpan<T>.Length
is never negative.