From a6c7492c1e20178cacad2a2fb0770747e157f474 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 29 Oct 2021 22:14:51 +0300 Subject: [PATCH 1/4] Fix value numbering of field assignments 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. --- src/coreclr/jit/valuenum.cpp | 201 +++++++++++++++++------------------ src/coreclr/jit/valuenum.h | 4 + 2 files changed, 103 insertions(+), 102 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 8d255447da77e..998fe886a6c27 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2495,6 +2495,52 @@ ValueNum ValueNumStore::VNForMapSelectWork( } } +//------------------------------------------------------------------------ +// VNForFieldSelector: A specialized version (with logging) of VNForHandle +// that is used for field handle selectors. +// +// Arguments: +// fieldHnd - handle of the field in question +// pFieldType - [out] parameter for the field's type +// pStructHnd - optional [out] parameter for the struct handle, +// populated if the field is of a struct type +// +// Return Value: +// Value number corresponding to the given field handle. +// +ValueNum ValueNumStore::VNForFieldSelector(CORINFO_FIELD_HANDLE fieldHnd, + var_types* pFieldType, + CORINFO_CLASS_HANDLE* pStructHnd) +{ + CORINFO_CLASS_HANDLE structHnd = NO_CLASS_HANDLE; + ValueNum fldHndVN = VNForHandle(ssize_t(fieldHnd), GTF_ICON_FIELD_HDL); + + CorInfoType fieldCit = m_pComp->info.compCompHnd->getFieldType(fieldHnd, &structHnd); + var_types fieldType = JITtype2varType(fieldCit); + +#ifdef DEBUG + if (m_pComp->verbose) + { + const char* modName; + const char* fldName = m_pComp->eeGetFieldName(fieldHnd, &modName); + printf(" VNForHandle(%s) is " FMT_VN ", fieldType is %s", fldName, fldHndVN, varTypeName(fieldType)); + if (varTypeIsStruct(fieldType)) + { + printf(", size = %u", m_pComp->info.compCompHnd->getClassSize(structHnd)); + } + printf("\n"); + } +#endif + + if (pStructHnd != nullptr) + { + *pStructHnd = structHnd; + } + *pFieldType = fieldType; + + return fldHndVN; +} + ValueNum ValueNumStore::EvalFuncForConstantArgs(var_types typ, VNFunc func, ValueNum arg0VN) { assert(CanEvalForConstantArgs(func)); @@ -3812,12 +3858,11 @@ ValueNum ValueNumStore::VNApplySelectors(ValueNumKind vnk, assert(field != FieldSeqStore::NotAField()); - CORINFO_FIELD_HANDLE fldHnd = field->m_fieldHnd; - CORINFO_CLASS_HANDLE structHnd = NO_CLASS_HANDLE; - ValueNum fldHndVN = VNForHandle(ssize_t(fldHnd), GTF_ICON_FIELD_HDL); - noway_assert(fldHnd != nullptr); - CorInfoType fieldCit = m_pComp->info.compCompHnd->getFieldType(fldHnd, &structHnd); - var_types fieldType = JITtype2varType(fieldCit); + JITDUMP(" VNApplySelectors:\n"); + var_types fieldType; + CORINFO_CLASS_HANDLE structHnd; + CORINFO_FIELD_HANDLE fldHnd = field->GetFieldHandle(); + ValueNum fldHndVN = VNForFieldSelector(fldHnd, &fieldType, &structHnd); size_t structSize = 0; if (varTypeIsStruct(fieldType)) @@ -3835,21 +3880,6 @@ ValueNum ValueNumStore::VNApplySelectors(ValueNumKind vnk, *wbFinalStructSize = structSize; } -#ifdef DEBUG - if (m_pComp->verbose) - { - printf(" VNApplySelectors:\n"); - const char* modName; - const char* fldName = m_pComp->eeGetFieldName(fldHnd, &modName); - printf(" VNForHandle(%s) is " FMT_VN ", fieldType is %s", fldName, fldHndVN, varTypeName(fieldType)); - if (varTypeIsStruct(fieldType)) - { - printf(", size = %d", structSize); - } - printf("\n"); - } -#endif - map = VNForMapSelect(vnk, fieldType, map, fldHndVN); } @@ -4005,48 +4035,27 @@ ValueNum ValueNumStore::VNApplySelectorsAssign( return VNApplySelectorsAssign(vnk, map, fieldSeq->m_next, value, dstIndType); } + if (fieldSeq->m_next == nullptr) + { + JITDUMP(" VNApplySelectorsAssign:\n"); + } + // Otherwise, fldHnd is a real field handle. - CORINFO_FIELD_HANDLE fldHnd = fieldSeq->m_fieldHnd; - ValueNum fldHndVN = VNForHandle(ssize_t(fldHnd), GTF_ICON_FIELD_HDL); - noway_assert(fldHnd != nullptr); - CorInfoType fieldCit = m_pComp->info.compCompHnd->getFieldType(fldHnd); - var_types fieldType = JITtype2varType(fieldCit); + var_types fieldType; + ValueNum fldHndVN = VNForFieldSelector(fieldSeq->GetFieldHandle(), &fieldType); ValueNum valueAfter; - if (fieldSeq->m_next) + if (fieldSeq->m_next != nullptr) { -#ifdef DEBUG - if (m_pComp->verbose) - { - const char* modName; - const char* fldName = m_pComp->eeGetFieldName(fldHnd, &modName); - printf(" VNForHandle(%s) is " FMT_VN ", fieldType is %s\n", fldName, fldHndVN, - varTypeName(fieldType)); - } -#endif ValueNum fseqMap = VNForMapSelect(vnk, fieldType, map, fldHndVN); valueAfter = VNApplySelectorsAssign(vnk, fseqMap, fieldSeq->m_next, value, dstIndType); } else { -#ifdef DEBUG - if (m_pComp->verbose) - { - if (fieldSeq->m_next == nullptr) - { - printf(" VNApplySelectorsAssign:\n"); - } - const char* modName; - const char* fldName = m_pComp->eeGetFieldName(fldHnd, &modName); - printf(" VNForHandle(%s) is " FMT_VN ", fieldType is %s\n", fldName, fldHndVN, - varTypeName(fieldType)); - } -#endif valueAfter = VNApplySelectorsAssignTypeCoerce(value, dstIndType); } - ValueNum newMap = VNForMapStore(fieldType, map, fldHndVN, valueAfter); - return newMap; + return VNForMapStore(fieldType, map, fldHndVN, valueAfter); } } @@ -7768,82 +7777,70 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree) assert(fldSeq != nullptr); } - // Get a field sequence for just the first field in the sequence - // - FieldSeqNode* firstFieldOnly = GetFieldSeqStore()->CreateSingleton(fldSeq->m_fieldHnd); + // The value number from the rhs of the assignment + ValueNum storeVal = rhsVNPair.GetLiberal(); + ValueNum newHeapVN = ValueNumStore::NoVN; - // The final field in the sequence will need to match the 'indType' + // We will check that the final field in the sequence matches '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); - - // The value number from the rhs of the assignment - ValueNum storeVal = rhsVNPair.GetLiberal(); - ValueNum newFldMapVN = ValueNumStore::NoVN; // when (obj != nullptr) we have an instance field, otherwise a static field // when (staticOffset != nullptr) it represents a offset into a static or the call to // Shared Static Base if ((obj != nullptr) || (staticOffset != nullptr)) { - ValueNum valAtAddr = fldMapVN; - ValueNum normVal = ValueNumStore::NoVN; + var_types firstFieldType; + ValueNum firstFieldSelectorVN = vnStore->VNForFieldSelector(fldSeq->GetFieldHandle(), + &firstFieldType); + + // Construct the "field map" VN. It represents memory state of the first field + // of all objects on the heap. This is our primary map. + ValueNum fldMapVN = vnStore->VNForMapSelect(VNK_Liberal, TYP_REF, fgCurMemoryVN[GcHeap], + firstFieldSelectorVN); + ValueNum firstFieldValueSelectorVN = ValueNumStore::NoVN; if (obj != nullptr) { - // Unpack, Norm,Exc for 'obj' - ValueNum vnObjExcSet; - vnStore->VNUnpackExc(obj->gtVNPair.GetLiberal(), &normVal, &vnObjExcSet); - vnExcSet = vnStore->VNExcSetUnion(vnExcSet, vnObjExcSet); - - // construct the ValueNumber for 'fldMap at obj' - valAtAddr = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN, normVal); + firstFieldValueSelectorVN = vnStore->VNLiberalNormalValue(obj->gtVNPair); } else // (staticOffset != nullptr) { - // construct the ValueNumber for 'fldMap at staticOffset' - normVal = vnStore->VNLiberalNormalValue(staticOffset->gtVNPair); - valAtAddr = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN, normVal); - } - // Now get rid of any remaining struct field dereferences. (if they exist) - if (fldSeq->m_next) - { - storeVal = vnStore->VNApplySelectorsAssign(VNK_Liberal, valAtAddr, fldSeq->m_next, - storeVal, indType); + firstFieldValueSelectorVN = vnStore->VNLiberalNormalValue(staticOffset->gtVNPair); } - // From which we can construct the new ValueNumber for 'fldMap at normVal' - newFldMapVN = - vnStore->VNForMapStore(vnStore->TypeOfVN(fldMapVN), fldMapVN, normVal, storeVal); + // Construct the ValueNumber for fldMap[obj/offset]. This + // map represents the specific field we're looking to store to. + ValueNum firstFieldValueVN = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN, + firstFieldValueSelectorVN); + + // Now construct the maps updating the rest of the + // fields in the sequence (of which there could be none). + ValueNum newFirstFieldValueVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, + firstFieldValueVN, + fldSeq->m_next, + storeVal, indType); + + // Finally, construct the new field map... + ValueNum newFldMapVN = vnStore->VNForMapStore(vnStore->TypeOfVN(fldMapVN), fldMapVN, + firstFieldValueSelectorVN, + newFirstFieldValueVN); + // ...and a new value for the heap. + newHeapVN = vnStore->VNForMapStore(TYP_REF, fgCurMemoryVN[GcHeap], + firstFieldSelectorVN, newFldMapVN); } else { - // plain static field - - // Now get rid of any remaining struct field dereferences. (if they exist) - if (fldSeq->m_next) - { - storeVal = vnStore->VNApplySelectorsAssign(VNK_Liberal, fldMapVN, fldSeq->m_next, - storeVal, indType); - } - - newFldMapVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, fgCurMemoryVN[GcHeap], fldSeq, - storeVal, indType); + // Plain static field. + newHeapVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, fgCurMemoryVN[GcHeap], + fldSeq, storeVal, indType); } // It is not strictly necessary to set the lhs value number, // but the dumps read better with it set to the 'storeVal' that we just computed lhs->gtVNPair.SetBoth(storeVal); - // Update the field map for firstField in GcHeap to this new value. - ValueNum heapVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, fgCurMemoryVN[GcHeap], - firstFieldOnly, newFldMapVN, indType); - - recordGcHeapStore(tree, heapVN DEBUGARG("StoreField")); + // Update the GcHeap value. + recordGcHeapStore(tree, newHeapVN DEBUGARG("StoreField")); } } else diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index afb9510090e39..55bdeee7b8b94 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -590,6 +590,10 @@ class ValueNumStore // A specialized version of VNForFunc that is used for VNF_MapStore and provides some logging when verbose is set ValueNum VNForMapStore(var_types type, ValueNum map, ValueNum index, ValueNum value); + ValueNum VNForFieldSelector(CORINFO_FIELD_HANDLE fieldHnd, + var_types* pFieldType, + CORINFO_CLASS_HANDLE* pStructHnd = nullptr); + // These functions parallel the ones above, except that they take liberal/conservative VN pairs // as arguments, and return such a pair (the pair of the function applied to the liberal args, and // the function applied to the conservative args). From 0a4c6aebebe56130b8cdc1d7b883a5cdd02a99e0 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Tue, 2 Nov 2021 21:46:58 +0300 Subject: [PATCH 2/4] Add an optimization It was confusing in the dumps to look at unused results of VNForMapSelect, so just avoid looking for the first field's value if we do not need it. --- src/coreclr/jit/valuenum.cpp | 32 +++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 998fe886a6c27..82f868ef1a48d 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -7808,22 +7808,32 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree) firstFieldValueSelectorVN = vnStore->VNLiberalNormalValue(staticOffset->gtVNPair); } - // Construct the ValueNumber for fldMap[obj/offset]. This - // map represents the specific field we're looking to store to. - ValueNum firstFieldValueVN = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN, - firstFieldValueSelectorVN); - - // Now construct the maps updating the rest of the - // fields in the sequence (of which there could be none). - ValueNum newFirstFieldValueVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, - firstFieldValueVN, - fldSeq->m_next, - storeVal, indType); + ValueNum newFirstFieldValueVN = ValueNumStore::NoVN; + // Optimization: avoid traversting the maps for the value of the first field if + // we do not need it, which is the case if the rest of the field sequence is empty. + if (fldSeq->m_next == nullptr) + { + newFirstFieldValueVN = vnStore->VNApplySelectorsAssignTypeCoerce(storeVal, indType); + } + else + { + // Construct the ValueNumber for fldMap[obj/offset]. This (struct) + // map represents the specific field we're looking to store to. + ValueNum firstFieldValueVN = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, + fldMapVN, + firstFieldValueSelectorVN); + + // Construct the maps updating the rest of the fields in the sequence. + newFirstFieldValueVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, firstFieldValueVN, + fldSeq->m_next, storeVal, + indType); + } // Finally, construct the new field map... ValueNum newFldMapVN = vnStore->VNForMapStore(vnStore->TypeOfVN(fldMapVN), fldMapVN, firstFieldValueSelectorVN, newFirstFieldValueVN); + // ...and a new value for the heap. newHeapVN = vnStore->VNForMapStore(TYP_REF, fgCurMemoryVN[GcHeap], firstFieldSelectorVN, newFldMapVN); From 3744a29cc7328206cf293644f00a2106ace45784 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Tue, 2 Nov 2021 22:54:11 +0300 Subject: [PATCH 3/4] Fix formatting --- src/coreclr/jit/valuenum.cpp | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 82f868ef1a48d..1468fd1b11e4e 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -7790,8 +7790,8 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree) if ((obj != nullptr) || (staticOffset != nullptr)) { var_types firstFieldType; - ValueNum firstFieldSelectorVN = vnStore->VNForFieldSelector(fldSeq->GetFieldHandle(), - &firstFieldType); + ValueNum firstFieldSelectorVN = + vnStore->VNForFieldSelector(fldSeq->GetFieldHandle(), &firstFieldType); // Construct the "field map" VN. It represents memory state of the first field // of all objects on the heap. This is our primary map. @@ -7819,30 +7819,30 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree) { // Construct the ValueNumber for fldMap[obj/offset]. This (struct) // map represents the specific field we're looking to store to. - ValueNum firstFieldValueVN = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, - fldMapVN, - firstFieldValueSelectorVN); + ValueNum firstFieldValueVN = + vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN, + firstFieldValueSelectorVN); // Construct the maps updating the rest of the fields in the sequence. - newFirstFieldValueVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, firstFieldValueVN, - fldSeq->m_next, storeVal, - indType); + newFirstFieldValueVN = + vnStore->VNApplySelectorsAssign(VNK_Liberal, firstFieldValueVN, fldSeq->m_next, + storeVal, indType); } // Finally, construct the new field map... - ValueNum newFldMapVN = vnStore->VNForMapStore(vnStore->TypeOfVN(fldMapVN), fldMapVN, - firstFieldValueSelectorVN, - newFirstFieldValueVN); + ValueNum newFldMapVN = + vnStore->VNForMapStore(vnStore->TypeOfVN(fldMapVN), fldMapVN, firstFieldValueSelectorVN, + newFirstFieldValueVN); // ...and a new value for the heap. - newHeapVN = vnStore->VNForMapStore(TYP_REF, fgCurMemoryVN[GcHeap], - firstFieldSelectorVN, newFldMapVN); + newHeapVN = vnStore->VNForMapStore(TYP_REF, fgCurMemoryVN[GcHeap], firstFieldSelectorVN, + newFldMapVN); } else { // Plain static field. - newHeapVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, fgCurMemoryVN[GcHeap], - fldSeq, storeVal, indType); + newHeapVN = vnStore->VNApplySelectorsAssign(VNK_Liberal, fgCurMemoryVN[GcHeap], fldSeq, + storeVal, indType); } // It is not strictly necessary to set the lhs value number, From 352acf46d2283bffd069c381f3ca269b805962d1 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Tue, 2 Nov 2021 23:44:02 +0300 Subject: [PATCH 4/4] Go back to typing the "first field map" as a field 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. --- src/coreclr/jit/valuenum.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 1468fd1b11e4e..c8edfdfb8fe9d 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -7795,8 +7795,8 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree) // Construct the "field map" VN. It represents memory state of the first field // of all objects on the heap. This is our primary map. - ValueNum fldMapVN = vnStore->VNForMapSelect(VNK_Liberal, TYP_REF, fgCurMemoryVN[GcHeap], - firstFieldSelectorVN); + ValueNum fldMapVN = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, + fgCurMemoryVN[GcHeap], firstFieldSelectorVN); ValueNum firstFieldValueSelectorVN = ValueNumStore::NoVN; if (obj != nullptr)