Skip to content

Proper VNs for zeroed structs #61285

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.

Sign up for GitHub

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

Merged
jakobbotsch merged 2 commits into from
Nov 9, 2021
Merged

Proper VNs for zeroed structs #61285

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
132f31f
Number zero-initialized struct properly
SingleAccretion Nov 5, 2021
c884372
Fix VNZeroForType's handling of SIMD types
SingleAccretion Nov 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Number zero-initialized struct properly 
Previously, zero-initialized struct locals were given
a special $VNForZeroMap, which had the nice property
that VNForMapSelect($VNForZeroMap, Field) would always
return zero of the appropriate type. However, the fact
that this VN is not unique meant that it was dangerous
to propagate it, e. g. through copies, which was hidden
by the fact that ApplySelectorsTypeCheck's logic is very
conservative.

This change introduces a new VNFunc to represent zeroed
objects - VNF_ZeroObj, that takes a struct handle as its
argument and is thus free of the aforementioned problem and
can be propagated freely and not treated specially. It
also, of course, retains the ZeroMap's selection property.

There are almost no diffs for this change because of some
other deficiencies in assertion and copy propagation, they
are being/will be fixed separately.
  • Loading branch information
@SingleAccretion
SingleAccretion committed Nov 7, 2021
commit 132f31fc270dca6e312ecafb2f9c3352518d227d
2 changes: 1 addition & 1 deletion src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7986,7 +7986,7 @@ class Compiler
bool eeIsJitIntrinsic(CORINFO_METHOD_HANDLE ftn);
bool eeIsFieldStatic(CORINFO_FIELD_HANDLE fldHnd);

var_types eeGetFieldType(CORINFO_FIELD_HANDLE fldHnd);
var_types eeGetFieldType(CORINFO_FIELD_HANDLE fldHnd, CORINFO_CLASS_HANDLE* pStructHnd = nullptr);

#if defined(DEBUG) || defined(FEATURE_JIT_METHOD_PERF) || defined(FEATURE_SIMD) || defined(TRACK_LSRA_STATS)

Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/ee_il_dll.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ bool Compiler::eeIsFieldStatic(CORINFO_FIELD_HANDLE fldHnd)
}

FORCEINLINE
var_types Compiler::eeGetFieldType(CORINFO_FIELD_HANDLE fldHnd)
var_types Compiler::eeGetFieldType(CORINFO_FIELD_HANDLE fldHnd, CORINFO_CLASS_HANDLE* pStructHnd)
{
return JITtype2varType(info.compCompHnd->getFieldType(fldHnd));
return JITtype2varType(info.compCompHnd->getFieldType(fldHnd, pStructHnd));
}

FORCEINLINE
Expand Down
106 changes: 62 additions & 44 deletions src/coreclr/jit/valuenum.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,11 +464,9 @@ ValueNumStore::ValueNumStore(Compiler* comp, CompAllocator alloc)
{
m_VNsForSmallIntConsts[i] = NoVN;
}
// We will reserve chunk 0 to hold some special constants, like the constant NULL, the "exception" value, and the
// "zero map."
// We will reserve chunk 0 to hold some special constants.
Chunk* specialConstChunk = new (m_alloc) Chunk(m_alloc, &m_nextChunkBase, TYP_REF, CEA_Const);
specialConstChunk->m_numUsed +=
SRC_NumSpecialRefConsts; // Implicitly allocate 0 ==> NULL, and 1 ==> Exception, 2 ==> ZeroMap.
specialConstChunk->m_numUsed += SRC_NumSpecialRefConsts;
ChunkNum cn = m_chunks.Push(specialConstChunk);
assert(cn == 0);

Expand Down Expand Up @@ -1787,8 +1785,6 @@ ValueNum ValueNumStore::VNForHandle(ssize_t cnsVal, GenTreeFlags handleFlags)
}
}

