-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[RyuJIT] Unnecessary register moves from Unsafe.AsRef<T> #8730
Comments
Looks similar to #8011 (see dotnet/corefxlab#1479 for some detailed notes). If so, it is fallout from how pinning is described in the IL. |
@AndyAyersMS I think it is different. Here is the JIT dump.
Then these three variables and assignments are retained all through codegen:
|
There is still an initial long->byref assignment which may block subsequent simplification. Unsafe.AsRef shouldn't need the ldarg / stloc / ldloc workaround anymore. The jit issue should have been fixed by dotnet/coreclr#11218 (let me know if not). Removing this workaround should remove the latter two movs. |
It still needs it when running on .NET Framework. The build is shared between all runtimes right now. To remove the workaround, we should add netcoreapp-specific build and remove the workaround in that build only. |
@jkotas How would that be done practically for the IL source of |
in project file; with source inludes/excludes? |
@benaadams that would imply almost duplicating IL file as far as I understand, with maintenance issues. Just curious because I wasn't sure how this could be done in a good way with |
ilasm has defines and ifdefs. It should be possible to build the fix on top of the existing include files: https://github.com/dotnet/corefx/tree/master/src/System.Runtime.CompilerServices.Unsafe/src/include . |
I did not know you could do this, good to know, thanks
…On Aug 19, 2017 17:18, "Jan Kotas" ***@***.***> wrote:
ilasm has defines and ifdefs. It should be possible to build the fix on
top of the existing include files: https://github.com/dotnet/
corefx/tree/master/src/System.Runtime.CompilerServices.Unsafe/src/include
.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<https://github.com/dotnet/coreclr/issues/13341#issuecomment-323529222>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKTG77uX-jxGZzGMRSziJWEtc_BcZY-wks5sZvytgaJpZM4O1AgP>
.
|
FYI I have started looking at a PR for this, as it seems like a small change. My plan is something like:
Is that it? Let me know if I am completely off here and if anything is missing? Not sure how we could test this if needed? cc: @karelz |
According to VTune characterization, ParseHeaders is one of the hot functions in TechEmpower-Plaintext scenario.
ParseHeaders
invokes inlinedDangerousGetPinnableReference
twice, which generates roundtripmov
sequences from inlinedUnsafe.AsRef<T>
:These redundant moves are from
Unsafe.AsRef<T>
source code:@jkotas 's example shows that this roundtrip has been correctly eliminated by the compiler, but it appears in the generated code of
Span.DangerousGetPinnableReference
.I will look into providing a solution.
The text was updated successfully, but these errors were encountered: