Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Commit

Permalink
JIT: force all local var ref counts to be accessed via API (#18979)
Browse files Browse the repository at this point in the history
This is a preparatory change for auditing and controlling how local
variable ref counts are observed and manipulated.

See #18969 for context.

No diffs seen locally. No TP impact expected.

There is a small chance we may see some asserts in broader testing
as there were places in original code where local ref counts were
incremented without checking for possible overflows. The new APIs
will assert for overflow cases.
  • Loading branch information
AndyAyersMS authored Jul 18, 2018
1 parent 4037baf commit b2842bb
Show file tree
Hide file tree
Showing 18 changed files with 174 additions and 122 deletions.
4 changes: 2 additions & 2 deletions src/jit/assertionprop.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ void Compiler::optAddCopies()
}

// We require that the weighted ref count be significant.
if (varDsc->lvRefCntWtd <= (BB_LOOP_WEIGHT * BB_UNITY_WEIGHT / 2))
if (varDsc->lvRefCntWtd() <= (BB_LOOP_WEIGHT * BB_UNITY_WEIGHT / 2))
{
continue;
}
Expand All @@ -144,7 +144,7 @@ void Compiler::optAddCopies()
BlockSet paramImportantUseDom(BlockSetOps::MakeFull(this));

// This will be threshold for determining heavier-than-average uses
unsigned paramAvgWtdRefDiv2 = (varDsc->lvRefCntWtd + varDsc->lvRefCnt / 2) / (varDsc->lvRefCnt * 2);
unsigned paramAvgWtdRefDiv2 = (varDsc->lvRefCntWtd() + varDsc->lvRefCnt() / 2) / (varDsc->lvRefCnt() * 2);

bool paramFoundImportantUse = false;