// Returns the value number for zero of the given "typ".
// It has an unreached() for a "typ" that has no zero value, such as TYP_VOID.
ValueNum ValueNumStore::VNZeroForType(var_types typ)
{
switch (typ)
Expand All @@ -1812,8 +1808,6 @@ ValueNum ValueNumStore::VNZeroForType(var_types typ)
return VNForNull();
case TYP_BYREF:
return VNForByrefCon(0);
case TYP_STRUCT:
return VNForZeroMap(); // Recursion!

#ifdef FEATURE_SIMD
case TYP_SIMD8:
Expand All @@ -1829,6 +1823,16 @@ ValueNum ValueNumStore::VNZeroForType(var_types typ)
}
}

ValueNum ValueNumStore::VNForZeroObj(CORINFO_CLASS_HANDLE structHnd)
{
assert(structHnd != NO_CLASS_HANDLE);

ValueNum structHndVN = VNForHandle(ssize_t(structHnd), GTF_ICON_CLASS_HDL);
ValueNum zeroObjVN = VNForFunc(TYP_STRUCT, VNF_ZeroObj, structHndVN);

return zeroObjVN;
}

// Returns the value number for one of the given "typ".
// It returns NoVN for a "typ" that has no one value, such as TYP_REF.
ValueNum ValueNumStore::VNOneForType(var_types typ)
Expand Down Expand Up @@ -2317,14 +2321,9 @@ ValueNum ValueNumStore::VNForMapSelectWork(
return RecursiveVN;
}

if (map == VNForZeroMap())
{
return VNZeroForType(type);
}
else if (IsVNFunc(map))
VNFuncApp funcApp;
if (GetVNFunc(map, &funcApp))
{
VNFuncApp funcApp;
GetVNFunc(map, &funcApp);
if (funcApp.m_func == VNF_MapStore)
{
// select(store(m, i, v), i) == v
Expand Down Expand Up @@ -2476,6 +2475,22 @@ ValueNum ValueNumStore::VNForMapSelectWork(
m_fixedPointMapSels.Pop();
}
}
else if (funcApp.m_func == VNF_ZeroObj)
{
// For structs, we need to extract the handle from the selector.
if (type == TYP_STRUCT)
{
// We only expect field selectors here.
assert(GetHandleFlags(index) == GTF_ICON_FIELD_HDL);
CORINFO_FIELD_HANDLE fieldHnd = CORINFO_FIELD_HANDLE(ConstantValue<ssize_t>(index));
CORINFO_CLASS_HANDLE structHnd = NO_CLASS_HANDLE;
m_pComp->eeGetFieldType(fieldHnd, &structHnd);

return VNForZeroObj(structHnd);
}

return VNZeroForType(type);
}
}

// We may have run out of budget and already assigned a result
Expand Down Expand Up @@ -5792,11 +5807,6 @@ void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr)
{
printf("void");
}
else
{
assert(vn == VNForZeroMap());
printf("zeroMap");
}
break;
case TYP_BYREF:
printf("byrefVal");
Expand Down Expand Up @@ -5868,6 +5878,9 @@ void ValueNumStore::vnDump(Compiler* comp, ValueNum vn, bool isPtr)
case VNF_CastOvf:
vnDumpCast(comp, vn);
break;
case VNF_ZeroObj:
vnDumpZeroObj(comp, &funcApp);
break;

default:
printf("%s(", VNFuncName(funcApp.m_func));
Expand Down Expand Up @@ -6089,6 +6102,12 @@ void ValueNumStore::vnDumpCast(Compiler* comp, ValueNum castVN)
}
}

void ValueNumStore::vnDumpZeroObj(Compiler* comp, VNFuncApp* zeroObj)
{
printf("ZeroObj(");
comp->vnPrint(zeroObj->m_args[0], 0);
printf(": %s)", comp->eeGetClassName(CORINFO_CLASS_HANDLE(ConstantValue<ssize_t>(zeroObj->m_args[0]))));
}
#endif // DEBUG

