-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Mark the vectorized CRC update methods noinline #118544
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
Conversation
This works around a deficiency in the jit inliner. When it tries to inline a large method with many important callees into a small method, it runs out of budget trying to inline the callees. The budget is set based on the size of the root method, so the vectorized update methods have enough budget to inline their callees. Fixes dotnet#114383.
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.
Pull Request Overview
This PR addresses a JIT inliner deficiency by marking vectorized CRC update methods as NoInlining
. The issue occurs when the JIT inliner attempts to inline large methods with many important callees into smaller methods, causing it to run out of inlining budget. By preventing inlining of the vectorized methods themselves, the budget allocation is preserved for their internal callees.
- Adds
[MethodImpl(MethodImplOptions.NoInlining)]
attribute to prevent inlining of the vectorized update methods - Includes explanatory comments documenting the JIT inliner workaround rationale
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/libraries/System.IO.Hashing/src/System/IO/Hashing/Crc64.Vectorized.cs |
Adds NoInlining attribute and comment to UpdateVectorized method |
src/libraries/System.IO.Hashing/src/System/IO/Hashing/Crc32.Vectorized.cs |
Adds NoInlining attribute and comment to UpdateVectorized method |
@EgorBo PTAL Locally on an avx-512 box System.IO.Hashing.Tests.Crc64_AppendPerf.Append(BufferSize: 10240)
|
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
I wonder if there's some kind of tool, analyzer, or log function we could use to help identify and automatically make the right decision in cases like this.
Ideally we could figure out how much code is actually being generated for a given call and make better decisions, but that's of course expensive and non-trivial.
@EgorBot --arm --intel |
This works around a deficiency in the jit inliner. When it tries to inline a large method with many important callees into a small method, it runs out of budget trying to inline the callees.
The budget is set based on the size of the root method, so the vectorized update methods have enough budget to inline their callees.
Fixes #114383.