Skip to content

Commit 39ece73

Browse files
SingleAccretionSingleAccretion
authored
Fix value numbering of field assignments (#61113)
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.
1 parent ff361bd commit 39ece73

File tree

2 files changed

+111
-100
lines changed

2 files changed

+111
-100
lines changed

‎src/coreclr/jit/valuenum.cpp‎

Lines changed: 107 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -2495,6 +2495,52 @@ ValueNum ValueNumStore::VNForMapSelectWork(
24952495
}
24962496
}
24972497

2498+
//------------------------------------------------------------------------
2499+
// VNForFieldSelector: A specialized version (with logging) of VNForHandle
2500+
// that is used for field handle selectors.
2501+
//
2502+
// Arguments:
2503+
// fieldHnd - handle of the field in question
2504+
// pFieldType - [out] parameter for the field's type
2505+
// pStructHnd - optional [out] parameter for the struct handle,
2506+
// populated if the field is of a struct type
2507+
//
2508+
// Return Value:
2509+
// Value number corresponding to the given field handle.
2510+
//
2511+
ValueNum ValueNumStore::VNForFieldSelector(CORINFO_FIELD_HANDLE fieldHnd,
2512+
var_types* pFieldType,
2513+
CORINFO_CLASS_HANDLE* pStructHnd)
2514+
{
2515+
CORINFO_CLASS_HANDLE structHnd = NO_CLASS_HANDLE;
2516+
ValueNum fldHndVN = VNForHandle(ssize_t(fieldHnd), GTF_ICON_FIELD_HDL);
2517+
2518+
CorInfoType fieldCit = m_pComp->info.compCompHnd->getFieldType(fieldHnd, &structHnd);
2519+
var_types fieldType = JITtype2varType(fieldCit);
2520+
2521+
#ifdef DEBUG
2522+
if (m_pComp->verbose)
2523+
{
2524+
const char* modName;
2525+
const char* fldName = m_pComp->eeGetFieldName(fieldHnd, &modName);
2526+
printf(" VNForHandle(%s) is " FMT_VN ", fieldType is %s", fldName, fldHndVN, varTypeName(fieldType));
2527+
if (varTypeIsStruct(fieldType))
2528+
{
2529+
printf(", size = %u", m_pComp->info.compCompHnd->getClassSize(structHnd));
2530+
}
2531+
printf("\n");
2532+
}
2533+
#endif
2534+
2535+
if (pStructHnd != nullptr)
2536+
{
2537+
*pStructHnd = structHnd;
2538+
}
2539+
*pFieldType = fieldType;
2540+
2541+
return fldHndVN;
2542+
}
2543+
24982544
ValueNum ValueNumStore::EvalFuncForConstantArgs(var_types typ, VNFunc func, ValueNum arg0VN)
24992545
{
25002546
assert(CanEvalForConstantArgs(func));
@@ -3812,12 +3858,11 @@ ValueNum ValueNumStore::VNApplySelectors(ValueNumKind vnk,
38123858

38133859
assert(field != FieldSeqStore::NotAField());
38143860

3815-
CORINFO_FIELD_HANDLE fldHnd = field->m_fieldHnd;
3816-
CORINFO_CLASS_HANDLE structHnd = NO_CLASS_HANDLE;
3817-
ValueNum fldHndVN = VNForHandle(ssize_t(fldHnd), GTF_ICON_FIELD_HDL);
3818-
noway_assert(fldHnd != nullptr);
3819-
CorInfoType fieldCit = m_pComp->info.compCompHnd->getFieldType(fldHnd, &structHnd);
3820-
var_types fieldType = JITtype2varType(fieldCit);
3861+
JITDUMP(" VNApplySelectors:\n");
3862+
var_types fieldType;
3863+
CORINFO_CLASS_HANDLE structHnd;
3864+
CORINFO_FIELD_HANDLE fldHnd = field->GetFieldHandle();
3865+
ValueNum fldHndVN = VNForFieldSelector(fldHnd, &fieldType, &structHnd);
38213866

38223867
size_t structSize = 0;
38233868
if (varTypeIsStruct(fieldType))
@@ -3835,21 +3880,6 @@ ValueNum ValueNumStore::VNApplySelectors(ValueNumKind vnk,
38353880
*wbFinalStructSize = structSize;
38363881
}
38373882

3838-
#ifdef DEBUG
3839-
if (m_pComp->verbose)
3840-
{
3841-
printf(" VNApplySelectors:\n");
3842-
const char* modName;
3843-
const char* fldName = m_pComp->eeGetFieldName(fldHnd, &modName);
3844-
printf(" VNForHandle(%s) is " FMT_VN ", fieldType is %s", fldName, fldHndVN, varTypeName(fieldType));
3845-
if (varTypeIsStruct(fieldType))
3846-
{
3847-
printf(", size = %d", structSize);
3848-
}
3849-
printf("\n");
3850-
}
3851-
#endif
3852-
38533883
map = VNForMapSelect(vnk, fieldType, map, fldHndVN);
38543884
}
38553885

@@ -4005,48 +4035,27 @@ ValueNum ValueNumStore::VNApplySelectorsAssign(
40054035
return VNApplySelectorsAssign(vnk, map, fieldSeq->m_next, value, dstIndType);
40064036
}
40074037

4038+
if (fieldSeq->m_next == nullptr)
4039+
{
4040+
JITDUMP(" VNApplySelectorsAssign:\n");
4041+
}
4042+
40084043
// Otherwise, fldHnd is a real field handle.
4009-
CORINFO_FIELD_HANDLE fldHnd = fieldSeq->m_fieldHnd;
4010-
ValueNum fldHndVN = VNForHandle(ssize_t(fldHnd), GTF_ICON_FIELD_HDL);
4011-
noway_assert(fldHnd != nullptr);
4012-
CorInfoType fieldCit = m_pComp->info.compCompHnd->getFieldType(fldHnd);
4013-
var_types fieldType = JITtype2varType(fieldCit);
4044+
var_types fieldType;
4045+
ValueNum fldHndVN = VNForFieldSelector(fieldSeq->GetFieldHandle(), &fieldType);
40144046

40154047
ValueNum valueAfter;
4016-
if (fieldSeq->m_next)
4048+
if (fieldSeq->m_next != nullptr)
40174049
{
4018-
#ifdef DEBUG
4019-
if (m_pComp->verbose)
4020-
{
4021-
const char* modName;
4022-
const char* fldName = m_pComp->eeGetFieldName(fldHnd, &modName);
4023-
printf(" VNForHandle(%s) is " FMT_VN ", fieldType is %s\n", fldName, fldHndVN,
4024-
varTypeName(fieldType));
4025-
}
4026-
#endif
40274050
ValueNum fseqMap = VNForMapSelect(vnk, fieldType, map, fldHndVN);
40284051
valueAfter = VNApplySelectorsAssign(vnk, fseqMap, fieldSeq->m_next, value, dstIndType);
40294052
}
40304053
else
40314054
{
4032-
#ifdef DEBUG
4033-
if (m_pComp->verbose)
4034-
{
4035-
if (fieldSeq->m_next == nullptr)
4036-
{
4037-
printf(" VNApplySelectorsAssign:\n");
4038-
}
4039-
const char* modName;
4040-
const char* fldName = m_pComp->eeGetFieldName(fldHnd, &modName);
4041-
printf(" VNForHandle(%s) is " FMT_VN ", fieldType is %s\n", fldName, fldHndVN,
4042-
varTypeName(fieldType));
4043-
}
4044-
#endif
40454055
valueAfter = VNApplySelectorsAssignTypeCoerce(value, dstIndType);
40464056
}
40474057

4048-
ValueNum newMap = VNForMapStore(fieldType, map, fldHndVN, valueAfter);
4049-
return newMap;
4058+
return VNForMapStore(fieldType, map, fldHndVN, valueAfter);
40504059
}
40514060
}
40524061