// Static fields, methods.
Expand Down Expand Up @@ -6252,10 +6271,9 @@ static const char* s_reservedNameArr[] = {
"$VN.Recursive", // -2 RecursiveVN
"$VN.No", // -1 NoVN
"$VN.Null", // 0 VNForNull()
"$VN.ZeroMap", // 1 VNForZeroMap()
"$VN.ReadOnlyHeap", // 2 VNForROH()
"$VN.Void", // 3 VNForVoid()
"$VN.EmptyExcSet" // 4 VNForEmptyExcSet()
"$VN.ReadOnlyHeap", // 1 VNForROH()
"$VN.Void", // 2 VNForVoid()
"$VN.EmptyExcSet" // 3 VNForEmptyExcSet()
};

// Returns the string name of "vn" when it is a reserved value number, nullptr otherwise
Expand Down Expand Up @@ -6680,7 +6698,8 @@ void Compiler::fgValueNumber()
if (isZeroed)
{
// By default we will zero init these LclVars
initVal = vnStore->VNZeroForType(typ);
initVal = (typ == TYP_STRUCT) ? vnStore->VNForZeroObj(varDsc->GetStructHnd())
: vnStore->VNZeroForType(typ);
}
else
{
Expand Down Expand Up @@ -7942,39 +7961,38 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
// Should not have been recorded as updating the GC heap.
assert(!GetMemorySsaMap(GcHeap)->Lookup(tree));

unsigned lclNum = lclVarTree->GetLclNum();

unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree);

// Ignore vars that we excluded from SSA (for example, because they're address-exposed). They don't have
// SSA names in which to store VN's on defs. We'll yield unique VN's when we read from them.
if (lvaInSsa(lclNum) && lclDefSsaNum != SsaConfig::RESERVED_SSA_NUM)
if (lclDefSsaNum != SsaConfig::RESERVED_SSA_NUM)
{
LclVarDsc* lclVarDsc = lvaGetDesc(lclVarTree);

// Should not have been recorded as updating ByrefExposed.
assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree));

ValueNum initBlkVN = ValueNumStore::NoVN;
GenTree* initConst = rhs;
if (isEntire && initConst->OperGet() == GT_CNS_INT)
ValueNum lclVarVN = ValueNumStore::NoVN;
if (isEntire && rhs->IsIntegralConst(0))
{
unsigned initVal = 0xFF & (unsigned)initConst->AsIntConCommon()->IconValue();
if (initVal == 0)
{
initBlkVN = vnStore->VNZeroForType(lclVarTree->TypeGet());
}
// Note that it is possible to see pretty much any kind of type for the local
// (not just TYP_STRUCT) here because of the ASG(BLK(ADDR(LCL_VAR/FLD)), 0) form.
lclVarVN = (lclVarDsc->TypeGet() == TYP_STRUCT) ? vnStore->VNForZeroObj(lclVarDsc->GetStructHnd())
: vnStore->VNZeroForType(lclVarDsc->TypeGet());
}
else
{
// Non-zero block init is very rare so we'll use a simple, unique VN here.
lclVarVN = vnStore->VNForExpr(compCurBB, lclVarDsc->TypeGet());
}
ValueNum lclVarVN = (initBlkVN != ValueNumStore::NoVN)
? initBlkVN
: vnStore->VNForExpr(compCurBB, var_types(lvaTable[lclNum].lvType));

