Skip to content

Commit

Permalink
Physical value numbering (#68712)
Browse files Browse the repository at this point in the history 
Value numbering supports precise numbering of field accesses using "maps":
where each access is modeled as a selection: "map[indices...]". It has been
the case until this change that said indices were always "precise" - VNs
of field handles.

This system has proven to be problematic for representing struct field access:

 1) The precise model effectively means that each field access represented
    by a unique handle cannot alias access to the same location, but using
    a different handle. This meant that reinterpretation of structs, reasonably
    common both in user code and in the IR compiler creates itself, was UB.
 2) The precise model for struct fields entailed supporting "zero-offset
    field sequences", which were maintained in a side map and caused a good
    number of bugs.

This change solves both of these problems by eliminating the need to use
precise selectors for struct fields, introducing a new kind of selector
(and maps to go with it): "the physical selector": offset plus load/store
size, with "VNForMapSelectWork" enhanced to look through physical store
maps, correctly detecting aliasing.

The precise selection rules are maintained for the maps indexing off of the
heap, where we don't have the same aliasing concerns. Physical maps are now
used exclusively for numbering locals.

This change seeks to preserve previous behavior to avoid diffs: many places
with now-not-needed pessimization are marked with "TODO-PhysicalVN". Similarly,
the field sequence infrastructure supporting the old precise selection scheme
is retained in its full generality. Future changes are expected to remove much
of it.
  • Loading branch information
@SingleAccretion
SingleAccretion authored May 6, 2022
1 parent e03614d commit ed2472b
Show file tree
Hide file tree
Showing 12 changed files with 1,441 additions and 1,218 deletions.
53 changes: 19 additions & 34 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4700,40 +4700,25 @@ class Compiler
// tree node).
void fgValueNumber();

// Computes new GcHeap VN via the assignment H[elemTypeEq][arrVN][inx][fldSeq] = rhsVN.
// Assumes that "elemTypeEq" is the (equivalence class rep) of the array element type.
// The 'indType' is the indirection type of the lhs of the assignment and will typically
// match the element type of the array or fldSeq. When this type doesn't match
// or if the fldSeq is 'NotAField' we invalidate the array contents H[elemTypeEq][arrVN]
//
ValueNum fgValueNumberArrIndexAssign(CORINFO_CLASS_HANDLE elemTypeEq,
ValueNum arrVN,
ValueNum inxVN,
FieldSeqNode* fldSeq,
ValueNum rhsVN,
var_types indType);

// Requires that "tree" is a GT_IND marked as an array index, and that its address argument
// has been parsed to yield the other input arguments. If evaluation of the address
// can raise exceptions, those should be captured in the exception set "addrXvnp".
// Assumes that "elemTypeEq" is the (equivalence class rep) of the array element type.
// Marks "tree" with the VN for H[elemTypeEq][arrVN][inx][fldSeq] (for the liberal VN; a new unique
// VN for the conservative VN.) Also marks the tree's argument as the address of an array element.
// The type tree->TypeGet() will typically match the element type of the array or fldSeq.
// When this type doesn't match or if the fldSeq is 'NotAField' we return a new unique VN
//
ValueNum fgValueNumberArrIndexVal(GenTree* tree,
CORINFO_CLASS_HANDLE elemTypeEq,
ValueNum arrVN,
ValueNum inxVN,
ValueNumPair addrXvnp,
FieldSeqNode* fldSeq);

// Requires "funcApp" to be a VNF_PtrToArrElem, and "addrXvnp" to represent the exception set thrown
// by evaluating the array index expression "tree". Returns the value number resulting from
// dereferencing the array in the current GcHeap state. If "tree" is non-null, it must be the
// "GT_IND" that does the dereference, and it is given the returned value number.
ValueNum fgValueNumberArrIndexVal(GenTree* tree, VNFuncApp* funcApp, ValueNumPair addrXvnp);
void fgValueNumberLocalStore(GenTree* storeNode,
GenTreeLclVarCommon* lclDefNode,
ssize_t offset,
unsigned storeSize,
ValueNumPair value,
bool normalize = true);

