Properly type primary selectors in fgMemoryVNForLoopSideEffects

[SingleAccretion]

When performing value numbering of the initial memory state for blocks, we treat single-entry loops specially. First, we walk the loop to determine if it contains non-analyzable (from VN's point of view, i. e. not stores to fields or arrays) memory-related side effects ("havoc"). While doing so, we collect the handles of fields and types of arrays, later to be used as primary selectors in VN. If the loop is determined to not have "havoc", we compute the initial memory VN (which would ordinarily consist of a PHI with one unknown input) as follows: take the outgoing VN of the non-loop predecessor, and reset all the maps corresponding to primary selectors that we know will be stored to in the loop from the previous walk to "new, unique" values.

This then allows us to find the values for locations that were not modified in the loop when walking the MapStore chains while numbering nodes in the loop.

This is all rather nice, but has one flaw: the VNs given to primary maps (corresponding to the values obtained via indexing into the heap with a primary selector) all have TYP_REF types, which is not correct for static fields, and this change fixes that.

No diffs for this change (as expected).

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 performing value numbering of the initial memory state for loop entries, we treat single-entry loops specially. First, we walk the loop to determine it contains non-analyzable (from VN's point of view, i. e. not stores to fields or arrays) memory-related side effects ("havoc"). While doing so, we collect the handles of fields and types of arrays, later to be used as primary selectors in VN, of modified fields. If the loop is determined to not have "havoc", we compute the initial memory VN (which would ordinarily consist of a PHI with one unknown input) as follows: take the outgoing VN of the non-loop predecessor, and reset all the maps corresponding to primary selectors that we know will be stored to in the loop from the previous walk to "new, unique" values.

This then allows us to find the values for locations that were not modified in the loop when walking the MapStore chains while numbering nodes in the loop.

This is all rather nice, but has one flaw: the VNs given to primary maps (corresponding to the values obtained via indexing into the heap with a primary selector) all have TYP_REF types, which is strictly not "right" for at least static fields, and for others, confuses the code that numbers stores to indirections.

Consider the following C# snippet:

private static void Problem(ClassWithFields a)
{
    for (int i = 0; i < 10; i++)
    {
        a.Field++;

        if (a.Field == 1)
        {
            break;
        }
    }
}

We had the following numbering for the interesting trees:

finish(BB02).
SSA PHI definition: set VN of local 2/3 to $280 {PhiDef($2, $3, $d9)} .
Computing GcHeap state for block BB07, entry block for loops 0 to 0:
  Init GcHeap state is $247, with new, fresh VN at:
     VNForHandle(hackishFieldName) is $187
    VNForMapStore($247, $187, $8e):ref in BB07 returns $2c0 {$247[$187 := $8e]@L00}
     Array map int[]
    VNForMapStore($2c0, $188, $8f):ref in BB07 returns $2c1 {$2c0[$188 := $8f]@L00}
  Final GcHeap state is $2c1.
The SSA definition for GcHeap (#6) at start of BB07 is $2c1 {$2c0[$188 := $8f]@L00}

***** BB07, STMT00007(before)
N011 ( 11, 11) [000035] -A-XG---R---              *  ASG       int   
N010 (  4,  4) [000034] D--XG--N----              +--*  IND       int   
N009 (  2,  2) [000120] -------N----              |  \--*  ADD       byref 
N007 (  1,  1) [000029] ------------              |     +--*  LCL_VAR   ref    V00 this         u:1
N008 (  1,  1) [000119] ------------              |     \--*  CNS_INT   long   20 field offset Fseq[hackishFieldName]
N006 (  6,  6) [000033] ---XG-------              \--*  ADD       int   
N004 (  4,  4) [000031] ---XG-------                 +--*  IND       int   
N003 (  2,  2) [000122] -------N----                 |  \--*  ADD       byref 
N001 (  1,  1) [000030] ------------                 |     +--*  LCL_VAR   ref    V00 this         u:1
N002 (  1,  1) [000121] ------------                 |     \--*  CNS_INT   long   20 field offset Fseq[hackishFieldName]
N005 (  1,  1) [000032] ------------                 \--*  CNS_INT   int    1

N001 [000030]   LCL_VAR   V00 this         u:1 => $80 {InitVal($40)}
N002 [000121]   CNS_INT   20 field offset Fseq[hackishFieldName] => $105 {LngCns:  20}
N003 [000122]   ADD       => $148 {ADD($80, $105)}
  VNApplySelectors:
    VNForHandle(hackishFieldName) is $187, fieldType is int
      AX2: $187 != $188 ==> select([$2c1]store($2c0, $188, $8f), $187) ==> select($2c0, $187) remaining budget is 99.
      AX1: select([$247]store($2c0, $187, $8e), $187) ==> $8e.
      ==> Not updating loop memory dependence of [000031], memory $247 not defined in a loop
    VNForMapSelect($2c1, $187):int returns $8e {MemOpaque:L00}
    VNForMapSelect($8e, $80):ref returns $210 {$8e[$80]}
    VNForCastOper(int) is $48
    VNForCast($210, $48) returns $da {Cast($210, $48)}
N004 [000031]   IND       => <l:$dc {norm=$da {Cast($210, $48)}, exc=$200 {NullPtrExc($80)}}, c:$db {norm=$1c4 {MemOpaque:L00}, exc=$200 {NullPtrExc($80)}}>
N005 [000032]   CNS_INT   1 => $41 {IntCns 1}
N006 [000033]   ADD       => <l:$e0 {norm=$de {ADD($41, $da)}, exc=$200 {NullPtrExc($80)}}, c:$df {norm=$dd {ADD($41, $1c4)}, exc=$200 {NullPtrExc($80)}}>
N007 [000029]   LCL_VAR   V00 this         u:1 => $80 {InitVal($40)}
N008 [000119]   CNS_INT   20 field offset Fseq[hackishFieldName] => $105 {LngCns:  20}
N009 [000120]   ADD       => $148 {ADD($80, $105)}
  VNApplySelectors:
    VNForHandle(hackishFieldName) is $187, fieldType is int
      AX2: $187 != $188 ==> select([$2c1]store($2c0, $188, $8f), $187) ==> select($2c0, $187) remaining budget is 99.
      AX1: select([$247]store($2c0, $187, $8e), $187) ==> $8e.
      ==> Not updating loop memory dependence of [000035], memory $247 not defined in a loop
    VNForMapSelect($2c1, $187):int returns $8e {MemOpaque:L00}
    VNForMapSelect($8e, $80):ref returns $210 {$8e[$80]}
    VNForMapStore($8e, $80, $de):ref in BB07 returns $2c2 {$8e[$80 := $de]@L00}
  VNApplySelectorsAssign:
    VNForHandle(hackishFieldName) is $187, fieldType is int
    VNForCastOper(int) is $48
    VNForCast($2c2, $48) returns $e1 {Cast($2c2, $48)}
    Cast to int inserted in VNApplySelectorsAssignTypeCoerce (elemTyp is ref)
    VNForMapStore($2c1, $187, $e1):int in BB07 returns $24a {$2c1[$187 := $e1]@L00}
  fgCurMemoryVN[GcHeap] assigned for StoreField at [000035] to VN: $24a.
N011 [000035]   ASG       => $VN.Void

***** BB07, STMT00007(after)
N011 ( 11, 11) [000035] -A-XG---R---              *  ASG       int    $VN.Void
N010 (  4,  4) [000034] D--XG--N----              +--*  IND       int    $de
N009 (  2,  2) [000120] -------N----              |  \--*  ADD       byref  $148
N007 (  1,  1) [000029] ------------              |     +--*  LCL_VAR   ref    V00 this         u:1 $80
N008 (  1,  1) [000119] ------------              |     \--*  CNS_INT   long   20 field offset Fseq[hackishFieldName] $105
N006 (  6,  6) [000033] ---XG-------              \--*  ADD       int    <l:$e0, c:$df>
N004 (  4,  4) [000031] ---XG-------                 +--*  IND       int    <l:$dc, c:$db>
N003 (  2,  2) [000122] -------N----                 |  \--*  ADD       byref  $148
N001 (  1,  1) [000030] ------------                 |     +--*  LCL_VAR   ref    V00 this         u:1 $80
N002 (  1,  1) [000121] ------------                 |     \--*  CNS_INT   long   20 field offset Fseq[hackishFieldName] $105
N005 (  1,  1) [000032] ------------                 \--*  CNS_INT   int    1 $41

---------

***** BB07, STMT00008(before)
N007 (  8,  8) [000040] ---XG-------              *  JTRUE     void  
N006 (  6,  6) [000039] J--XG--N----              \--*  LE        int   
N004 (  4,  4) [000037] ---XG-------                 +--*  IND       int   
N003 (  2,  2) [000124] -------N----                 |  \--*  ADD       byref 
N001 (  1,  1) [000036] ------------                 |     +--*  LCL_VAR   ref    V00 this         u:1
N002 (  1,  1) [000123] ------------                 |     \--*  CNS_INT   long   20 field offset Fseq[hackishFieldName]
N005 (  1,  1) [000038] ------------                 \--*  CNS_INT   int    26

N001 [000036]   LCL_VAR   V00 this         u:1 => $80 {InitVal($40)}
N002 [000123]   CNS_INT   20 field offset Fseq[hackishFieldName] => $105 {LngCns:  20}
N003 [000124]   ADD       => $148 {ADD($80, $105)}
  VNApplySelectors:
    VNForHandle(hackishFieldName) is $187, fieldType is int
      AX1: select([$2c1]store($24a, $187, $e1), $187) ==> $e1.
      ==> Updating loop memory dependence of [000037] to L00
    VNForMapSelect($24a, $187):int returns $e1 {Cast($2c2, $48)}
    VNForMapSelect($e1, $80):int returns $e2 {$e1[$80]}
N004 [000037]   IND       => <l:$e4 {norm=$e2 {$e1[$80]}, exc=$200 {NullPtrExc($80)}}, c:$e3 {norm=$1c5 {MemOpaque:L00}, exc=$200 {NullPtrExc($80)}}>
N005 [000038]   CNS_INT   26 => $49 {IntCns 26}
N006 [000039]   LE        => <l:$e8 {norm=$e6 {LE($e2, $49)}, exc=$200 {NullPtrExc($80)}}, c:$e7 {norm=$e5 {LE($1c5, $49)}, exc=$200 {NullPtrExc($80)}}>

***** BB07, STMT00008(after)
N007 (  8,  8) [000040] ---XG-------              *  JTRUE     void  
N006 (  6,  6) [000039] J--XG--N----              \--*  LE        int    <l:$e8, c:$e7>
N004 (  4,  4) [000037] ---XG-------                 +--*  IND       int    <l:$e4, c:$e3>
N003 (  2,  2) [000124] -------N----                 |  \--*  ADD       byref  $148
N001 (  1,  1) [000036] ------------                 |     +--*  LCL_VAR   ref    V00 this         u:1 $80
N002 (  1,  1) [000123] ------------                 |     \--*  CNS_INT   long   20 field offset Fseq[hackishFieldName] $105
N005 (  1,  1) [000038] ------------                 \--*  CNS_INT   int    26 $49

As can be seen, there is a cast inserted when VNApplySelectorsAssign performs the map updates for the store, it is coming from here:

runtime/src/coreclr/jit/valuenum.cpp

Lines 8325 to 8330 in 972891f

	// Update the field map for firstField in GcHeap to this new value.
	ValueNum heapVN =
	vnStore->VNApplySelectorsAssign(VNK_Liberal, fgCurMemoryVN[GcHeap], firstFieldOnly,
	newFldMapVN, indType, compCurBB);
	recordGcHeapStore(tree, heapVN DEBUGARG("StoreField"));

Here newFldMapVN will have TYP_REF, indType - TYP_INT, causing the mismatch, because the code just above uses TypeOfVN(fldMapVN):

runtime/src/coreclr/jit/valuenum.cpp

Lines 8301 to 8303 in 972891f

	// From which we can construct the new ValueNumber for 'fldMap at normVal'
	newFldMapVN = vnStore->VNForMapStore(vnStore->TypeOfVN(fldMapVN), fldMapVN, normVal,
	storeVal);

The cast is inserted at an unfortunate point: above the primary selector map (in this case, the field map), breaking the MapStore chain and causing subsequent load to not find the value from the store. Giving proper types to the primary selector maps helps avoid that.

Now, a keen reader will notice that the code above is actually just wrong, as it will constantly insert casts in cases when the first fields is actually of a struct type, and where no casts are needed at all, as they, if required, would have been inserted in the VNApplySelectorsAssign call that "gets rid of the remaining struct references". So the question is: why am I not fixing this code, and instead fixing some other code that just happens to trigger this bug in another way?

The answer is that I think it is beneficial to the model to have it be typed consistently, which is both more understandable and more efficient (by avoiding allocating unnecessary VNs). Changes to VN are somewhat high in risk, so I will be making them very incrementally, and in small chunks. Ideally, most of them should be zero-diff ones. This one is not, so I am making it separately.

Question: with this change, more VM will be made, is that a concern? Answer: the method in question is rather cold (not discernible on a profile taken from replaying libraries.pmi), the new VM calls should be of the quick variety, and additionally, there are many, many more similar VM calls made inside VNApplySelectors/Assign, their cost will dwarf anything added here (I actually have an experiment that caches some of the info on the FldSeq nodes, it shows 0.1% PIN improvements under SPMI, but it is probably not worth it in the VM scenario as it also increases the memory consumption by about the same margin).

Diffs: win-x64.

Part of #58312.

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

[SingleAccretion]

@dotnet/jit-contrib

[JulieLeeMSFT]

@jakobbotsch for the VN related PR.

[jakobbotsch]

It's weird to me that we are giving the maps themselves any meaningful type in the first place given that the actual type of a map is outside the simple JIT type system. Ideally, they would be typed as something like VN -> field_type, but I understand that we have to compromise.
Would it be possible to give them no type (e.g. TYP_UNDEF) and fix the sites that are relying on the type being meaningful instead? For instance

// The final field in the sequence will need to match the 'indType'
var_types indType = lhs->TypeGet();
ValueNum  fldMapVN =
    vnStore->VNApplySelectors(VNK_Liberal, fgCurMemoryVN[GcHeap], firstFieldOnly);

// The type of the field is "struct" if there are more fields in the sequence,
// otherwise it is the type returned from VNApplySelectors above.
-var_types firstFieldType = vnStore->TypeOfVN(fldMapVN);
+var_types firstFieldType = JITtype2varType(info.compCompHnd->getFieldType(firstFieldOnly->GetFieldHandle()));

in the case you linked. Would it be too expensive?

[jakobbotsch]

Otherwise I'm ok with the retyping, but would like to see some comments added here and there that the type of a map is the type of the codomain.

[SingleAccretion]

Would it be possible to give them no type (e.g. TYP_UNDEF) and fix the sites that are relying on the type being meaningful instead?

Yes, it would be, that's what we've been doing already with TYP_REF effectively being this placeholder type. I don't think we would need more VM calls with it, because the placeholder types would be "unused", and all the VNs representing values would be typed appropriately.

The reason I wanted to type the maps is that I eventually want to add two asserts:

1. TypeOf(MapSelect) == Type implied by the selector.
2. TypeOf(MapStore.Value) == Type implied by the selector.

So that we guard against things like MapStore(struct map, int field, float value), which today go unnoticed.

And it appeared to me that the only way to do this is to give the primary maps these "fake" types that match the types that selectors nominally point to, field types. However, now that I have again thought about it, there should be a way to discover this information without the retyping, by examining the selector according to our model. E. g. for fields:

1. If the selector is an instance field handle => type must be a placeholder one, this is a "first field map".
2. If the selector is of TYP_REF => we are selecting from a "first field map", type must match that of the field (and the map being selected from must be a MapStore/MapSelect).
3. If the selector is a struct field handle => we are selecting a value, type must match the field's type.

And similar for arrays, statics and struct locals.

So I think I will shelve this change and pursue the "placeholder type" design, as you have suggested.

All that said, there is still a useful fix here - we need to type statics properly. Will update accordingly.

[jakobbotsch]

That sounds very good to me.

[jakobbotsch]

LGTM. You were planning to fix the code around

runtime/src/coreclr/jit/valuenum.cpp

Lines 8259 to 8261 in 972891f

	// The type of the field is "struct" if there are more fields in the sequence,
	// otherwise it is the type returned from VNApplySelectors above.
	var_types firstFieldType = vnStore->TypeOfVN(fldMapVN);

in a separate change, is that right?

[SingleAccretion]

is that right?

Yes.

[jakobbotsch]

Sounds good, thanks for another contribution!