@@ -7768,82 +7777,80 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree)
77687777
assert(fldSeq != nullptr);
77697778
}
77707779

7771-
// Get a field sequence for just the first field in the sequence
7772-
//
7773-
FieldSeqNode* firstFieldOnly = GetFieldSeqStore()->CreateSingleton(fldSeq->m_fieldHnd);
7780+
// The value number from the rhs of the assignment
7781+
ValueNum storeVal = rhsVNPair.GetLiberal();
7782+
ValueNum newHeapVN = ValueNumStore::NoVN;
77747783

7775-
// The final field in the sequence will need to match the 'indType'
7784+
// We will check that the final field in the sequence matches 'indType'.
77767785
var_types indType = lhs->TypeGet();
7777-
ValueNum fldMapVN =
7778-
vnStore->VNApplySelectors(VNK_Liberal, fgCurMemoryVN[GcHeap], firstFieldOnly);
7779-
7780-
// The type of the field is "struct" if there are more fields in the sequence,
7781-
// otherwise it is the type returned from VNApplySelectors above.
7782-
var_types firstFieldType = vnStore->TypeOfVN(fldMapVN);
7783-
7784-
// The value number from the rhs of the assignment
7785-
ValueNum storeVal = rhsVNPair.GetLiberal();
7786-
ValueNum newFldMapVN = ValueNumStore::NoVN;
77877786

