Treat System.Runtime.CompilerServices.Unsafe as intrinsic #68739
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT Issue DetailsThis is a draft to get initial feedback and confirm that tests pass. Most of these methods are extremely trivial and currently cause the JIT to do more work overall to inline them and optimize away the various boundaries that otherwise exist. Treating them as intrinsic allows them to be "inlined directly" without the normal overhead of the inliner, which should increase code quality and overall JIT throughput.
|
|
Resolved feedback and added handling for
The SuperPMI diffs look promising. Throughput has minor wins across the board:
Code Size diffs looks to be generally an improvement, but there are both small wins and small regressions. Will need to spot check some of them |
|
Just spot checking, several of the regressions are things like "now the register allocator decides to use a different register, which causes some downstream changes" Several of the cases are reduced local count and others are because we end up with other opts triggering that didn't before. The biggest difference that looks to cause most of these is that we have cases where a I'm probably going to need to get a proper jit dump to get a more in depth look here, but will probably get to it later in the evening. |
|
Reverted |
| { | ||
| assert(sig->sigInst.methInstCount == 1); | ||
|
|
||
| // ldarg.0 |
There was a problem hiding this comment.
I really like the comments describing these :)
|
#68851 resolves the AV that was occuring and removes the need for the workaround. This is ready for review in either case. |
Revert these workarounds and wait for the proper fix to get merged? |
I was planning on doing that when the fix was merged to avoid unnecessary CI churn |
|
#68851 was just merged so the workaround should no longer be necessary. |
|
Merged with dotnet/main and reverted the unnecessary spilling. I kept the extracted |
|
Revert 982660e too? Or is there a problem with it? |
This reverts commit 982660e.
|
Shouldn't be, just forgot to include it. |
Is unrelated. It's complaining about an |
|
Updated #68739 (comment) with the throughput numbers. Still all positive there (anywhere from Will log an issue tracking the remaining For the asm-diffs there are wins and regressions with the overall average being an improvement. The wins vs regressions are the same on all platforms (with actual percentages differing by platform) with the regressions being in Spot checking a few, they are generally in cases where we now make different inlining decisions. For example, in - 268 single block inlinees; 165 inlinees without PGO data
+ 137 single block inlinees; 223 inlinees without PGO dataWe end up doing more inlining for things like |
|
@dotnet/jit-contrib for review. |
| * i is the stack entry which will be checked and spilled. | ||
| */ | ||
|
|
||
| inline void Compiler::impSpillSideEffect(bool spillGlobEffects, unsigned i DEBUGARG(const char* reason)) |
There was a problem hiding this comment.
Ok, so this was just a move of code to its own function.
There was a problem hiding this comment.
Yes, that way it could be reused more easily.
|
Thanks! Logged #69220 to track the remaining methods. |
ghost
locked as resolved and limited conversation to collaborators
This is a draft to get initial feedback and confirm that tests pass, remaining APIs (currently just
return nullptr) should also be handled if this looks good. That could happen as part of this PR or as a separate PR/work item.Most of these methods are extremely trivial and currently cause the JIT to do more work overall to inline them and optimize away the various boundaries that otherwise exist. Treating them as intrinsic allows them to be "inlined directly" without the normal overhead of the inliner, which should increase code quality and overall JIT throughput.