JIT: limit phi-refinement to loop-invariant VNs

[AndyAyersMS]

PhiArg VNs should be invariant in the loop that contains the Phi, otherwise the same VN may refer to values from more than one iteration.

Fixes #105792.

[AndyAyersMS]

@jakobbotsch PTAL
cc @dotnet/jit-contrib

Minimal diffs.

Still need to verify on the more complex repros for #105792.

[jakobbotsch]

Is this sufficient? Doesn't it need to use the same logic as optVNIsLoopInvariant?

[jakobbotsch]

E.g. for

    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Problem(int x)
    {
        int y = 0;
        while (x != 0)
        {
            if (y == x + 1) return -1;
            y = x + 1;
            Update(ref x);
        }
        return x;
    }

the VN will be VNF_ADD and LoopOfVN will return nullptr

[AndyAyersMS]

Ah, right.

We can directly use optVNIsLoopInvariant since we only ever try updating phis for loops.

[jakobbotsch]

Suggested change

	<ItemGroup>
	<CLRTestEnvironmentVariable Include="DOTNET_TieredCompilation" Value="0" />
	</ItemGroup>

(It won't have any effect without RequiresProcessIsolation anyway)

[AndyAyersMS]

I guess we get enough TC=0 coverage with external config settings.

[AndyAyersMS]

I think this version is now too conservative, and we are losing some of the key benefits of the original phi refinement. Eg we now see reversions in the key methods that #104752 was trying to improve

          16 (36.36 % of base) : 41867.dasm - System.Collections.IndexerSetReverse`1[int]:Array():int[]:this (FullOpts)

It seems like we should be able to handle having phi-defs as refinements, but not any VN that has a loop memory dependence. The key difference is that for tracked locations if we have a phi in the loop header we must also have an update in the loop body, so the "last value" of that location will be the one that shows up on the back edge PhiArg; whereas for a virtual location there is no Phi and we must impute a VN for the last value but have no means to do so; we cannot easily tell where the value might have been updated, so can't easily reason about what value the VN represents.

Need to look deeper into this... may not have time until tomorrow.

[AndyAyersMS]

@jakobbotsch PTAL once more -- this version allows refinement with phi defs since those accurately describe the previous iteration value. Only VNs that are implicitly memory dependent in the loop are excluded.

[AndyAyersMS]

@jakobbotsch PTAL once more -- this version allows refinement with phi defs since those accurately describe the previous iteration value. Only VNs that are implicitly memory dependent in the loop are excluded.

We had some offline discussion on this and there are still some questions about validity of this more aggressive refinement that are difficult to answer. It may be that the refinement is unsound but that checks in downstream phases compensate and prevent bad things from happening.

So I'm going to revert this last commit and go back to checking that any refined VN is actually a loop invariant.