From a6e0f250df68212d94887902c8799b16ee8d2c6b Mon Sep 17 00:00:00 2001 From: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> Date: Mon, 22 Nov 2021 21:07:11 +0300 Subject: [PATCH] Do not explicitly pass type to `VNForMapStore` (#61882) * Fix type checks in fgValueNumberArrIndexAssign The code was trying to obtain the value of the location to store to from the updated map for the element. This is simply incorrect for all but the trivial case of an empty field sequence. * Do not explicitly pass type to VNForMapStore It must always be equal to the type of the map being updated, not having redundancy eliminates the possibility for mistakes. --- src/coreclr/jit/valuenum.cpp | 41 ++++++++++++++---------------------- src/coreclr/jit/valuenum.h | 2 +- 2 files changed, 17 insertions(+), 26 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index d0a1132540bc5..581ec7c242874 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -2200,7 +2200,6 @@ ValueNum ValueNumStore::VNForFunc( // // // Arguments: -// type - Type for the new map // map - Map value number // index - Index value number // value - New value for map[index] @@ -2208,17 +2207,17 @@ ValueNum ValueNumStore::VNForFunc( // Return Value: // Value number for "map" with "map[index]" set to "value". // -ValueNum ValueNumStore::VNForMapStore(var_types type, ValueNum map, ValueNum index, ValueNum value) +ValueNum ValueNumStore::VNForMapStore(ValueNum map, ValueNum index, ValueNum value) { BasicBlock* const bb = m_pComp->compCurBB; BasicBlock::loopNumber const loopNum = bb->bbNatLoopNum; - ValueNum const result = VNForFunc(type, VNF_MapStore, map, index, value, loopNum); + ValueNum const result = VNForFunc(TypeOfVN(map), VNF_MapStore, map, index, value, loopNum); #ifdef DEBUG if (m_pComp->verbose) { printf(" VNForMapStore(" FMT_VN ", " FMT_VN ", " FMT_VN "):%s in " FMT_BB " returns ", map, index, value, - varTypeName(type), bb->bbNum); + varTypeName(TypeOfVN(result)), bb->bbNum); m_pComp->vnPrint(result, 1); printf("\n"); } @@ -4083,7 +4082,7 @@ ValueNum ValueNumStore::VNApplySelectorsAssign( valueAfter = VNApplySelectorsAssignTypeCoerce(value, dstIndType); } - return VNForMapStore(fieldType, map, fldHndVN, valueAfter); + return VNForMapStore(map, fldHndVN, valueAfter); } } @@ -4289,16 +4288,10 @@ ValueNum Compiler::fgValueNumberArrIndexAssign(CORINFO_CLASS_HANDLE elemTypeEq, // This is the value that should be stored at "arr[inx]". newValAtInx = vnStore->VNApplySelectorsAssign(VNK_Liberal, hAtArrTypeAtArrAtInx, fldSeq, rhsVN, indType); - var_types arrElemFldType = arrElemType; // Uses arrElemType unless we has a non-null fldSeq - if (vnStore->IsVNFunc(newValAtInx)) - { - VNFuncApp funcApp; - vnStore->GetVNFunc(newValAtInx, &funcApp); - if (funcApp.m_func == VNF_MapStore) - { - arrElemFldType = vnStore->TypeOfVN(newValAtInx); - } - } + // TODO-VNTypes: the validation below is a workaround for logic in ApplySelectorsAssignTypeCoerce + // not handling some cases correctly. Remove once ApplySelectorsAssignTypeCoerce has been fixed. + var_types arrElemFldType = + (fldSeq != nullptr) ? eeGetFieldType(fldSeq->GetTail()->GetFieldHandle()) : arrElemType; if (indType != arrElemFldType) { @@ -4315,8 +4308,8 @@ ValueNum Compiler::fgValueNumberArrIndexAssign(CORINFO_CLASS_HANDLE elemTypeEq, if (!invalidateArray) { - newValAtArr = vnStore->VNForMapStore(indType, hAtArrTypeAtArr, inxVN, newValAtInx); - newValAtArrType = vnStore->VNForMapStore(TYP_REF, hAtArrType, arrVN, newValAtArr); + newValAtArr = vnStore->VNForMapStore(hAtArrTypeAtArr, inxVN, newValAtInx); + newValAtArrType = vnStore->VNForMapStore(hAtArrType, arrVN, newValAtArr); } #ifdef DEBUG @@ -4354,7 +4347,7 @@ ValueNum Compiler::fgValueNumberArrIndexAssign(CORINFO_CLASS_HANDLE elemTypeEq, } #endif // DEBUG - return vnStore->VNForMapStore(TYP_REF, fgCurMemoryVN[GcHeap], elemTypeEqVN, newValAtArrType); + return vnStore->VNForMapStore(fgCurMemoryVN[GcHeap], elemTypeEqVN, newValAtArrType); } ValueNum Compiler::fgValueNumberArrIndexVal(GenTree* tree, VNFuncApp* pFuncApp, ValueNum addrXvn) @@ -7157,8 +7150,7 @@ ValueNum Compiler::fgMemoryVNForLoopSideEffects(MemoryKind memoryKind, // values. Static field maps, on the other hand, do, and so must be given proper types. var_types fldMapType = eeIsFieldStatic(fldHnd) ? eeGetFieldType(fldHnd) : TYP_REF; - newMemoryVN = - vnStore->VNForMapStore(TYP_REF, newMemoryVN, fldHndVN, vnStore->VNForExpr(entryBlock, fldMapType)); + newMemoryVN = vnStore->VNForMapStore(newMemoryVN, fldHndVN, vnStore->VNForExpr(entryBlock, fldMapType)); } } // Now do the array maps. @@ -7190,7 +7182,7 @@ ValueNum Compiler::fgMemoryVNForLoopSideEffects(MemoryKind memoryKind, ValueNum elemTypeVN = vnStore->VNForHandle(ssize_t(elemClsHnd), GTF_ICON_CLASS_HDL); ValueNum uniqueVN = vnStore->VNForExpr(entryBlock, TYP_REF); - newMemoryVN = vnStore->VNForMapStore(TYP_REF, newMemoryVN, elemTypeVN, uniqueVN); + newMemoryVN = vnStore->VNForMapStore(newMemoryVN, elemTypeVN, uniqueVN); } } } @@ -7885,12 +7877,11 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree) // Finally, construct the new field map... ValueNum newFldMapVN = - vnStore->VNForMapStore(vnStore->TypeOfVN(fldMapVN), fldMapVN, firstFieldValueSelectorVN, - newFirstFieldValueVN); + vnStore->VNForMapStore(fldMapVN, firstFieldValueSelectorVN, newFirstFieldValueVN); // ...and a new value for the heap. - newHeapVN = vnStore->VNForMapStore(TYP_REF, fgCurMemoryVN[GcHeap], firstFieldSelectorVN, - newFldMapVN); + newHeapVN = + vnStore->VNForMapStore(fgCurMemoryVN[GcHeap], firstFieldSelectorVN, newFldMapVN); } else { diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index da8d234dd988e..2d7559dd75fc3 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -589,7 +589,7 @@ class ValueNumStore ValueNumKind vnk, var_types type, ValueNum map, ValueNum index, int* pBudget, bool* pUsedRecursiveVN); // 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 VNForMapStore(ValueNum map, ValueNum index, ValueNum value); ValueNum VNForFieldSelector(CORINFO_FIELD_HANDLE fieldHnd, var_types* pFieldType, size_t* pStructSize = nullptr);