void fgValueNumberArrayElemLoad(GenTree* loadTree, VNFuncApp* addrFunc);

void fgValueNumberArrayElemStore(GenTree* storeNode, VNFuncApp* addrFunc, unsigned storeSize, ValueNum value);

void fgValueNumberFieldLoad(GenTree* loadTree, GenTree* baseAddr, FieldSeqNode* fieldSeq, ssize_t offset);

void fgValueNumberFieldStore(GenTree* storeNode,
GenTree* baseAddr,
FieldSeqNode* fieldSeq,
ssize_t offset,
unsigned storeSize,
ValueNum value);

// Compute the value number for a byref-exposed load of the given type via the given pointerVN.
ValueNum fgValueNumberByrefExposedLoad(var_types type, ValueNum pointerVN);
Expand Down
100 changes: 74 additions & 26 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16019,7 +16019,7 @@ bool GenTree::IsPartialLclFld(Compiler* comp)
(comp->lvaTable[this->AsLclVarCommon()->GetLclNum()].lvExactSize != genTypeSize(gtType)));
}

bool GenTree::DefinesLocal(Compiler* comp, GenTreeLclVarCommon** pLclVarTree, bool* pIsEntire)
bool GenTree::DefinesLocal(Compiler* comp, GenTreeLclVarCommon** pLclVarTree, bool* pIsEntire, ssize_t* pOffset)
{
GenTreeBlk* blkNode = nullptr;
if (OperIs(GT_ASG))
Expand All @@ -16039,12 +16039,17 @@ bool GenTree::DefinesLocal(Compiler* comp, GenTreeLclVarCommon** pLclVarTree, bo
*pIsEntire = true;
}
}
if (pOffset != nullptr)
{
*pOffset = AsOp()->gtOp1->AsLclVarCommon()->GetLclOffs();
}
return true;
}
else if (AsOp()->gtOp1->OperGet() == GT_IND)
{
GenTree* indArg = AsOp()->gtOp1->AsOp()->gtOp1;
return indArg->DefinesLocalAddr(comp, genTypeSize(AsOp()->gtOp1->TypeGet()), pLclVarTree, pIsEntire);
return indArg->DefinesLocalAddr(comp, genTypeSize(AsOp()->gtOp1->TypeGet()), pLclVarTree, pIsEntire,
pOffset);
}
else if (AsOp()->gtOp1->OperIsBlk())
{
Expand All @@ -16060,7 +16065,7 @@ bool GenTree::DefinesLocal(Compiler* comp, GenTreeLclVarCommon** pLclVarTree, bo
}

unsigned size = comp->typGetObjLayout(AsCall()->gtRetClsHnd)->GetSize();
return retBufArg->DefinesLocalAddr(comp, size, pLclVarTree, pIsEntire);
return retBufArg->DefinesLocalAddr(comp, size, pLclVarTree, pIsEntire, pOffset);
}
else if (OperIsBlk())
{
Expand All @@ -16086,14 +16091,14 @@ bool GenTree::DefinesLocal(Compiler* comp, GenTreeLclVarCommon** pLclVarTree, bo
}
}

return destAddr->DefinesLocalAddr(comp, width, pLclVarTree, pIsEntire);
return destAddr->DefinesLocalAddr(comp, width, pLclVarTree, pIsEntire, pOffset);
}
// Otherwise...
return false;
}

