From 9b2cb87d6e8e6fedbf84b1b34e8a2e531bf3f9ba Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Fri, 19 Nov 2021 23:15:35 +0300 Subject: [PATCH 1/2] 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. --- src/coreclr/jit/valuenum.cpp | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index d0a1132540bc5..ef056db8c336b 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -4289,16 +4289,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) { From a5bfcd2a1476615de1a1d31dd1e4c8baae6c7fb7 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sat, 30 Oct 2021 14:03:27 +0300 Subject: [PATCH 2/2] 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 | 27 ++++++++++++--------------- src/coreclr/jit/valuenum.h | 2 +- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index ef056db8c336b..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); } } @@ -4309,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 @@ -4348,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) @@ -7151,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. @@ -7184,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); } } } @@ -7879,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);