- 
                Notifications
    You must be signed in to change notification settings 
- Fork 5.2k
JIT: stop tracking TYP_I_IMPL locals in escape analysis #114130
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
JIT: stop tracking TYP_I_IMPL locals in escape analysis #114130
Conversation
Abstractly a non-GC local can't cause escape. On 32 bit platforms tracking connections to `TYP_I_IMPL` leads to confusion once we start struct field tracking, as we logically "overlay" GC and non-GC fields. Contributes to dotnet#104936
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (1)
- src/coreclr/jit/objectalloc.cpp: Language not supported
| Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch | 
| @jakobbotsch PTAL Looks like just one diff on win/x64 (x86 may have more, still running). | 
| We're walking up to some unexpected opcode ... just a guess but we likely need  | 
| // If we're not tracking stores to this local, the value | ||
| // does not escape. | ||
| if (!IsTrackedLocal(dstLclNum)) | ||
| { | ||
| canLclVarEscapeViaParentStack = false; | ||
| break; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that true? What if the object is pinned and a TYP_I_IMPL pointer to it is passed somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pinning only lasts for the current stack frame, and the object being pinned must already exist, so the pinning lifetime will be shorter than the allocating stack frame lifetime.
I suppose the object (or part of the object) could be exposed cross-thread this way, so even though it could be stack allocated it would not be thread private.
        
          
                src/coreclr/jit/objectalloc.cpp
              
                Outdated
          
        
      | if (parent->TypeGet() != newType) | ||
| { | ||
| parent->ChangeType(newType); | ||
| ++parentIndex; | ||
| keepChecking = true; | ||
| } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to keep retyping ancestors if the existing GT_SUB was not a pointer-result (e.g. byref - byref => TYP_I_IMPL)? I get it for the &byref - int case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we should stop propagation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this already happens, the only possibilities here are
parent    new
-----     ----
int       int
byref     byref
byref     int
so we only propagate in the byref->int case.
I will add some comments/asserts to make this clearer.
Abstractly a non-GC local can't cause escape. On 32 bit platforms tracking connections to
TYP_I_IMPLleads to confusion once we start struct field tracking, as we logically "overlay" GC and non-GC fields.Contributes to #104936