-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix value numbering of field assignments #61113
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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,80 @@ 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, firstFieldType, | ||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we not need to unpack the VN like before? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, scratch that, I see this just uses VNLiberalNormalValue now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that the tracking of exception sets this code did before, i. e. vnExcSet = vnStore->VNExcSetUnion(vnExcSet, vnObjExcSet); was dead code because nothing uses (Edit: in general, one should assume that the exception sets are only correct "for CSE purposes", because e. g. |
||
} | ||
else // (staticOffset != nullptr) | ||
{ | ||
// construct the ValueNumber for 'fldMap at staticOffset' | ||
normVal = vnStore->VNLiberalNormalValue(staticOffset->gtVNPair); | ||
valAtAddr = vnStore->VNForMapSelect(VNK_Liberal, firstFieldType, fldMapVN, normVal); | ||
firstFieldValueSelectorVN = vnStore->VNLiberalNormalValue(staticOffset->gtVNPair); | ||
} | ||
// Now get rid of any remaining struct field dereferences. (if they exist) | ||
if (fldSeq->m_next) | ||
|
||
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) | ||
{ | ||
storeVal = vnStore->VNApplySelectorsAssign(VNK_Liberal, valAtAddr, fldSeq->m_next, | ||
storeVal, indType); | ||
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); | ||
} | ||
|
||
// From which we can construct the new ValueNumber for 'fldMap at normVal' | ||
newFldMapVN = | ||
vnStore->VNForMapStore(vnStore->TypeOfVN(fldMapVN), fldMapVN, normVal, storeVal); | ||
// 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this dead code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I know, yes, because we use (I would not be surprised if it is reachable under some exotic circumstances of course) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think so, public class Program
{
public static S s_s;
public static int s_i;
public static void Main()
{
s_s = s_s;
Foo();
}
[MethodImpl(MethodImplOptions.NoInlining)]
public static unsafe void Foo()
{
Bar(ref s_i);
}
public static void Bar(ref int i) => i = 10;
} but currently we do not manage to VN this: ***** BB01, STMT00002(before)
N004 ( 6, 14) [000006] -A--G------- ▌ ASG int
N002 ( 4, 12) [000005] *------N---- ├──▌ IND int
N001 ( 2, 10) [000003] I----------- │ └──▌ CNS_INT(h) byref 0x7ffa80d505b8 static Fseq[s_i]
N003 ( 1, 1) [000004] ------------ └──▌ CNS_INT int 10
N001 [000003] CNS_INT(h) 0x7ffa80d505b8 static Fseq[s_i] => $c0 {Hnd const: 0x00007FFA80D505B8}
N003 [000004] CNS_INT 10 => $41 {IntCns 10}
fgCurMemoryVN[GcHeap] assigned for assign-of-IND at [000006] to VN: $81.
N004 [000006] ASG => $VN.Void
***** BB01, STMT00002(after)
N004 ( 6, 14) [000006] -A--G------- ▌ ASG int $VN.Void
N002 ( 4, 12) [000005] *------N---- ├──▌ IND int $41
N001 ( 2, 10) [000003] I----------- │ └──▌ CNS_INT(h) byref 0x7ffa80d505b8 static Fseq[s_i] $c0
N003 ( 1, 1) [000004] ------------ └──▌ CNS_INT int 10 $41 |
||
} | ||
|
||
// 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 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be using
TYP_REF
instead offirstFieldType
here accordingly to the discussion last week?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we should, but as standalone change, it will create regressions because the reading side of this code needs to be updated to use the same type (it is using
firstFieldType
at the moment). This is the reason for the 4th commit:In the next change, I will update the reading side, and in the change after that - the types for the heaps/"first field maps" (then will come the many changes needed to enable clean validation, after that the validation itself, and finally the actual bugfixes).