Skip to content

Commit

Permalink
Do not explicitly pass type to VNForMapStore (#61882)
Browse files Browse the repository at this point in the history 
* 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.
  • Loading branch information
@SingleAccretion
SingleAccretion authored Nov 22, 2021
1 parent c334e55 commit a6e0f25
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 26 deletions.
41 changes: 16 additions & 25 deletions src/coreclr/jit/valuenum.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2200,25 +2200,24 @@ ValueNum ValueNumStore::VNForFunc(
//
//
// Arguments:
// type - Type for the new map
// map - Map value number
// index - Index value number
// value - New value for map[index]
//
// 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");
}
Expand Down Expand Up @@ -4083,7 +4082,7 @@ ValueNum ValueNumStore::VNApplySelectorsAssign(
valueAfter = VNApplySelectorsAssignTypeCoerce(value, dstIndType);
}

return VNForMapStore(fieldType, map, fldHndVN, valueAfter);
return VNForMapStore(map, fldHndVN, valueAfter);
}
}

Expand Down Expand Up @@ -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)
{
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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
{
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/valuenum.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down

0 comments on commit a6e0f25

Please sign in to comment.