JIT: don't dead store untracked OSR locals based on ref count #89743

AndyAyersMS · 2023-07-31T21:56:25Z

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.

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.

ghost · 2023-07-31T21:56:36Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

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.

Author:	AndyAyersMS
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

AndyAyersMS · 2023-07-31T21:56:42Z

@jakobbotsch PTAL
cc @dotnet/jit-contrib

AndyAyersMS · 2023-07-31T21:57:57Z

We will want to port this back to 7.0.

AndyAyersMS · 2023-07-31T23:35:37Z

No diffs locally, no diffs in CI, no TP impact.

jakobbotsch · 2023-08-01T08:30:45Z

src/coreclr/jit/liveness.cpp

+    // 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)


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.)

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.

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);

}

}

Using lvImplicitlyReferenced seems likely to be a bit more robust so I think I'll change to using that.

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.

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.

AndyAyersMS · 2023-08-01T20:06:22Z

This revised version will have a few diffs; the implicit weight changes the overall local weights, and this sometimes perturbs LSRA.

jakobbotsch · 2023-08-01T20:13:34Z

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.

AndyAyersMS · 2023-08-01T22:13:49Z

Errors are known. Diffs

AndyAyersMS · 2023-08-01T22:17:24Z

/backport to release/7.0-staging

github-actions · 2023-08-01T22:17:34Z

Started backporting to release/7.0-staging: https://github.com/dotnet/runtime/actions/runs/5732393141

github-actions · 2023-08-01T22:19:41Z

@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!

github-actions · 2023-08-01T22:19:42Z

@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 #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.

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jul 31, 2023

ghost assigned AndyAyersMS Jul 31, 2023

AndyAyersMS requested a review from jakobbotsch July 31, 2023 21:57

jakobbotsch reviewed Aug 1, 2023

View reviewed changes

use implicit reference instead

cabc310

jakobbotsch approved these changes Aug 1, 2023

View reviewed changes

This was referenced Aug 1, 2023

Tracking issue for CI build timeouts #76454

Closed

Test failure readytorun/GenericCycleDetection/Depth3Test/Depth3Test.sh #88586

Closed

AndyAyersMS merged commit 11126f8 into dotnet:main Aug 1, 2023
132 of 135 checks passed

AndyAyersMS mentioned this pull request Aug 2, 2023

JIT: don't dead store OSR exposed locals #89867

Merged

ghost locked as resolved and limited conversation to collaborators Sep 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: don't dead store untracked OSR locals based on ref count #89743

JIT: don't dead store untracked OSR locals based on ref count #89743

AndyAyersMS commented Jul 31, 2023

ghost commented Jul 31, 2023

AndyAyersMS commented Jul 31, 2023

AndyAyersMS commented Jul 31, 2023

AndyAyersMS commented Jul 31, 2023 •

edited

Loading

jakobbotsch Aug 1, 2023

jakobbotsch Aug 1, 2023

AndyAyersMS Aug 1, 2023

AndyAyersMS Aug 1, 2023

jakobbotsch Aug 1, 2023

AndyAyersMS Aug 1, 2023

AndyAyersMS commented Aug 1, 2023

jakobbotsch commented Aug 1, 2023

AndyAyersMS commented Aug 1, 2023

AndyAyersMS commented Aug 1, 2023

github-actions bot commented Aug 1, 2023

github-actions bot commented Aug 1, 2023

github-actions bot commented Aug 1, 2023

	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);
	}
	}

JIT: don't dead store untracked OSR locals based on ref count #89743

JIT: don't dead store untracked OSR locals based on ref count #89743

Conversation

AndyAyersMS commented Jul 31, 2023

ghost commented Jul 31, 2023

AndyAyersMS commented Jul 31, 2023

AndyAyersMS commented Jul 31, 2023

AndyAyersMS commented Jul 31, 2023 • edited Loading

jakobbotsch Aug 1, 2023

Choose a reason for hiding this comment

jakobbotsch Aug 1, 2023

Choose a reason for hiding this comment

AndyAyersMS Aug 1, 2023

Choose a reason for hiding this comment

AndyAyersMS Aug 1, 2023

Choose a reason for hiding this comment

jakobbotsch Aug 1, 2023

Choose a reason for hiding this comment

AndyAyersMS Aug 1, 2023

Choose a reason for hiding this comment

AndyAyersMS commented Aug 1, 2023

jakobbotsch commented Aug 1, 2023

AndyAyersMS commented Aug 1, 2023

AndyAyersMS commented Aug 1, 2023

github-actions bot commented Aug 1, 2023

github-actions bot commented Aug 1, 2023

github-actions bot commented Aug 1, 2023

AndyAyersMS commented Jul 31, 2023 •

edited

Loading