Fix issue with hoisting and copy prop interaction #85493

BruceForstall · 2023-04-27T22:42:15Z

After hoisting creates a tree copy, it morphs it. That morph might lose the value numbers on LCL_VAR uses, but leave the SSA numbers. Copy prop was assuming that such a use had a VN. Instead, check this dynamically instead of asserting.

Fixes #84619

ghost · 2023-04-27T22:42:28Z

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

Issue Details

After hoisting creates a tree copy, it morphs it. That morph might lose the value numbers on LCL_VAR uses, but leave the SSA numbers. Copy prop was assuming that such a use had a VN. Instead, check this dynamically instead of asserting.

Fixes #84619

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

BruceForstall · 2023-04-27T22:42:35Z

/azp run Fuzzlyn

azure-pipelines · 2023-04-27T22:42:46Z

Azure Pipelines successfully started running 1 pipeline(s).

BruceForstall · 2023-04-28T05:05:45Z

No asm diffs

Slight TP cost

BruceForstall · 2023-04-28T05:06:20Z

@AndyAyersMS PTAL
cc @dotnet/jit-contrib

AndyAyersMS · 2023-04-28T06:28:56Z

After hoisting creates a tree copy, it morphs it. That morph might lose the value numbers on LCL_VAR uses, but leave the SSA numbers. Copy prop was assuming that such a use had a VN. Instead, check this dynamically instead of asserting.

Fixes #84619

I hate to open a can of worms here, but why does hoisting call morph? Seems like maybe it shouldn't.

BruceForstall · 2023-04-28T18:42:48Z

I hate to open a can of worms here, but why does hoisting call morph?

Disabling morphing only causes one family of diffs.

Another question, though: why does morphing lose VNs on LCL_VARs?

SingleAccretion · 2023-04-28T18:51:11Z

The case from #84619 is optNarrowTree's work, and it cannot (easily) preserve VNs on trees that it transforms (although it has some logic that tries). What we've been doing with such transformation is putting them under if (fgGlobalMorph). This may be expensive CQ-wise in this case.

AndyAyersMS · 2023-04-28T18:55:46Z

I hate to open a can of worms here, but why does hoisting call morph?

Disabling morphing only causes one family of diffs.

I was wondering if you'd actually see improvements, since hoisting relies on CSE and perhaps morphing the hoisted copy would break CSE (either by changing tree shape so it no longer matches the in-loop tree or perhaps by losing the GTF_MAKE_CSE flag) and hence messes up hoisting.

Oddly we don't check for GTF_MAKE_CSE in CSE proper, we just use it to keep morph and other things from deleting the thing we just hoisted.

Another question, though: why does morphing lose VNs on LCL_VARs?

Could be there's a missing surgical VN update that could be fixed, you'd have to dig in.

AndyAyersMS · 2023-05-09T02:15:36Z

@BruceForstall you're not waiting on me here, are you?

BruceForstall · 2023-05-09T18:07:14Z

@BruceForstall you're not waiting on me here, are you?

No. But the questions asked require investigations, which I've prioritized very low compared to other work (even though it would be nice to get Fuzzlyn cleaner, but it has good baselining anyway)

After hoisting creates a tree copy, it morphs it. That morph might lose the value numbers on LCL_VAR uses, but leave the SSA numbers. Copy prop was assuming that such a use had a VN. Instead, check this dynamically instead of asserting. Fixes dotnet#84619

If we optimize the cast, call `fgOptimizeCast` to see if it can be optimized further.

BruceForstall · 2023-05-16T04:14:32Z

@AndyAyersMS I updated this. I found the case that was creating a CAST that wasn't getting fully morphed/optimized, in fgOptimizeCastOnStore, and added a call to fgOptimizeCast. With this, the hoisted code that gets morphed doesn't actually optimize anything, and so the assert is avoided. (It turns out it doesn't create a CSE in this test case, either, but I didn't look into why.) I propose to leave the dynamic check in optBlockCopyProp for safety. Finally, if I remove morphing from hoisting there are no diffs now (at least on win-x64). Perhaps removing that can be done as a follow-up.

I also added a unit test.

There are a few diffs, with the additional call to fgOptimizeCast causing a few more cases of narrowing to kick in.

There is a bit of TP hit.

SingleAccretion · 2023-05-16T15:45:31Z

src/coreclr/jit/copyprop.cpp

+            else if (tree->OperIs(GT_LCL_VAR, GT_LCL_FLD) && tree->AsLclVarCommon()->HasSsaName() &&
+                     tree->gtVNPair.BothDefined())


I would recommend removing this check. optCopyProp doesn't actually depends on the VNs being present for trees it examines, so this is effectively the same as removing the assert.

optCopyProp does assert(tree->gtVNPair.BothDefined()) which is why I added this.

But it's true that optCopyProp does not actually use the tree->gtVNPair : it looks up the VN in the lvPerSsaData for the SSA def of the local. Presumably this assert is a partial expression of the expectation that if a local has an SSA number, then its SSA def should have the same VN as the SSA use (I believe that should be true?). It would therefore seem odd to ignore the fact that a lclvar use has no VN but its SSA does, and just use the def. That seems to indicate something is odd/wrong, perhaps due to morph, like what happened here.

Of course, I could remove the assert from optCopyProp, too.

Presumably this assert is a partial expression of the expectation that if a local has an SSA number, then its SSA def should have the same VN as the SSA use (I believe that should be true?).

