-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: don't dead store untracked OSR locals based on ref count #89743
Conversation
The local var ref count for OSR locals can be misleading, since some of the appearances may be in the "tier0" parts of the methods and won't be visible once we trim the OSR method down to just the part we're going to compile. So, we can't rely on ref counts to enable a dead store of an OSR local. Fixes dotnet#89666.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThe local var ref count for OSR locals can be misleading, since some of the appearances may be in the "tier0" parts of the methods and won't be visible once we trim the OSR method down to just the part we're going to compile. So, we can't rely on ref counts to enable a dead store of an OSR local. Fixes #89666.
|
@jakobbotsch PTAL |
We will want to port this back to 7.0. |
No diffs locally, no diffs in CI, no TP impact. |
src/coreclr/jit/liveness.cpp
Outdated
// For OSR locals we can't rely on ref counts this way, since some of the appearances | ||
// of the local may be in the now-unseen Tier0 portion. | ||
// | ||
if (isDef && compRationalIRForm && (varDsc.lvRefCnt() == 1) && !varDsc.lvPinned && !varDsc.lvIsOSRLocal) |
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.
Presumably the problem exists only for address-exposed locals (since otherwise we can see all uses, even in the OSR version). Add it as a condition to make the problem more self-evident? (I had to look at the test to understand why this would be illegal.)
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.
The interaction between OSR and flow opts here seems a little bit scary. We very much rely on a local like this one being address exposed in the OSR version, yet that only happens here because we import all IL and nothing optimizes the flow until after local morph. If we had any earlier flow opts (or if we ever tried to be smarter in local morph, e.g. via some RPO walk) then it seems like we would be able to notice that the code that exposes the local is statically unreachable.
It seems like long term it would be more robust to "lie" to the frontend of the JIT and wait with updating the flow until at least local morph has run.
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.
Presumably the problem exists only for address-exposed locals (since otherwise we can see all uses, even in the OSR version). Add it as a condition to make the problem more self-evident? (I had to look at the test to understand why this would be illegal.)
Yeah, I can add that.
Another option is to mark any OSR exposed local as implicitly referenced; we do that for pinned locals where we have similar issues not seeing all of the IR.
runtime/src/coreclr/jit/lclvars.cpp
Lines 276 to 295 in b5834a0
if ((corInfoTypeWithMod & CORINFO_TYPE_MOD_PINNED) != 0) | |
{ | |
if ((corInfoType == CORINFO_TYPE_CLASS) || (corInfoType == CORINFO_TYPE_BYREF)) | |
{ | |
JITDUMP("Setting lvPinned for V%02u\n", varNum); | |
varDsc->lvPinned = 1; | |
if (opts.IsOSR()) | |
{ | |
// OSR method may not see any references to the pinned local, | |
// but must still report it in GC info. | |
// | |
varDsc->lvImplicitlyReferenced = 1; | |
} | |
} | |
else | |
{ | |
JITDUMP("Ignoring pin for non-GC type V%02u\n", varNum); | |
} | |
} |
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.
Using lvImplicitlyReferenced
seems likely to be a bit more robust so I think I'll change to using that.
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.
Does the code in liveness also need to check for lvImplicitlyReferenced
? Or does something guarantee that lvRefCount
for implicitly referenced locals is something indeterminately large?
Overall I guess I would assume that all address exposed locals already have to effectively be considered implicitly referenced (whether or not we set that bit), but this seems fine with me.
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.
The ref count for implicit locals will be one greater than the number of actual occurrences, so will be at least 2 in this case.
This revised version will have a few diffs; the implicit weight changes the overall local weights, and this sometimes perturbs LSRA. |
I suppose it is a bit conservative/unnecessary to mark these as implicitly referenced; if we prove in the OSR version that it is not address exposed, then that should effectively override the conservative tier0 exposure information, and we can then safely reason about all defs/uses. |
Errors are known. Diffs |
/backport to release/7.0-staging |
Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/5732393141 |
@AndyAyersMS backporting to release/7.0-staging failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: JIT: don't dead store untracked OSR locals based on ref count
Applying: use implicit reference instead
error: sha1 information is lacking or useless (src/coreclr/jit/liveness.cpp).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 use implicit reference instead
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
@AndyAyersMS an error occurred while backporting to release/7.0-staging, please check the run log for details! Error: git am failed, most likely due to a merge conflict. |
Backport of dotnet#89743 to 7.0. The local var ref count for OSR locals can be misleading, since some of the appearances may be in the "tier0" parts of the methods and won't be visible once we trim the OSR method down to just the part we're going to compile. Fix by giving OSR-exposed locals an implicit ref count. Fixes dotnet#89666.
Backport of dotnet#89743 to 7.0. The local var ref count for OSR locals can be misleading, since some of the appearances may be in the "tier0" parts of the methods and won't be visible once we trim the OSR method down to just the part we're going to compile. Fix by giving OSR-exposed locals an implicit ref count. Fixes dotnet#89666.
Backport of #89743 to 7.0. The local var ref count for OSR locals can be misleading, since some of the appearances may be in the "tier0" parts of the methods and won't be visible once we trim the OSR method down to just the part we're going to compile. Fix by giving OSR-exposed locals an implicit ref count. Fixes #89666.
The local var ref count for OSR locals can be misleading, since some of the appearances may be in the "tier0" parts of the methods and won't be visible once we trim the OSR method down to just the part we're going to compile.
So, we can't rely on ref counts to enable a dead store of an OSR local.
Fixes #89666.