Repository navigation
JIT: Cold code inlining decisions are incorrectly used later #115918
Description
Description
It seems that inlining decisions made in cold code blocks are being remembered by JIT and blindly used later in completely unrelated methods.
Reproduction Steps
https://godbolt.org/z/9zas8rc6h
Expected behavior
C:InnocentMethod implicitly converts the provided Span into a ReadOnlySpan without using a op_Implicit call.
Actual behavior
Trivially inline-able methods (even intrinsic methods) are not inlined in normal methods.
Regression?
.NET 7 seems to not have this issue.
Known Workarounds
No reliable workarounds, can reduce chances by avoiding any potential inlining in cold code blocks or re-ordering method JIT compilation order.
Activity
dotnet-policy-service commentedon May 23, 2025
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.
pentp commentedon May 23, 2025
There's a second related issue that often masks this bug - using throw helpers to throw exceptions causes the throwing code blocks to have non-zero weights (0.5 seems to be common) which seems to allow inlining to work more normally and thus not poison it for later method compilations.
However this seems like an arbitrary and useless distinction between throw helpers and direct throws, which affects not only inlining, but also PerfScore calculation for example and IMHO cold blocks that throw an exception using a throw helper should also get a zero weight / zero PerfScore.
Should I open a separate issue for that?
AndyAyersMS commentedon May 24, 2025
Are you seeing this same codegen outside of godbolt/crossgen2?
@EgorBo FYI
pentp commentedon May 24, 2025
It's not crossgen2 actually, regular JIT, I forgot to update the window titles. I first noticed it when locally generating JIT dumps, with tiered compilation disabled (maybe that has some role in this also).
AndyAyersMS commentedon Jul 30, 2025
@EgorBo have you had a chance to look at this yet?
EgorBo commentedon Jul 30, 2025
Nope, feel free to grab this one if you want. I presume it's that reportInlineDecision thing
BoyBaykiller commentedon Jul 30, 2025
I believe this got fixed by #117692.
@pentp
EgorBo commentedon Aug 1, 2025
@AndyAyersMS shouldn't this be fixed by #114821 ?
EgorBo commentedon Aug 1, 2025
@pentp I've opened your godbolt link - I see no op_Explicit call - does that mean the issue is already fixed?
AndyAyersMS commentedon Aug 1, 2025
That should only apply if we're running with dynamic PGO. Not sure what godbolt does --I thought it used R2R.
pentp commentedon Aug 1, 2025
No, the issue is with this
call [System.Span`1[byte]:op_Implicit(System.Span`1[byte]):System.ReadOnlySpan`1[byte]]and it's still there.I think #114821 addressed the same issue but with limited scope (only with TC and PGO).
pentp commentedon Aug 1, 2025
There's a separate compiler group for Crossgen2 (which is R2R?), so I would assume it uses TC=0 for the "regular" output?
I first noticed the issue locally when using TC=0 to get some asm diffs.
pentp commentedon Aug 1, 2025
Would there be a significant TP impact if the NOINLINE marking was always enabled only for
InlineImpact::FATALmethods?EgorBo commentedon Aug 1, 2025
Ah I see the problem, I don't think it's R2R related, it's #78386 that introduced extremely limited conditions for callees inside BBJ_THROW blocks - we just shouldn't propagate NOINLINE in that case
Yeah it's likely too radical
EgorBo commentedon Aug 1, 2025
Filed #118280
EgorBo commentedon Aug 1, 2025
Since changes in the inliner tend to be problematic for some 1P, I guess it's better to land this in .NET 11, @AndyAyersMS thoughts?
The fix has an impact on NativeAOT/R2R, TC=0 and Dynamic context (which is basically TC=0), no impact on DynamicPGO eligible methods.
AndyAyersMS commentedon Aug 7, 2025
I guess I'm confused -- you just merged #118280 -- are you referring to some bigger fix?
EgorBo commentedon Aug 7, 2025
@AndyAyersMS No, that was my initial conclusion, but then based on your feedback here I merged the PR in 10.0.