77887787
// when (obj != nullptr) we have an instance field, otherwise a static field
77897788
// when (staticOffset != nullptr) it represents a offset into a static or the call to
77907789
// Shared Static Base
77917790
if ((obj != nullptr) || (staticOffset != nullptr))
77927791
{
7793-
ValueNum valAtAddr = fldMapVN;
7794-
ValueNum normVal = ValueNumStore::NoVN;
7792+
var_types firstFieldType;
7793+
ValueNum firstFieldSelectorVN =
7794+
vnStore->VNForFieldSelector(fldSeq->GetFieldHandle(), &firstFieldType);
77957795

7796+
// Construct the "field map" VN. It represents memory state of the first field
7797+
// of all objects on the heap. This is our primary map.
7798+
ValueNum fldMapVN = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType,
7799+
fgCurMemoryVN[GcHeap], firstFieldSelectorVN);
7800+
7801+
ValueNum firstFieldValueSelectorVN = ValueNumStore::NoVN;
77967802
if (obj != nullptr)
77977803
{
7798-
// Unpack, Norm,Exc for 'obj'
7799-
ValueNum vnObjExcSet;
7800-
vnStore->VNUnpackExc(obj->gtVNPair.GetLiberal(), &normVal, &vnObjExcSet);
7801-
vnExcSet = vnStore->VNExcSetUnion(vnExcSet, vnObjExcSet);
7802-
7803-
// construct the ValueNumber for 'fldMap at obj'
7804-
valAtAddr = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN, normVal);
7804+
firstFieldValueSelectorVN = vnStore->VNLiberalNormalValue(obj->gtVNPair);
78057805
}
78067806
else // (staticOffset != nullptr)
78077807
{
7808-
// construct the ValueNumber for 'fldMap at staticOffset'
7809-
normVal = vnStore->VNLiberalNormalValue(staticOffset->gtVNPair);
7810-
valAtAddr = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN, normVal);
7808+
firstFieldValueSelectorVN = vnStore->VNLiberalNormalValue(staticOffset->gtVNPair);
78117809
}
7812-
// Now get rid of any remaining struct field dereferences. (if they exist)
7813-
if (fldSeq->m_next)
7810+
7811+
ValueNum newFirstFieldValueVN = ValueNumStore::NoVN;
7812+
// Optimization: avoid traversting the maps for the value of the first field if
7813+
// we do not need it, which is the case if the rest of the field sequence is empty.
7814+
if (fldSeq->m_next == nullptr)
78147815
{
7815-
storeVal = vnStore->VNApplySelectorsAssign(VNK_Liberal, valAtAddr, fldSeq->m_next,
7816-
storeVal, indType);
7816+
newFirstFieldValueVN = vnStore->VNApplySelectorsAssignTypeCoerce(storeVal, indType);
7817+
}
7818+
else
7819+
{
7820+
// Construct the ValueNumber for fldMap[obj/offset]. This (struct)
7821+
// map represents the specific field we're looking to store to.
7822+
ValueNum firstFieldValueVN =
7823+
vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN,
7824+
firstFieldValueSelectorVN);
7825+
7826+
// Construct the maps updating the rest of the fields in the sequence.
7827+
newFirstFieldValueVN =
7828+
vnStore->VNApplySelectorsAssign(VNK_Liberal, firstFieldValueVN, fldSeq->m_next,
7829+
storeVal, indType);
78177830
}
78187831