Here is how this situation came to be: optCopyProp used to use the tree VN, but was changed to use the definition VN in #65250, with the main motivation being LCL_FLD handling. I left the tree VN assert as it seemed valuable on its own (indeed, as you say, to catch missing VNs - we are generally sorely lacking in validating this aspect of morph).

So it seems like I could either:

Remove both the tree->gtVNPair.BothDefined() check here and assert(tree->gtVNPair.BothDefined()) in optCopyProp (so optCopyProp will no longer check for "missing" VNs), or

Leave it as I've written it -- with more testing/checking than maybe should be required, or

Something else? (e.g., asserting for and fixing "missing VNs" -- but that seems out of scope for this fix)

Do you have a preference?

My preference would be to check for missing VNs and fix them (leave the assert without the check, or even migrate it to the general loop in optBlockCopyProp), but barring that removing the assert (1) seems better.

I think the SSA checker should also be able to spot missing VNs, though not perhaps in as timely a manner.

I think the SSA checker should also be able to spot missing VNs, though not perhaps in as timely a manner.

Well, fgDebugCheckSsa() already has this comment: * Verify VNs on uses match the VN on the def :-)

BruceForstall · 2023-05-16T22:48:16Z

Ok, I've removed the assert in optCopyProp.

BruceForstall · 2023-05-17T16:40:54Z

@AndyAyersMS ping

BruceForstall · 2023-05-18T16:10:42Z

ping

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

ghost assigned BruceForstall Apr 27, 2023

BruceForstall requested a review from AndyAyersMS April 28, 2023 05:06

build-analysis bot mentioned this pull request Apr 28, 2023

Tracking issue for CI build timeouts #76454

Closed

BruceForstall added 3 commits May 15, 2023 14:57

Further optimize cast in fgOptimizeCastOnStore()

43a2af8

If we optimize the cast, call `fgOptimizeCast` to see if it can be optimized further.

Add regression test

6235fe1

BruceForstall force-pushed the Fix84619 branch from a13d234 to 6235fe1 Compare May 16, 2023 01:43

runfoapp bot mentioned this pull request May 16, 2023

Infra improvements for Helix #68176

Closed

SingleAccretion reviewed May 16, 2023

View reviewed changes

Feedback

ae027b0

SingleAccretion approved these changes May 16, 2023

View reviewed changes

build-analysis bot mentioned this pull request May 17, 2023

Test failure tracing/runtimeeventsource/nativeruntimeeventsource/nativeruntimeeventsource.sh #86233

Closed

AndyAyersMS approved these changes May 18, 2023

View reviewed changes

BruceForstall merged commit 32b7be7 into dotnet:main May 18, 2023

BruceForstall deleted the Fix84619 branch May 18, 2023 16:16

ghost locked as resolved and limited conversation to collaborators Jun 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with hoisting and copy prop interaction #85493

Fix issue with hoisting and copy prop interaction #85493

BruceForstall commented Apr 27, 2023

ghost commented Apr 27, 2023

BruceForstall commented Apr 27, 2023

azure-pipelines bot commented Apr 27, 2023

BruceForstall commented Apr 28, 2023

BruceForstall commented Apr 28, 2023

AndyAyersMS commented Apr 28, 2023

BruceForstall commented Apr 28, 2023

SingleAccretion commented Apr 28, 2023

AndyAyersMS commented Apr 28, 2023

AndyAyersMS commented May 9, 2023

BruceForstall commented May 9, 2023

BruceForstall commented May 16, 2023

SingleAccretion May 16, 2023

BruceForstall May 16, 2023

SingleAccretion May 16, 2023

BruceForstall May 16, 2023

SingleAccretion May 16, 2023

AndyAyersMS May 16, 2023

BruceForstall May 16, 2023

BruceForstall commented May 16, 2023

BruceForstall commented May 17, 2023

BruceForstall commented May 18, 2023

		else if (tree->OperIs(GT_LCL_VAR, GT_LCL_FLD) && tree->AsLclVarCommon()->HasSsaName() &&
		tree->gtVNPair.BothDefined())

Fix issue with hoisting and copy prop interaction #85493

Fix issue with hoisting and copy prop interaction #85493

Conversation

BruceForstall commented Apr 27, 2023

ghost commented Apr 27, 2023

BruceForstall commented Apr 27, 2023

azure-pipelines bot commented Apr 27, 2023

BruceForstall commented Apr 28, 2023

BruceForstall commented Apr 28, 2023

AndyAyersMS commented Apr 28, 2023

BruceForstall commented Apr 28, 2023

SingleAccretion commented Apr 28, 2023

AndyAyersMS commented Apr 28, 2023

AndyAyersMS commented May 9, 2023

BruceForstall commented May 9, 2023

BruceForstall commented May 16, 2023

SingleAccretion May 16, 2023

Choose a reason for hiding this comment

BruceForstall May 16, 2023

Choose a reason for hiding this comment

SingleAccretion May 16, 2023

Choose a reason for hiding this comment

BruceForstall May 16, 2023

Choose a reason for hiding this comment

SingleAccretion May 16, 2023

Choose a reason for hiding this comment

AndyAyersMS May 16, 2023

Choose a reason for hiding this comment

BruceForstall May 16, 2023

Choose a reason for hiding this comment

BruceForstall commented May 16, 2023

BruceForstall commented May 17, 2023

BruceForstall commented May 18, 2023