// Returns true if this GenTree defines a result which is based on the address of a local.
bool GenTree::DefinesLocalAddr(Compiler* comp, unsigned width, GenTreeLclVarCommon** pLclVarTree, bool* pIsEntire)
bool GenTree::DefinesLocalAddr(
Compiler* comp, unsigned width, GenTreeLclVarCommon** pLclVarTree, bool* pIsEntire, ssize_t* pOffset)
{
if (OperIs(GT_ADDR, GT_LCL_VAR_ADDR, GT_LCL_FLD_ADDR))
{
Expand All @@ -16107,10 +16112,10 @@ bool GenTree::DefinesLocalAddr(Compiler* comp, unsigned width, GenTreeLclVarComm
{
GenTreeLclVarCommon* addrArgLcl = addrArg->AsLclVarCommon();
*pLclVarTree = addrArgLcl;
unsigned lclOffset = addrArgLcl->GetLclOffs();

if (pIsEntire != nullptr)
{
unsigned lclOffset = addrArgLcl->GetLclOffs();

if (lclOffset != 0)
{
// We aren't updating the bytes at [0..lclOffset-1] so *pIsEntire should be set to false
Expand All @@ -16129,29 +16134,45 @@ bool GenTree::DefinesLocalAddr(Compiler* comp, unsigned width, GenTreeLclVarComm
*pIsEntire = (varWidth == width);
}
}

if (pOffset != nullptr)
{
*pOffset += lclOffset;
}

return true;
}
else if (addrArg->OperGet() == GT_IND)
{
// A GT_ADDR of a GT_IND can both be optimized away, recurse using the child of the GT_IND
return addrArg->AsOp()->gtOp1->DefinesLocalAddr(comp, width, pLclVarTree, pIsEntire);
return addrArg->AsOp()->gtOp1->DefinesLocalAddr(comp, width, pLclVarTree, pIsEntire, pOffset);
}
}
else if (OperGet() == GT_ADD)
{
if (AsOp()->gtOp1->IsCnsIntOrI())
{
if (pOffset != nullptr)
{
*pOffset += AsOp()->gtOp1->AsIntCon()->IconValue();
}

// If we just adding a zero then we allow an IsEntire match against width
// otherwise we change width to zero to disallow an IsEntire Match
return AsOp()->gtOp2->DefinesLocalAddr(comp, AsOp()->gtOp1->IsIntegralConst(0) ? width : 0, pLclVarTree,
pIsEntire);
pIsEntire, pOffset);
}
else if (AsOp()->gtOp2->IsCnsIntOrI())
{
if (pOffset != nullptr)
{
*pOffset += AsOp()->gtOp2->AsIntCon()->IconValue();
}

// If we just adding a zero then we allow an IsEntire match against width
// otherwise we change width to zero to disallow an IsEntire Match
return AsOp()->gtOp1->DefinesLocalAddr(comp, AsOp()->gtOp2->IsIntegralConst(0) ? width : 0, pLclVarTree,
pIsEntire);
pIsEntire, pOffset);
}
}
// Post rationalization we could have GT_IND(GT_LEA(..)) trees.
Expand All @@ -16167,20 +16188,20 @@ bool GenTree::DefinesLocalAddr(Compiler* comp, unsigned width, GenTreeLclVarComm
GenTree* index = AsOp()->gtOp2;
if (index != nullptr)
{
assert(!index->DefinesLocalAddr(comp, width, pLclVarTree, pIsEntire));
assert(!index->DefinesLocalAddr(comp, width, pLclVarTree, pIsEntire, pOffset));
}
#endif // DEBUG

// base
GenTree* base = AsOp()->gtOp1;
GenTree* base = AsAddrMode()->Base();
if (base != nullptr)
{
// Lea could have an Indir as its base.
if (base->OperGet() == GT_IND)
if (pOffset != nullptr)
{
base = base->AsOp()->gtOp1->gtEffectiveVal(/*commas only*/ true);
*pOffset += AsAddrMode()->Offset();
}
return base->DefinesLocalAddr(comp, width, pLclVarTree, pIsEntire);

return base->DefinesLocalAddr(comp, width, pLclVarTree, pIsEntire, pOffset);
}
}
// Otherwise...
Expand Down Expand Up @@ -16626,6 +16647,12 @@ ssize_t GenTreeIndir::Offset()
}
}

