Fix value numbering of field assignments

[SingleAccretion]

When value numbering an assignment to a field, VNApplySelectorsAssign was called twice, first for the field sequence to find the exact value, then for the heap VN update. This is not correct as the method expects to be called only for sequences that will end up updating a map with exact values, as it type checks the store with the helper VNApplySelectorsAssignTypeCoerce. Usage of VNApplySelectorsAssign to update the heap ended up meaning that any stores to struct fields ended up with casts for maps, blocking traversal in VNForMapSelect.

A handful of positive diffs for this commit resulting from more CSEs and redundant branch optimizations, due to more precise liberal VNs. About the same number of (size) regressions for the same reasons, e. g. CSE discovering new defs for CSEs without the exception sets of all uses.

Diffs: win-x64, win-arm64, win-x86.

Part of #58312.

[ghost]

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

Issue Details

---

When value numbering an assignment to a field, VNApplySelectorsAssign was called twice, first for the field sequence to find the exact value, then for the heap VN update. This is not correct as the method expects to be called only for sequences that will end up updating a map with exact values, as it type checks the store with the helper VNApplySelectorsAssignTypeCoerce. Usage of VNApplySelectorsAssign to update the heap ended up meaning that any stores to struct fields ended up with casts for maps, blocking traversal in VNForMapSelect.

A handful of positive diffs for this commit resulting from more CSEs and redundant branch optimizations, due to more precise liberal VNs. Some regressions for the same reasons, e. g. CSE discovering new defs for CSEs without the exception sets of all uses.

Diffs: win-x64.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr, community-contribution
Milestone:	-

[SingleAccretion]

@dotnet/jit-contrib

[jakobbotsch]

Do we not need to unpack the VN like before?

[jakobbotsch]

Ah, scratch that, I see this just uses VNLiberalNormalValue now.

[SingleAccretion]

Note that the tracking of exception sets this code did before, i. e.

vnExcSet = vnStore->VNExcSetUnion(vnExcSet, vnObjExcSet);

was dead code because nothing uses vnExcSet in this method - the ASG tree is assigned VNForVoid() at the end, always, i. e. we were always losing all the exceptions. We are "ok" correctness-wise here only because CSE is the only consumer of exception sets right now and it does not look at trees marked with GTF_ASG.

(Edit: in general, one should assume that the exception sets are only correct "for CSE purposes", because e. g. VNHasExc(unique VN assigned to a user call) will be false)

[jakobbotsch]

Should we be using TYP_REF instead of firstFieldType here accordingly to the discussion last week?

[SingleAccretion]

Yes we should, but as standalone change, it will create regressions because the reading side of this code needs to be updated to use the same type (it is using firstFieldType at the moment). This is the reason for the 4th commit:

This is a temporary change to minimize diffs.
The issue is that we need to update both the
reading and writing code at once if we want
to change the types of the maps without some
regressions due to new artificial mismatches.

In the next change, I will update the reading side, and in the change after that - the types for the heaps/"first field maps" (then will come the many changes needed to enable clean validation, after that the validation itself, and finally the actual bugfixes).

[jakobbotsch]

Is this dead code?

[SingleAccretion]

As far as I know, yes, because we use CLS_VAR for these cases.

(I would not be surprised if it is reachable under some exotic circumstances of course)

[jakobbotsch]

I don't think so, IsFieldAddr can never return true without assigning one of the out args. Maybe at some point code like the following would hit this path:

public class Program
{
    public static S s_s;
    public static int s_i;

    public static void Main()
    {
        s_s = s_s;
        Foo();
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static unsafe void Foo()
    {
        Bar(ref s_i);
    }

    public static void Bar(ref int i) => i = 10;
}

but currently we do not manage to VN this:

***** BB01, STMT00002(before)
N004 (  6, 14) [000006] -A--G-------              ▌  ASG       int   
N002 (  4, 12) [000005] *------N----              ├──▌  IND       int   
N001 (  2, 10) [000003] I-----------              │  └──▌  CNS_INT(h) byref  0x7ffa80d505b8 static Fseq[s_i]
N003 (  1,  1) [000004] ------------              └──▌  CNS_INT   int    10

N001 [000003]   CNS_INT(h) 0x7ffa80d505b8 static Fseq[s_i] => $c0 {Hnd const: 0x00007FFA80D505B8}
N003 [000004]   CNS_INT   10 => $41 {IntCns 10}
  fgCurMemoryVN[GcHeap] assigned for assign-of-IND at [000006] to VN: $81.
N004 [000006]   ASG       => $VN.Void

***** BB01, STMT00002(after)
N004 (  6, 14) [000006] -A--G-------              ▌  ASG       int    $VN.Void
N002 (  4, 12) [000005] *------N----              ├──▌  IND       int    $41
N001 (  2, 10) [000003] I-----------              │  └──▌  CNS_INT(h) byref  0x7ffa80d505b8 static Fseq[s_i] $c0
N003 (  1,  1) [000004] ------------              └──▌  CNS_INT   int    10 $41

[jakobbotsch]

LGTM, thanks!

[kunalspathak]

windows/x64 improvements - dotnet/perf-autofiling-issues#2204