lvaTable[lclNum].GetPerSsaData(lclDefSsaNum)->m_vnPair.SetBoth(lclVarVN);
lclVarDsc->GetPerSsaData(lclDefSsaNum)->m_vnPair.SetBoth(lclVarVN);
#ifdef DEBUG
if (verbose)
{
printf("N%03u ", tree->gtSeqNum);
printf("Tree ");
Compiler::printTreeID(tree);
printf(" ");
gtDispNodeName(tree);
printf(" V%02u/%d => ", lclNum, lclDefSsaNum);
printf(" assigned VN to local var V%02u/%d: ", lclVarTree->GetLclNum(), lclDefSsaNum);
vnPrint(lclVarVN, 1);
printf("\n");
}
Expand Down
14 changes: 6 additions & 8 deletions src/coreclr/jit/valuenum.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,13 +420,6 @@ class ValueNumStore
return ValueNum(SRC_Null);
}

// The zero map is the map that returns a zero "for the appropriate type" when indexed at any index.
static ValueNum VNForZeroMap()
{
// We reserve Chunk 0 for "special" VNs. Let SRC_ZeroMap (== 1) be the zero map.
return ValueNum(SRC_ZeroMap);
}

// The ROH map is the map for the "read-only heap". We assume that this is never mutated, and always
// has the same value number.
static ValueNum VNForROH()
Expand Down Expand Up @@ -463,6 +456,9 @@ class ValueNumStore
// It has an unreached() for a "typ" that has no zero value, such as TYP_VOID.
ValueNum VNZeroForType(var_types typ);

// Returns the value number for a zero-initialized struct.
ValueNum VNForZeroObj(CORINFO_CLASS_HANDLE structHnd);

// Returns the value number for one of the given "typ".
// It returns NoVN for a "typ" that has no one value, such as TYP_REF.
ValueNum VNOneForType(var_types typ);
Expand Down Expand Up @@ -1021,6 +1017,9 @@ class ValueNumStore
// Prints the cast's representation mirroring GT_CAST's dump format.
void vnDumpCast(Compiler* comp, ValueNum castVN);

// Requires "zeroObj" to be a VNF_ZeroObj. Prints its representation.
void vnDumpZeroObj(Compiler* comp, VNFuncApp* zeroObj);

// Returns the string name of "vnf".
static const char* VNFuncName(VNFunc vnf);
// Used in the implementation of the above.
Expand Down Expand Up @@ -1449,7 +1448,6 @@ class ValueNumStore
enum SpecialRefConsts
{
SRC_Null,
SRC_ZeroMap,
SRC_ReadOnlyHeap,
SRC_Void,
SRC_EmptyExcSet,
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/valuenumfuncs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ ValueNumFuncDef(MapSelect, 2, false, false, false) // Args: 0: map, 1: key.

ValueNumFuncDef(FieldSeq, 2, false, false, false) // Sequence (VN of null == empty) of (VN's of) field handles.
ValueNumFuncDef(NotAField, 0, false, false, false) // Value number function for FieldSeqStore::NotAField.
ValueNumFuncDef(ZeroMap, 0, false, false, false) // The "ZeroMap": indexing at any index yields "zero of the desired type".

ValueNumFuncDef(PtrToLoc, 2, false, false, false) // Pointer (byref) to a local variable. Args: VN's of: 0: var num, 1: FieldSeq.
ValueNumFuncDef(PtrToArrElem, 4, false, false, false) // Pointer (byref) to an array element. Args: 0: array elem type eq class var_types value, VN's of: 1: array, 2: index, 3: FieldSeq.
Expand All @@ -22,6 +21,7 @@ ValueNumFuncDef(Phi, 2, false, false, false) // A phi function. Only occ
ValueNumFuncDef(PhiDef, 3, false, false, false) // Args: 0: local var # (or -1 for memory), 1: SSA #, 2: VN of definition.
// Wouldn't need this if I'd made memory a regular local variable...
ValueNumFuncDef(PhiMemoryDef, 2, false, false, false) // Args: 0: VN for basic block pointer, 1: VN of definition
ValueNumFuncDef(ZeroObj, 1, false, false, false) // Zero-initialized struct. Args: 0: VN of the class handle.
ValueNumFuncDef(InitVal, 1, false, false, false) // An input arg, or init val of a local Args: 0: a constant VN.


Expand Down