7819-
// From which we can construct the new ValueNumber for 'fldMap at normVal'
7820-
newFldMapVN =
7821-
vnStore->VNForMapStore(vnStore->TypeOfVN(fldMapVN), fldMapVN, normVal, storeVal);
7832+
// Finally, construct the new field map...
7833+
ValueNum newFldMapVN =
7834+
vnStore->VNForMapStore(vnStore->TypeOfVN(fldMapVN), fldMapVN, firstFieldValueSelectorVN,
7835+
newFirstFieldValueVN);
7836+
7837+
// ...and a new value for the heap.
7838+
newHeapVN = vnStore->VNForMapStore(TYP_REF, fgCurMemoryVN[GcHeap], firstFieldSelectorVN,
7839+
newFldMapVN);
78227840
}
78237841
else
78247842
{
7825-
// plain static field
7826-
7827-
// Now get rid of any remaining struct field dereferences. (if they exist)
7828-
if (fldSeq->m_next)
7829-
{
7830-
storeVal = vnStore->VNApplySelectorsAssign(VNK_Liberal, fldMapVN, fldSeq->m_next,
7831-
storeVal, indType);
7832-
}
7833-
7834-
newFldMapVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, fgCurMemoryVN[GcHeap], fldSeq,
7835-
storeVal, indType);
7843+
// Plain static field.
7844+
newHeapVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, fgCurMemoryVN[GcHeap], fldSeq,
7845+
storeVal, indType);
78367846
}
78377847

78387848
// It is not strictly necessary to set the lhs value number,
78397849
// but the dumps read better with it set to the 'storeVal' that we just computed
78407850
lhs->gtVNPair.SetBoth(storeVal);
78417851

7842-
// Update the field map for firstField in GcHeap to this new value.
7843-
ValueNum heapVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, fgCurMemoryVN[GcHeap],
7844-
firstFieldOnly, newFldMapVN, indType);
7845-
7846-
recordGcHeapStore(tree, heapVN DEBUGARG("StoreField"));
7852+
// Update the GcHeap value.
7853+
recordGcHeapStore(tree, newHeapVN DEBUGARG("StoreField"));
78477854
}
78487855
}
78497856
else

‎src/coreclr/jit/valuenum.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,10 @@ class ValueNumStore
590590
// A specialized version of VNForFunc that is used for VNF_MapStore and provides some logging when verbose is set
591591
ValueNum VNForMapStore(var_types type, ValueNum map, ValueNum index, ValueNum value);
592592

593+
ValueNum VNForFieldSelector(CORINFO_FIELD_HANDLE fieldHnd,
594+
var_types* pFieldType,
595+
CORINFO_CLASS_HANDLE* pStructHnd = nullptr);
596+
593597
// These functions parallel the ones above, except that they take liberal/conservative VN pairs
594598
// as arguments, and return such a pair (the pair of the function applied to the liberal args, and
595599
// the function applied to the conservative args).

0 commit comments

Comments
 (0)