From bb3bdf8a4779ef3006755395a21b279abf2b9cc6 Mon Sep 17 00:00:00 2001 From: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> Date: Mon, 15 Nov 2021 18:24:55 +0300 Subject: [PATCH] Rewrite selection for fields (#61370) The issue was that VNApplySelectors uses the types of fields when selecting, but that's not valid for "the first field" maps. Mirror how the stores to fields are numbered. --- src/coreclr/jit/valuenum.cpp | 114 ++++++++++++++++++++----------------------- src/coreclr/jit/valuenum.h | 4 +- 2 files changed, 55 insertions(+), 63 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index a09e2f8..d1f108d 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2531,23 +2531,32 @@ TailCall: // 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 +// fieldHnd - handle of the field in question +// pFieldType - [out] parameter for the field's type +// pStructSize - optional [out] parameter for the size of the struct, +// populated if the field in question is of a struct type, +// otherwise set to zero // // Return Value: // Value number corresponding to the given field handle. // -ValueNum ValueNumStore::VNForFieldSelector(CORINFO_FIELD_HANDLE fieldHnd, - var_types* pFieldType, - CORINFO_CLASS_HANDLE* pStructHnd) +ValueNum ValueNumStore::VNForFieldSelector(CORINFO_FIELD_HANDLE fieldHnd, var_types* pFieldType, size_t* pStructSize) { - CORINFO_CLASS_HANDLE structHnd = NO_CLASS_HANDLE; - ValueNum fldHndVN = VNForHandle(ssize_t(fieldHnd), GTF_ICON_FIELD_HDL); + CORINFO_CLASS_HANDLE structHnd = NO_CLASS_HANDLE; + ValueNum fldHndVN = VNForHandle(ssize_t(fieldHnd), GTF_ICON_FIELD_HDL); + var_types fieldType = m_pComp->eeGetFieldType(fieldHnd, &structHnd); + size_t structSize = 0; - CorInfoType fieldCit = m_pComp->info.compCompHnd->getFieldType(fieldHnd, &structHnd); - var_types fieldType = JITtype2varType(fieldCit); + if (fieldType == TYP_STRUCT) + { + structSize = m_pComp->info.compCompHnd->getClassSize(structHnd); + + // We have to normalize here since there is no CorInfoType for vectors... + if (m_pComp->structSizeMightRepresentSIMDType(structSize)) + { + fieldType = m_pComp->impNormStructType(structHnd); + } + } #ifdef DEBUG if (m_pComp->verbose) @@ -2555,17 +2564,17 @@ ValueNum ValueNumStore::VNForFieldSelector(CORINFO_FIELD_HANDLE fieldHnd, 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)) + if (structSize != 0) { - printf(", size = %u", m_pComp->info.compCompHnd->getClassSize(structHnd)); + printf(", size = %u", structSize); } printf("\n"); } #endif - if (pStructHnd != nullptr) + if (pStructSize != nullptr) { - *pStructHnd = structHnd; + *pStructSize = structSize; } *pFieldType = fieldType; @@ -3890,22 +3899,10 @@ ValueNum ValueNumStore::VNApplySelectors(ValueNumKind vnk, assert(field != FieldSeqStore::NotAField()); JITDUMP(" VNApplySelectors:\n"); - var_types fieldType; - CORINFO_CLASS_HANDLE structHnd; - CORINFO_FIELD_HANDLE fldHnd = field->GetFieldHandle(); - ValueNum fldHndVN = VNForFieldSelector(fldHnd, &fieldType, &structHnd); + var_types fieldType; + size_t structSize; + ValueNum fldHndVN = VNForFieldSelector(field->GetFieldHandle(), &fieldType, &structSize); - size_t structSize = 0; - if (varTypeIsStruct(fieldType)) - { - structSize = m_pComp->info.compCompHnd->getClassSize(structHnd); - // We do not normalize the type field accesses during importation unless they - // are used in a call, return or assignment. - if ((fieldType == TYP_STRUCT) && m_pComp->structSizeMightRepresentSIMDType(structSize)) - { - fieldType = m_pComp->impNormStructType(structHnd); - } - } if (wbFinalStructSize != nullptr) { *wbFinalStructSize = structSize; @@ -9016,10 +9013,11 @@ void Compiler::fgValueNumberTree(GenTree* tree) } else if (fldSeq2 != nullptr) { - // Get the first (instance or static) field from field seq. GcHeap[field] will yield the "field - // map". - CLANG_FORMAT_COMMENT_ANCHOR; - + if (fldSeq2->IsFirstElemFieldSeq()) + { + fldSeq2 = fldSeq2->m_next; + assert(fldSeq2 != nullptr); + } #ifdef DEBUG CORINFO_CLASS_HANDLE fldCls = info.compCompHnd->getFieldClass(fldSeq2->m_fieldHnd); if (obj != nullptr) @@ -9031,42 +9029,38 @@ void Compiler::fgValueNumberTree(GenTree* tree) } #endif // DEBUG - // Get a field sequence for just the first field in the sequence - // - FieldSeqNode* firstFieldOnly = GetFieldSeqStore()->CreateSingleton(fldSeq2->m_fieldHnd); - size_t structSize = 0; - ValueNum fldMapVN = - vnStore->VNApplySelectors(VNK_Liberal, fgCurMemoryVN[GcHeap], firstFieldOnly, &structSize); + // The size of the ultimate value we will select, if it is of a struct type. + size_t structSize = 0; - // The final field in the sequence will need to match the 'indType' - var_types indType = tree->TypeGet(); + // Get the selector for the first field. + var_types firstFieldType; + ValueNum firstFieldSelectorVN = + vnStore->VNForFieldSelector(fldSeq2->GetFieldHandle(), &firstFieldType, &structSize); - // 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); + ValueNum fldMapVN = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fgCurMemoryVN[GcHeap], + firstFieldSelectorVN); - ValueNum valAtAddr = fldMapVN; + ValueNum firstFieldValueSelectorVN; if (obj != nullptr) { - // construct the ValueNumber for 'fldMap at obj' - ValueNum objNormVal = vnStore->VNLiberalNormalValue(obj->gtVNPair); - valAtAddr = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN, objNormVal); + firstFieldValueSelectorVN = vnStore->VNLiberalNormalValue(obj->gtVNPair); } - else if (staticOffset != nullptr) + else { - // construct the ValueNumber for 'fldMap at staticOffset' - ValueNum offsetNormVal = vnStore->VNLiberalNormalValue(staticOffset->gtVNPair); - valAtAddr = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN, offsetNormVal); + assert(staticOffset != nullptr); + firstFieldValueSelectorVN = vnStore->VNLiberalNormalValue(staticOffset->gtVNPair); } - // Now get rid of any remaining struct field dereferences. - if (fldSeq2->m_next) - { - valAtAddr = vnStore->VNApplySelectors(VNK_Liberal, valAtAddr, fldSeq2->m_next, &structSize); - } - valAtAddr = vnStore->VNApplySelectorsTypeCheck(valAtAddr, indType, structSize); + // Construct the value number for fldMap[obj/offset]. + ValueNum firstFieldValueVN = + vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN, firstFieldValueSelectorVN); + + // Finally, account for the rest of the fields in the sequence. + ValueNum valueVN = + vnStore->VNApplySelectors(VNK_Liberal, firstFieldValueVN, fldSeq2->m_next, &structSize); - tree->gtVNPair.SetLiberal(valAtAddr); + valueVN = vnStore->VNApplySelectorsTypeCheck(valueVN, tree->TypeGet(), structSize); + tree->gtVNPair.SetLiberal(valueVN); // The conservative value is a new, unique VN. tree->gtVNPair.SetConservative(vnStore->VNForExpr(compCurBB, tree->TypeGet())); diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 40ab7b0..b1e840c 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -591,9 +591,7 @@ public: // 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); + ValueNum VNForFieldSelector(CORINFO_FIELD_HANDLE fieldHnd, var_types* pFieldType, size_t* pStructSize = 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 -- 2.7.4