unsigned GenTreeIndir::Size() const
{
assert(isIndir() || OperIsBlk());
return OperIsBlk() ? AsBlk()->Size() : genTypeSize(TypeGet());
}

//------------------------------------------------------------------------
// GenTreeIntConCommon::ImmedValNeedsReloc: does this immediate value needs recording a relocation with the VM?
//
Expand Down Expand Up @@ -16753,6 +16780,7 @@ bool GenTreeIntConCommon::AddrNeedsReloc(Compiler* comp)
// comp - the Compiler object
// pBaseAddr - [out] parameter for "the base address"
// pFldSeq - [out] parameter for the field sequence
// pOffset - [out] parameter for the offset of the component struct fields
//
// Return Value:
// If "this" matches patterns denoted above, and the FldSeq found is "full",
Expand All @@ -16764,7 +16792,7 @@ bool GenTreeIntConCommon::AddrNeedsReloc(Compiler* comp)
// reference, for statics - the address to which the field offset with the
// field sequence is added, see "impImportStaticFieldAccess" and "fgMorphField".
//
bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pFldSeq)
bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pFldSeq, ssize_t* pOffset)
{
assert(TypeIs(TYP_I_IMPL, TYP_BYREF, TYP_REF));

Expand All @@ -16773,6 +16801,7 @@ bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pF

GenTree* baseAddr = nullptr;
FieldSeqNode* fldSeq = FieldSeqStore::NotAField();
ssize_t offset = 0;

if (OperIs(GT_ADD))
{
Expand All @@ -16781,14 +16810,16 @@ bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pF
if (AsOp()->gtOp1->IsCnsIntOrI() && AsOp()->gtOp1->IsIconHandle())
{
assert(!AsOp()->gtOp2->IsCnsIntOrI() || !AsOp()->gtOp2->IsIconHandle());
fldSeq = AsOp()->gtOp1->AsIntCon()->gtFieldSeq;
baseAddr = AsOp()->gtOp2;
fldSeq = AsOp()->gtOp1->AsIntCon()->gtFieldSeq;
offset = AsOp()->gtOp1->AsIntCon()->IconValue();
}
else if (AsOp()->gtOp2->IsCnsIntOrI())
{
assert(!AsOp()->gtOp1->IsCnsIntOrI() || !AsOp()->gtOp1->IsIconHandle());
fldSeq = AsOp()->gtOp2->AsIntCon()->gtFieldSeq;
baseAddr = AsOp()->gtOp1;
fldSeq = AsOp()->gtOp2->AsIntCon()->gtFieldSeq;
offset = AsOp()->gtOp2->AsIntCon()->IconValue();
}
else
{
Expand All @@ -16800,12 +16831,15 @@ bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pF
else if (IsCnsIntOrI() && IsIconHandle(GTF_ICON_STATIC_HDL))
{
assert(!comp->GetZeroOffsetFieldMap()->Lookup(this) && (AsIntCon()->gtFieldSeq != nullptr));
fldSeq = AsIntCon()->gtFieldSeq;
baseAddr = this;
fldSeq = AsIntCon()->gtFieldSeq;
offset = AsIntCon()->IconValue();
}
else if (comp->GetZeroOffsetFieldMap()->Lookup(this, &fldSeq))
{
assert((fldSeq != FieldSeqStore::NotAField()) || (fldSeq->GetOffset() == 0));
baseAddr = this;
offset = 0;
}
else
{
Expand All @@ -16819,6 +16853,9 @@ bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pF
return false;
}

// Subtract from the offset such that the portion remaining is relative to the field itself.
offset -= fldSeq->GetOffset();

// The above screens out obviously invalid cases, but we have more checks to perform. The
// sequence returned from this method *must* start with either a class (NOT struct) field
// or a static field. To avoid the expense of calling "getFieldClass" here, we will instead
Expand All @@ -16833,6 +16870,7 @@ bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pF
}

*pFldSeq = fldSeq;
*pOffset = offset;
return true;
}

Expand All @@ -16842,6 +16880,7 @@ bool GenTree::IsFieldAddr(Compiler* comp, GenTree** pBaseAddr, FieldSeqNode** pF

*pBaseAddr = baseAddr;
*pFldSeq = fldSeq;
*pOffset = offset;
return true;
}

Expand Down Expand Up @@ -18110,16 +18149,18 @@ bool GenTree::IsArrayAddr(GenTreeArrAddr** pArrAddr)
// Note that the value of the below field doesn't matter; it exists only to provide a distinguished address.
//
// static
FieldSeqNode FieldSeqStore::s_notAField(nullptr, nullptr, FieldSeqNode::FieldKind::Instance);
FieldSeqNode FieldSeqStore::s_notAField(nullptr, nullptr, 0, FieldSeqNode::FieldKind::Instance);

// FieldSeqStore methods.
FieldSeqStore::FieldSeqStore(CompAllocator alloc) : m_alloc(alloc), m_canonMap(new (alloc) FieldSeqNodeCanonMap(alloc))
{
}

FieldSeqNode* FieldSeqStore::CreateSingleton(CORINFO_FIELD_HANDLE fieldHnd, FieldSeqNode::FieldKind fieldKind)
FieldSeqNode* FieldSeqStore::CreateSingleton(CORINFO_FIELD_HANDLE fieldHnd,
size_t offset,
FieldSeqNode::FieldKind fieldKind)
{
FieldSeqNode fsn(fieldHnd, nullptr, fieldKind);
FieldSeqNode fsn(fieldHnd, nullptr, offset, fieldKind);
FieldSeqNode* res = nullptr;
if (m_canonMap->Lookup(fsn, &res))
{
Expand Down Expand Up @@ -18158,7 +18199,7 @@ FieldSeqNode* FieldSeqStore::Append(FieldSeqNode* a, FieldSeqNode* b)
assert(a != b);

FieldSeqNode* tmp = Append(a->GetNext(), b);
FieldSeqNode fsn(a->GetFieldHandleValue(), tmp, a->GetKind());
FieldSeqNode fsn(a->GetFieldHandleValue(), tmp, a->GetOffset(), a->GetKind());
FieldSeqNode* res = nullptr;
if (m_canonMap->Lookup(fsn, &res))
{
Expand All @@ -18174,7 +18215,8 @@ FieldSeqNode* FieldSeqStore::Append(FieldSeqNode* a, FieldSeqNode* b)
}
}

FieldSeqNode::FieldSeqNode(CORINFO_FIELD_HANDLE fieldHnd, FieldSeqNode* next, FieldKind fieldKind) : m_next(next)
FieldSeqNode::FieldSeqNode(CORINFO_FIELD_HANDLE fieldHnd, FieldSeqNode* next, size_t offset, FieldKind fieldKind)
: m_next(next), m_offset(offset)
{
uintptr_t handleValue = reinterpret_cast<uintptr_t>(fieldHnd);

Expand All @@ -18184,6 +18226,7 @@ FieldSeqNode::FieldSeqNode(CORINFO_FIELD_HANDLE fieldHnd, FieldSeqNode* next, Fi
if (fieldHnd != NO_FIELD_HANDLE)
{
assert(JitTls::GetCompiler()->eeIsFieldStatic(fieldHnd) == IsStaticField());
// TODO-PhysicalVN: assert that "offset" is correct.
}
else
{
Expand Down Expand Up @@ -23173,6 +23216,11 @@ unsigned GenTreeHWIntrinsic::GetResultOpNumForFMA(GenTree* use, GenTree* op1, Ge
}
#endif // TARGET_XARCH && FEATURE_HW_INTRINSICS

unsigned GenTreeLclFld::GetSize() const
{
return genTypeSize(TypeGet());
}

#ifdef TARGET_ARM
//------------------------------------------------------------------------
// IsOffsetMisaligned: check if the field needs a special handling on arm.
Expand Down
Loading

0 comments on commit ed2472b

Please sign in to comment.