Conversation
If a method has GDVs and the JIT is able to to improve types during inlining, run a pass after inlining that tries to resolve GDVs using those improved types. This typically happens when the object tested in a GDV is returned from some kind of factory method where the exact type becomes apparent only after inlining. Contributes to dotnet#25448.
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
@EgorBot --filter "System.Collections.IterateForEach*.*Dictionary" |
|
libraries tests and SPMI both hit an assert EgorBot confirms this improves the |
|
Can't repro the SPMI assert. I have some other changes to make here so will see if it recurs when I push those. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new JIT phase that resolves Guarded Devirtualization (GDV) checks using improved type information discovered during inlining. Key changes include additions to the GuardInfo structure, a new static IsGuard method implementation, a new fgResolveGDVs phase in fgopt.cpp, and updates in the compiler phase handling to integrate this new functionality.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/objectalloc.h | Added new fields (m_stmt, m_relop) in GuardInfo and updated the IsGuard method signature. |
| src/coreclr/jit/objectalloc.cpp | Updated IsGuard implementation to capture guard-related nodes, but missing assignment of m_block. |
| src/coreclr/jit/fgopt.cpp | Added a new fgResolveGDVs phase that resolves GDV checks based on improved type information. |
| src/coreclr/jit/fginline.cpp | Updated inline walker to set hasUpdatedTypeLocals when type updates occur during inlining. |
| src/coreclr/jit/compphases.h | Registered the new PHASE_RESOLVE_GDVS phase. |
| src/coreclr/jit/compiler.h | Added a flag and declaration for fgResolveGDVs. |
| src/coreclr/jit/compiler.cpp | Invoked fgResolveGDVs in the main compile flow if optimizations are enabled. |
|
@dotnet/jit-contrib PTAL Arguably we should try folding other branches here too, though I don't know of any other cases where we'd expect to succeed, so have kept this narrowly focused for now. |
| @@ -3204,6 +3204,9 @@ GenTree* ObjectAllocator::IsGuard(BasicBlock* block, GuardInfo* info) | |||
| bool isNonNull = false; | |||
There was a problem hiding this comment.
nit for style:
- GenTree* ObjectAllocator::IsGuard(BasicBlock* block, GuardInfo* info)
+ /* static */ GenTree* ObjectAllocator::IsGuard(BasicBlock* block, GuardInfo* info)There was a problem hiding this comment.
Thanks. I will add this to my (growing) list of deferred cleanups.
There was a problem hiding this comment.
Is that actually in our coding conventions? I certainly never add those and prefer to keep them only in the header.
There was a problem hiding this comment.
It's mentioned here, though with a slightly different format. To be honest, I based this suggestion on what I've seen elsewhere in the codebase, rather than what the guidelines say -- in some respects, we've diverged quite a bit from them (or the document hasn't been maintained).
If a method has GDVs and the JIT is able to to improve types during inlining, run a pass after inlining that tries to resolve GDVs using those improved types.
This typically happens when the object tested in a GDV is returned from some kind of factory method where the exact type becomes apparent only after inlining.
Contributes to #25448.