-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Assist JIT in eliminating bounds checks #81036
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
@@ -61,7 +61,7 @@ private static bool TryParseNumber(ReadOnlySpan<byte> source, ref Number.NumberB | |||
int maxDigitCount = digits.Length - 1; | |||
|
|||
// Throw away any leading zeroes | |||
while (srcIndex != source.Length) | |||
while (srcIndex < source.Length) |
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 these need uint
-casts to eliminate the bound checks?
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.
In this case casts appear to regress bounds check elimination.
Perhaps this is unnecessary because srcIndex
is a local variable that is zero-initialized?
runtime/src/libraries/System.Private.CoreLib/src/System/Buffers/Text/Utf8Parser/Utf8Parser.Number.cs
Line 34 in ce18ccf
int srcIndex = 0; |
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.
In this case casts appear to regress bounds check elimination.
Can you elaborate? I wouldn't expect this to remove the bounds check on source[srcIndex]
without casting srcIndex to uint here.
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 already do checks on source.Length
before the loop. Adding uint casts seems to cause JIT to "forget" about these.
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 still don't understand what you mean. Can you share a sharplab.io example?
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.
It's worth noting that SharpLab is .NET 7
@tannergooding For future reference, does the codegen from Compiler Explorer have the same optimizations as a local disassembly using .NET 8 nightly build.
Currently, .e.g. https://csharp.godbolt.org/z/cKhTfjvqd, seems to be using:
// crossgen2 8.0.0-preview.3.23152.99+8d2ddc45e8f6f2a726c2b91c1ba2b9f7208b1f3b
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 for godbolt you need to pass -O
or better --Ot
argument to compiler to enable optimizations. godbolt uses crossgen (R2R) which is, on average, worse than what JIT emits (sharplab). Although, even sharplab's codegen is a bit synthetic since in the real world our JIT is tiered and Tier1 codegen might be different from what you see in sharplab.
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.
for godbolt you need to pass -O or better --Ot
@EgorBo Not seeing any differeences in codegen for this example: https://csharp.godbolt.org/z/cKhTfjvqd, is this expected?
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.
Yes. However, its showing the crossgen code and so isn't strictly the same as what sharplab shows or what a user will see for Tier 1 codegen.
You can customize the instruction set targeted and I'd recommend using --instruction-set x86-x64-v3
as that targets AVX2, BMI1, BMI2, LZCNT, MOVBE, and FMA
You can customize the OS: --targetos windows
(defaults to linux
)
and the architecture: --targetarch arm64
(defaults to x64
)
and in general pass in other options supported by crossgen (such as --Ot
to optimize for speed)
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.
@tannergooding Great help with your tips, I knew about the parameters but couldn't find a useful reference with a search engine.
.../System.Linq.Parallel/src/System/Linq/Parallel/Merging/AsynchronousChannelMergeEnumerator.cs
Show resolved
Hide resolved
...s/System.Linq.Parallel/src/System/Linq/Parallel/Merging/SynchronousChannelMergeEnumerator.cs
Show resolved
Hide resolved
src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs
Show resolved
Hide resolved
Tagging subscribers to this area: @dotnet/area-meta Issue DetailsFollow-up: #81001. Change condition in loops from
cc: @stephentoub
|
Should we create an issue to add support to JIT to eliminate range checks for these? |
Issue opened: #84697. |
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.
LGTM. Most of these won't actually help eliminate bounds checks like the title suggests, but they're fine for clarity.
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.
LGTM. Most of these won't actually help eliminate bounds checks like the title suggests, but they're fine for clarity.
Follow-up: #81001.
Change condition in loops from
index != T.Length
toindex < T.Length
where T:U[]
string
Span<U>
ReadOnlySpan<U>
cc: @stephentoub