Expand Down
10 changes: 5 additions & 5 deletions src/jit/codegencommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3678,7 +3678,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere
// For LSRA, it may not be in regArgMaskLive if it has a zero
// refcnt. This is in contrast with the non-LSRA case in which all
// non-tracked args are assumed live on entry.
noway_assert((varDsc->lvRefCnt == 0) || (varDsc->lvType == TYP_STRUCT) ||
noway_assert((varDsc->lvRefCnt() == 0) || (varDsc->lvType == TYP_STRUCT) ||
(varDsc->lvAddrExposed && compiler->info.compIsVarArgs) ||
(varDsc->lvAddrExposed && compiler->opts.compUseSoftFP));
#endif // !_TARGET_X86_
Expand Down Expand Up @@ -3979,7 +3979,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere

if (!varDsc->lvOnFrame)
{
noway_assert(varDsc->lvRefCnt == 0);
noway_assert(varDsc->lvRefCnt() == 0);
}
else
{
Expand Down Expand Up @@ -4638,7 +4638,7 @@ void CodeGen::genCheckUseBlockInit()

if (!varDsc->lvIsInReg() && !varDsc->lvOnFrame)
{
noway_assert(varDsc->lvRefCnt == 0);
noway_assert(varDsc->lvRefCnt() == 0);
continue;
}

Expand Down Expand Up @@ -7876,7 +7876,7 @@ void CodeGen::genFnProlog()

if (!varDsc->lvIsInReg() && !varDsc->lvOnFrame)
{
noway_assert(varDsc->lvRefCnt == 0);
noway_assert(varDsc->lvRefCnt() == 0);
continue;
}

Expand Down Expand Up @@ -8492,7 +8492,7 @@ void CodeGen::genFnProlog()
// (our argument pointer register has a refcount > 0).
unsigned argsStartVar = compiler->lvaVarargsBaseOfStkArgs;

if (compiler->info.compIsVarArgs && compiler->lvaTable[argsStartVar].lvRefCnt > 0)
if (compiler->info.compIsVarArgs && compiler->lvaTable[argsStartVar].lvRefCnt() > 0)
{
varDsc = &compiler->lvaTable[argsStartVar];

Expand Down
58 changes: 54 additions & 4 deletions src/jit/compiler.h
Original file line number Diff line number Diff line change
Expand Up @@ -591,15 +591,65 @@ class LclVarDsc
regMaskSmall lvPrefReg; // set of regs it prefers to live in

unsigned short lvVarIndex; // variable tracking index
unsigned short lvRefCnt; // unweighted (real) reference count. For implicit by reference

private:
unsigned short m_lvRefCnt; // unweighted (real) reference count. For implicit by reference
// parameters, this gets hijacked from fgMarkImplicitByRefArgs
// through fgMarkDemotedImplicitByRefArgs, to provide a static
// appearance count (computed during address-exposed analysis)
// that fgMakeOutgoingStructArgCopy consults during global morph
// to determine if eliding its copy is legal.
unsigned lvRefCntWtd; // weighted reference count
int lvStkOffs; // stack offset of home
unsigned lvExactSize; // (exact) size of the type in bytes
unsigned m_lvRefCntWtd; // weighted reference count

public:
unsigned short lvRefCnt() const
{
return m_lvRefCnt;
}

void incLvRefCnt(unsigned short delta)
{
unsigned short oldRefCnt = m_lvRefCnt;
m_lvRefCnt += delta;
assert(m_lvRefCnt >= oldRefCnt);
}

void decLvRefCnt(unsigned short delta)
{
assert(m_lvRefCnt >= delta);
m_lvRefCnt -= delta;
}

void setLvRefCnt(unsigned short newValue)
{
m_lvRefCnt = newValue;
}

unsigned lvRefCntWtd() const
{
return m_lvRefCntWtd;
}

void incLvRefCntWtd(unsigned delta)
{
unsigned oldRefCntWtd = m_lvRefCntWtd;
m_lvRefCntWtd += delta;
assert(m_lvRefCntWtd >= oldRefCntWtd);
}

void decLvRefCntWtd(unsigned delta)
{
assert(m_lvRefCntWtd >= delta);
m_lvRefCntWtd -= delta;
}

void setLvRefCntWtd(unsigned newValue)
{
m_lvRefCntWtd = newValue;
}

int lvStkOffs; // stack offset of home
unsigned lvExactSize; // (exact) size of the type in bytes

// Is this a promoted struct?
// This method returns true only for structs (including SIMD structs), not for
Expand Down
38 changes: 20 additions & 18 deletions src/jit/compiler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1798,8 +1798,8 @@ inline unsigned Compiler::lvaGrabTempWithImplicitUse(bool shortLifetime DEBUGARG
lvaSetVarAddrExposed(lclNum);

// We need lvRefCnt to be non-zero to prevent various asserts from firing.
varDsc->lvRefCnt = 1;
varDsc->lvRefCntWtd = BB_UNITY_WEIGHT;
varDsc->setLvRefCnt(1);
varDsc->setLvRefCntWtd(BB_UNITY_WEIGHT);

return lclNum;
}
Expand All @@ -1819,9 +1819,9 @@ inline void LclVarDsc::lvaResetSortAgainFlag(Compiler* comp)
comp->lvaSortAgain = true;
}
/* Set weighted ref count to zero if ref count is zero */
if (lvRefCnt == 0)
if (lvRefCnt() == 0)
{
lvRefCntWtd = 0;
setLvRefCntWtd(0);
}
}

Expand All @@ -1844,17 +1844,17 @@ inline void LclVarDsc::decRefCnts(BasicBlock::weight_t weight, Compiler* comp, b
//
if (lvType != TYP_STRUCT || promotionType != Compiler::PROMOTION_TYPE_INDEPENDENT)
{
assert(lvRefCnt); // Can't decrement below zero
assert(lvRefCnt()); // Can't decrement below zero

// TODO: Well, the assert above could be bogus.
// If lvRefCnt has overflowed before, then might drop to 0.
// Therefore we do need the following check to keep lvRefCnt from underflow:
if (lvRefCnt > 0)
if (lvRefCnt() > 0)
{
//
// Decrement lvRefCnt
//
lvRefCnt--;
decLvRefCnt(1);

//
// Decrement lvRefCntWtd
Expand All @@ -1866,13 +1866,13 @@ inline void LclVarDsc::decRefCnts(BasicBlock::weight_t weight, Compiler* comp, b
weight *= 2;
}

if (lvRefCntWtd <= weight)
if (lvRefCntWtd() <= weight)
{ // Can't go below zero
lvRefCntWtd = 0;
setLvRefCntWtd(0);
}
else
{
lvRefCntWtd -= weight;
decLvRefCntWtd(weight);
}
}
}
Expand Down Expand Up @@ -1910,7 +1910,8 @@ inline void LclVarDsc::decRefCnts(BasicBlock::weight_t weight, Compiler* comp, b
{
unsigned varNum = (unsigned)(this - comp->lvaTable);
assert(&comp->lvaTable[varNum] == this);
printf("New refCnts for V%02u: refCnt = %2u, refCntWtd = %s\n", varNum, lvRefCnt, refCntWtd2str(lvRefCntWtd));
printf("New refCnts for V%02u: refCnt = %2u, refCntWtd = %s\n", varNum, lvRefCnt(),
refCntWtd2str(lvRefCntWtd()));
}
#endif
}
Expand All @@ -1936,10 +1937,10 @@ inline void LclVarDsc::incRefCnts(BasicBlock::weight_t weight, Compiler* comp, b
//
// Increment lvRefCnt
//
int newRefCnt = lvRefCnt + 1;
int newRefCnt = lvRefCnt() + 1;
if (newRefCnt == (unsigned short)newRefCnt) // lvRefCnt is an "unsigned short". Don't overflow it.
{
lvRefCnt = (unsigned short)newRefCnt;
setLvRefCnt((unsigned short)newRefCnt);
}

// This fires when an uninitialize value for 'weight' is used (see lvaMarkRefsWeight)
Expand All @@ -1956,14 +1957,14 @@ inline void LclVarDsc::incRefCnts(BasicBlock::weight_t weight, Compiler* comp, b
weight *= 2;
}

unsigned newWeight = lvRefCntWtd + weight;
if (newWeight >= lvRefCntWtd)
unsigned newWeight = lvRefCntWtd() + weight;
if (newWeight >= lvRefCntWtd())
{ // lvRefCntWtd is an "unsigned". Don't overflow it
lvRefCntWtd = newWeight;
setLvRefCntWtd(newWeight);
}
else
{ // On overflow we assign ULONG_MAX
lvRefCntWtd = ULONG_MAX;
setLvRefCntWtd(ULONG_MAX);
}
}
}
Expand Down Expand Up @@ -2000,7 +2001,8 @@ inline void LclVarDsc::incRefCnts(BasicBlock::weight_t weight, Compiler* comp, b
{
unsigned varNum = (unsigned)(this - comp->lvaTable);
assert(&comp->lvaTable[varNum] == this);
printf("New refCnts for V%02u: refCnt = %2u, refCntWtd = %s\n", varNum, lvRefCnt, refCntWtd2str(lvRefCntWtd));
printf("New refCnts for V%02u: refCnt = %2u, refCntWtd = %s\n", varNum, lvRefCnt(),
refCntWtd2str(lvRefCntWtd()));
}
#endif
}
Expand Down
2 changes: 1 addition & 1 deletion src/jit/emit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4656,7 +4656,7 @@ unsigned emitter::emitEndCodeGen(Compiler* comp,

assert(!dsc->lvRegister);
assert(dsc->lvTracked);
assert(dsc->lvRefCnt != 0);
assert(dsc->lvRefCnt() != 0);

assert(dsc->TypeGet() == TYP_REF || dsc->TypeGet() == TYP_BYREF);

Expand Down
4 changes: 2 additions & 2 deletions src/jit/gcencode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2240,7 +2240,7 @@ size_t GCInfo::gcMakeRegPtrTable(BYTE* dest, int mask, const InfoHdr& header, un
/* If this non-enregistered pointer arg is never
* used, we don't need to report it
*/
assert(varDsc->lvRefCnt == 0); // This assert is currently a known issue for X86-RyuJit
assert(varDsc->lvRefCnt() == 0); // This assert is currently a known issue for X86-RyuJit
continue;
}
else if (varDsc->lvIsRegArg && varDsc->lvTracked)
Expand Down Expand Up @@ -4220,7 +4220,7 @@ void GCInfo::gcMakeRegPtrTable(
{
// If this non-enregistered pointer arg is never
// used, we don't need to report it.
assert(varDsc->lvRefCnt == 0);
assert(varDsc->lvRefCnt() == 0);
continue;
}
else if (varDsc->lvIsRegArg && varDsc->lvTracked)
Expand Down
2 changes: 1 addition & 1 deletion src/jit/gcinfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -427,7 +427,7 @@ void GCInfo::gcCountForHeader(UNALIGNED unsigned int* untrackedCount, UNALIGNED
/* If this non-enregistered pointer arg is never
* used, we don't need to report it
*/
assert(varDsc->lvRefCnt == 0);
assert(varDsc->lvRefCnt() == 0);
continue;
}
else if (varDsc->lvIsRegArg && varDsc->lvTracked)
Expand Down
2 changes: 1 addition & 1 deletion src/jit/gentree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2983,7 +2983,7 @@ bool Compiler::gtIsLikelyRegVar(GenTree* tree)
return false;
}

if (varDsc->lvRefCntWtd < (BB_UNITY_WEIGHT * 3))
if (varDsc->lvRefCntWtd() < (BB_UNITY_WEIGHT * 3))
{
return false;
}
Expand Down
Loading

0 comments on commit b2842bb

Please sign in to comment.