JIT: Always track the context for late devirt #112396
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
dotnet-policy-service
bot
added
the
community-contribution
hez2010
changed the title
hez2010
changed the title
hez2010
changed the title
|
/azp run runtime-coreclr superpmi-asmdiffs-checked-release |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
it seems that the inline context is still incorrect. |
|
Okay, seems that the failure cases are late devirted without |
|
It should work now. |
|
spmi failures seem to be an unrelated infra issue |
|
/azp run runtime-coreclr superpmi-asmdiffs-checked-release |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@hez2010 I'm seeing the assert |
|
Diff @amanasifkhalid All tests should pass now, can you trigger another superpmi-asmdiffs-checked-release run? |
|
Just to check if there's any diff under FullOpts. |
There was a problem hiding this comment.
I still think we might be better off making the inline context a explicit parameter to impMarkInlineCandidate and friends. There was an implicit assumption before that an inline candidate was always done in the current compiler's inline context, and this isn't true anymore.
We will need something similar if we're ever going to do a non-depth first inlining order.
|
/azp run runtime-coreclr superpmi-asmdiffs-checked-release |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
CI is passing, I'm pushing the refactor commit. |
hez2010
changed the title
|
CI is passing, and diff is identical with before. PTAL. |
|
Is there anything else blocking this from being merged? |
| CORINFO_GENERIC_HANDLE compileTimeHelperArgumentHandle; // Used to track type handle argument of dynamic helpers | ||
| void* gtDirectCallAddress; // Used to pass direct call address between lower and codegen | ||
| }; | ||
|
|
||
| LateDevirtualizationInfo* gtLateDevirtualizationInfo; // Always available for user virtual calls |
There was a problem hiding this comment.
Have you checked whether this can be merged with some other information of GenTreeCall to avoid growing the size of all large nodes?
There was a problem hiding this comment.
Late devirtualization can happen even without generic context, so we need to always track the InlineContext now regardless of whether it has a generic context or not.
We used to union the gtLateDevirtualizationInfo with gtHandleHistogramProfileCandidateInfo together because we didn't save generic context for methods that have class probe, but now we will have to save an InlineContext for them anyway so we can also save the generic context for those methods as well.
This happens to enable late devirtualization for tier-1 methods under PGO and this is why it has diffs for PGO cases only.
I was trying to merge it into the union but it would require class probing to "overwite" the gtLateDevirtualizationInfo and unset the flag for it else where, which may be hard to get right... see https://github.com/dotnet/runtime/runs/37064711496, we have cases where late devit inlining happens while we have class probing.
There was a problem hiding this comment.
Instead we can remove the member class InlineContext* gtInlineContext from GenTreeCall now, though it's debug only.
There was a problem hiding this comment.
Or alternatively, we will have to block all inlining for devirted methods that don't have a generic context in late devirt.
There was a problem hiding this comment.
For debug it's ok to use extra memory, we are more concerned with release. I suppose we cannot move this into the union above this one because of gtStubCallStubAddr?
There was a problem hiding this comment.
Yeah, unfortunately. Maybe we can merge it with gtControlExpr, do we ever morph a call before inlining?
There was a problem hiding this comment.
I think it's ok to leave it as a separate field for now. Maybe we should consider some better approaches in the future to reducing the size of GenTreeCall (e.g. storing all rarely used fields in a separate object that gets allocated on demand).
There was a problem hiding this comment.
Okay. I just pushed a commit to remove the redundant flag.
|
Can you update the PR title and description to indicate what new cases we now are devirtualization+inlining for compared to before? It's not entirely clear to me why this PR has diffs when it was meant to just fix checked-release diffs. |
Done. |
hez2010
changed the title
|
PTAL. |
|
cc @jakobbotsch |
There was a problem hiding this comment.
LGTM. @AndyAyersMS do you want to take a final look as well?
|
Thanks again @hez2010! |
* main: (27 commits) Fold null checks against known non-null values (dotnet#109164) JIT: Always track the context for late devirt (dotnet#112396) JIT: array allocation fixes (dotnet#112676) [H/3] Fix test closing connection too fast (dotnet#112691) Fix LINQ handling of iterator.Take(...).Last(...) (dotnet#112680) [browser][MT] move wasm MT CI legs to extra-platforms (dotnet#112690) JIT: Don't use `Compiler::compFloatingPointUsed` to check if FP kills are needed (dotnet#112668) [LoongArch64] Fix a typo within PR#112166. (dotnet#112672) Fix new EH hang on DebugBreak (dotnet#112640) Use encode callback instead of renting a buffer to write to in DSAKeyFormatHelper Move some links to other doc (dotnet#112574) Reflection-based XmlSerializer - Deserialize empty collections and allow for sub-types in collection items. (dotnet#111723) JIT: Use `fgCalledCount` for OSR method entry weight (dotnet#112662) Use Avx10.2 Instructions in Floating Point Conversions (dotnet#111775) Expose StressLog via CDAC and port StressLogAnalyzer to managed code (dotnet#104999) JIT: Use linear block order for MinOpts in LSRA (dotnet#108147) Update dependencies from https://github.com/dotnet/arcade build 20250213.2 (dotnet#112625) JIT: Clean up and optimize call arg lowering (dotnet#112639) Update dependencies from https://github.com/dotnet/emsdk build 20250217.1 (dotnet#112645) JIT: Support `FIELD_LIST` for returns (dotnet#112308) ...
Resolves #110827 (comment)
Late devirtualization can happen even without generic context, so we need to always track the
InlineContextnow regardless of whether it has a generic context or not.We used to union the
gtLateDevirtualizationInfowithgtHandleHistogramProfileCandidateInfotogether because we didn't save generic context for methods that have class probe, but now we will have to save anInlineContextfor them anyway so we also save the generic context for those methods as well, and move thegtLateDevirtualizationInfoout of the union.I was trying to merge it into the union but it immediately failed the CI because we have cases where late devit inlining happens while we have class probing.
This happens to enable late devirtualization for tier-1 methods under PGO, so we may see minor diffs for PGO cases.
cc @amanasifkhalid