Don't allow the hoisting of GT_CLS_VARs that were assigned a constant value.

[briansull]

Fixes Issue #26417

[briansull]

@dotnet/jit-contrib PTAL
@BruceForstall

[briansull]

Updated fix.

@dotnet/jit-contrib PTAL

[CarolEidt]

I might just request a bit more lengthy comment here. As I understand it, the reason we can't do this thing that seems safe, though perhaps not all that useful, is that we're not checking safety that something that's known to evaluate to a constant is not being modified within the loop. If I have that right, it would be nice to add that here so that people (like me) won't be confused in future when they see this.

[erozenfeld]

Is this the right place for this fix? optVNIsLoopInvariant has logic for determining whether the definition is inside the loop. That logic is not executed in this case because of this code:

coreclr/src/jit/optimizer.cpp

Lines 7197 to 7201 in ef585a4

	// We'll always short-circuit constants.
	if (vnStore->IsVNConstant(vn) || vn == vnStore->VNForVoid())
	{
	return true;
	}

Perhaps that short-circuit should be removed so that we try to determine if the definition is inside the loop even for const vn's.

[briansull]

That function examines the vn passed in and sees it it is a VNF_PhiDef or a VNF_PhiMemoryDef.

So it can only identify the GT_LCL_VARS that have definitions that are tracked by Phi nodes.

When we have a constant vn we can't trace back to the definition.

[erozenfeld]

optVNIsLoopInvariant also has logic for when the vn is not VNF_PhiDef or a VNF_PhiMemoryDef: it checks function args if vn is a function and it checks vn loop annotations if vn is not a function.

[briansull]

That handles things like binary operators such as Add, Sub, etc..
Constant values are terminal node with no children, only a value.

[mikedn]

That handles things like binary operators such as Add, Sub, etc..

That part of VN invariance is actually dubious since it's redundant to what the rest of loop hoisting does.

[briansull]

That part of VN invariance is actually dubious

One important case is handled here:
We have several "pure" Jit helper functions that take constants as arguments.
It is important that such calls get hoisted into the loop pre-header and then turned into CSE's:

Examples are CORINFO_HELP_GETSHARED_GCSTATIC_BASE and CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE

[mikedn]

Those are already handled in HoistVisitor::PostOrderVisit - it already checks for pure helpers with invariant args.

As far as I can tell the only good reason to check VN invariance is to detect memory loads that are invariant. Apart from that the VN invariance check is effectively redundant and confusing:

• PostOrderVisit determines that a and b are invariant and then trivially concludes that a + b is also invariant.
• At that point it checks VN invariance - it goes "down" the VN "tree" to check if a + b is invariant

Basically as PostOrderVisit goes up the expression tree it keeps going down the VN tree again and again attempting to check what has already been checked. If it weren't for loopVnInvariantCache this would probably rather slow.

To make things worse, VN invariance could handle the case of a variable defined inside the loop by an invariant expression. Sadly this doesn't happen because PostOrderVisit specifically requires that the definition be outside the loop and doesn't check VN invariance if the def is inside the loop.

[erozenfeld]

What are the diffs with this fix?

[briansull]

In System.Private.CoreLib no diffs
working on the rest of the diffs

[briansull]

No diffs in --frameworks

[erozenfeld]

LGTM
Perhaps we should open an issue to address cleanup and improvement suggestions by @mikedn (#26551 (comment)).

[CarolEidt]

A couple of minor things.

[briansull]

With the current fix we will see a regression in a couple of the benchmarks:

         -39 : JIT\Performance\CodeQuality\Benchstones\BenchF\Simpsn\Simpsn\Simpsn.dasm (-4.74% of base)
         -29 : JIT\Performance\CodeQuality\Benchstones\BenchF\Trap\Trap\Trap.dasm (-3.66% of base)

The old implementation will hoist expressions such as this one found in
Performance\CodeQuality\Benchstones\BenchF\Simpsn

Hoisting a copy of [000223] into PreHeader for loop L01 <BB03..BB05>:
N005 ( 46, 15) [000223] --C---------              *  INTRINSIC double exp $285
N004 ( 10, 11) [000222] ------------              \--*  MUL       double $49
N002 (  2,  3) [000220] ------------                 +--*  NEG       double $49
N001 (  1,  2) [000098] ------------                 |  \--*  LCL_VAR   double V00 loc0         u:2 $40
N003 (  3,  4) [000221] ------------                 \--*  CNS_DBL   double 2.0000000000000000 $45

[briansull]

I will implement a new fix that replaces nodes in the hoisted expressions that have constant value number, but are not true constants such as GT_CNS_INT or GT_CNS_DBL

[briansull]

I am going to limit this to just GT_CLS_VARS. so that we don't regress the cases that improve the CodeQuality tests.

Running the Asm diffs

[erozenfeld]

With this fix we still have a bug here since we'll hoist any non-GT_CLS_VAR node with a constant value number, which is unsafe. I like your previous suggestion

I will implement a new fix that replaces nodes in the hoisted expressions that have constant value number, but are not true constants such as GT_CNS_INT or GT_CNS_DBL

better. Was there a problem with that approach?

[briansull]

It is a much bigger change and I don't think it is required to fix this issue.
Only assignments can create constant value-numbers.
I will continue to investigate further, and likely implement the other fix.
But I would prefer to close this issue with a small fix that has zero Asm Diffs.

[erozenfeld]

The issue can potentially happen with assignments of constants to non-faulting indirections (e.g., fields or array elements). I couldn't get a repro with an indirection but only because we recognize the indirections as non-faulting too late.

[jkotas]

@briansull This PR was merged with failing test in coreclr-ci (Run Test Pri0 Windows_NT x86 checked).. This failure was most likely introduced by this change and all PR jobs are failing because of it. I submitting a PR to revert this change.

[BruceForstall]

New proposed fix: #26952