Skip to content

Commit

Permalink
Revert "JIT: Support retbuf optimization for non 'lvIsTemp' locals (d…
Browse files Browse the repository at this point in the history 
…otnet#104467)"

This reverts commit dd4b757.
  • Loading branch information
@matouskozak
matouskozak committed Jul 11, 2024
1 parent 393d667 commit 3e294d3
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 112 deletions.
4 changes: 1 addition & 3 deletions src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5516,7 +5516,6 @@ class Compiler

void fgLocalVarLivenessInit();

template <bool lowered>
void fgPerNodeLocalVarLiveness(GenTree* node);
void fgPerBlockLocalVarLiveness();

Expand All @@ -5528,7 +5527,7 @@ class Compiler

void fgLiveVarAnalysis();

void fgComputeLifeCall(VARSET_TP& life, VARSET_VALARG_TP keepAliveVars, GenTreeCall* call);
void fgComputeLifeCall(VARSET_TP& life, GenTreeCall* call);

void fgComputeLifeTrackedLocalUse(VARSET_TP& life, LclVarDsc& varDsc, GenTreeLclVarCommon* node);
bool fgComputeLifeTrackedLocalDef(VARSET_TP& life,
Expand All @@ -5550,7 +5549,6 @@ class Compiler
bool* pStmtInfoDirty DEBUGARG(bool* treeModf));

void fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALARG_TP keepAliveVars);
bool fgIsTrackedRetBufferAddress(LIR::Range& range, GenTree* node);

bool fgTryRemoveNonLocal(GenTree* node, LIR::Range* blockRange);

Expand Down
3 changes: 0 additions & 3 deletions src/coreclr/jit/gschecks.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,6 @@ void Compiler::gsParamsToShadows()
shadowVarDsc->lvDoNotEnregister = varDsc->lvDoNotEnregister;
#ifdef DEBUG
shadowVarDsc->SetDoNotEnregReason(varDsc->GetDoNotEnregReason());
shadowVarDsc->SetHiddenBufferStructArg(varDsc->IsHiddenBufferStructArg());
#endif

if (varTypeIsStruct(type))
Expand Down Expand Up @@ -504,7 +503,6 @@ void Compiler::gsParamsToShadows()
}
}

compCurBB = fgFirstBB;
// Now insert code to copy the params to their shadow copy.
for (UINT lclNum = 0; lclNum < lvaOldCount; lclNum++)
{
Expand All @@ -524,7 +522,6 @@ void Compiler::gsParamsToShadows()
compCurBB = fgFirstBB; // Needed by some morphing
(void)fgNewStmtAtBeg(fgFirstBB, fgMorphTree(store));
}
compCurBB = nullptr;

// If the method has "Jmp CalleeMethod", then we need to copy shadow params back to original
// params before "jmp" to CalleeMethod.
Expand Down
9 changes: 6 additions & 3 deletions src/coreclr/jit/lclmorph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1250,9 +1250,12 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
if (m_compiler->opts.compJitOptimizeStructHiddenBuffer && (callUser != nullptr) &&
m_compiler->IsValidLclAddr(lclNum, val.Offset()))
{
// We will only attempt this optimization for locals that do not
// later turn into indirections.
bool isSuitableLocal = varTypeIsStruct(varDsc) && !m_compiler->lvaIsImplicitByRefLocal(lclNum);
// We will only attempt this optimization for locals that are:
// a) Not susceptible to liveness bugs (see "lvaSetHiddenBufferStructArg").
// b) Do not later turn into indirections.
//
bool isSuitableLocal =
varTypeIsStruct(varDsc) && varDsc->lvIsTemp && !m_compiler->lvaIsImplicitByRefLocal(lclNum);
#ifdef TARGET_X86
if (m_compiler->lvaIsArgAccessedViaVarArgsCookie(lclNum))
{
Expand Down
6 changes: 6 additions & 0 deletions src/coreclr/jit/lclvars.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3068,6 +3068,12 @@ void Compiler::lvaSetVarAddrExposed(unsigned varNum DEBUGARG(AddressExposedReaso
// *do not* mark the local address-exposed and treat the call much like a local store node throughout
// the compilation.
//
// TODO-ADDR-Bug: currently, we rely on these locals not being present in call argument lists,
// outside of the buffer address argument itself, as liveness - currently - treats the location node
// associated with the address itself as the definition point, and call arguments can be reordered
// rather arbitrarily. We should fix liveness to treat the call as the definition point instead and
// enable this optimization for "!lvIsTemp" locals.
//
void Compiler::lvaSetHiddenBufferStructArg(unsigned varNum)
{
LclVarDsc* varDsc = lvaGetDesc(varNum);
Expand Down
114 changes: 11 additions & 103 deletions src/coreclr/jit/liveness.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,14 +192,9 @@ void Compiler::fgLocalVarLivenessInit()
// Set fgCurMemoryUse and fgCurMemoryDef when memory is read or updated
// Call fgMarkUseDef for any Local variables encountered
//
// Template arguments:
// lowered - Whether or not this is liveness on lowered IR, where LCL_ADDRs
// on tracked locals may appear.
//
// Arguments:
// tree - The current node.
//
template <bool lowered>
void Compiler::fgPerNodeLocalVarLiveness(GenTree* tree)
{
assert(tree != nullptr);
Expand All @@ -214,25 +209,12 @@ void Compiler::fgPerNodeLocalVarLiveness(GenTree* tree)

case GT_LCL_VAR:
case GT_LCL_FLD:
case GT_LCL_ADDR:
case GT_STORE_LCL_VAR:
case GT_STORE_LCL_FLD:
fgMarkUseDef(tree->AsLclVarCommon());
break;

case GT_LCL_ADDR:
if (lowered)
{
// If this is a definition of a retbuf then we process it as
// part of the GT_CALL node.
if (fgIsTrackedRetBufferAddress(LIR::AsRange(compCurBB), tree))
{
break;
}

fgMarkUseDef(tree->AsLclVarCommon());
}
break;

case GT_IND:
case GT_BLK:
// For Volatile indirection, first mutate GcHeap/ByrefExposed
Expand Down Expand Up @@ -321,12 +303,6 @@ void Compiler::fgPerNodeLocalVarLiveness(GenTree* tree)
}
}
}

GenTreeLclVarCommon* definedLcl = gtCallGetDefinedRetBufLclAddr(call);
if (definedLcl != nullptr)
{
fgMarkUseDef(definedLcl);
}
break;
}

Expand Down Expand Up @@ -391,7 +367,7 @@ void Compiler::fgPerBlockLocalVarLiveness()
{
for (GenTree* node : LIR::AsRange(block))
{
fgPerNodeLocalVarLiveness<true>(node);
fgPerNodeLocalVarLiveness(node);
}
}
else if (fgNodeThreading == NodeThreading::AllTrees)
Expand All @@ -401,7 +377,7 @@ void Compiler::fgPerBlockLocalVarLiveness()
compCurStmt = stmt;
for (GenTree* const node : stmt->TreeList())
{
fgPerNodeLocalVarLiveness<false>(node);
fgPerNodeLocalVarLiveness(node);
}
}
}
Expand Down Expand Up @@ -768,11 +744,10 @@ void Compiler::fgLiveVarAnalysis()
// due to a GT_CALL node.
//
// Arguments:
// life - The live set that is being computed.
// keepAliveVars - Tracked locals that must be kept alive everywhere in the block
// call - The call node in question.
// life - The live set that is being computed.
// call - The call node in question.
//
void Compiler::fgComputeLifeCall(VARSET_TP& life, VARSET_VALARG_TP keepAliveVars, GenTreeCall* call)
void Compiler::fgComputeLifeCall(VARSET_TP& life, GenTreeCall* call)
{
assert(call != nullptr);

Expand Down Expand Up @@ -833,12 +808,6 @@ void Compiler::fgComputeLifeCall(VARSET_TP& life, VARSET_VALARG_TP keepAliveVars
}
}
}

GenTreeLclVarCommon* definedLcl = gtCallGetDefinedRetBufLclAddr(call);
if (definedLcl != nullptr)
{
fgComputeLifeLocal(life, keepAliveVars, definedLcl);
}
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -1171,11 +1140,11 @@ void Compiler::fgComputeLife(VARSET_TP& life,
AGAIN:
assert(tree->OperGet() != GT_QMARK);

if (tree->IsCall())
if (tree->gtOper == GT_CALL)
{
fgComputeLifeCall(life, keepAliveVars, tree->AsCall());
fgComputeLifeCall(life, tree->AsCall());
}
else if (tree->OperIsNonPhiLocal())
else if (tree->OperIsNonPhiLocal() || tree->OperIs(GT_LCL_ADDR))
{
bool isDeadStore = fgComputeLifeLocal(life, keepAliveVars, tree);
if (isDeadStore)
Expand Down Expand Up @@ -1222,15 +1191,6 @@ void Compiler::fgComputeLife(VARSET_TP& life,
}
}

//---------------------------------------------------------------------
// fgComputeLifeLIR - fill out liveness flags in the IR nodes of the block
// provided the live-out set.
//
// Arguments
// life - the set of live-out variables from the block
// block - the block
// keepAliveVars - variables that are globally live (usually due to being live into an EH successor)
//
void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALARG_TP keepAliveVars)
{
noway_assert(VarSetOps::IsSubset(this, keepAliveVars, life));
Expand Down Expand Up @@ -1287,7 +1247,7 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
}
else
{
fgComputeLifeCall(life, keepAliveVars, call);
fgComputeLifeCall(life, call);
}
break;
}
Expand Down Expand Up @@ -1335,21 +1295,11 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
}
else
{
// For LCL_ADDRs that are defined by being passed as a
// retbuf we will handle them when we get to the call. We
// cannot consider them to be defined at the point of the
// LCL_ADDR since there may be uses between the LCL_ADDR
// and call.
if (fgIsTrackedRetBufferAddress(blockRange, node))
{
break;
}

isDeadStore = fgComputeLifeLocal(life, keepAliveVars, node);
if (isDeadStore)
{
LIR::Use addrUse;
if (blockRange.TryGetUse(node, &addrUse) && addrUse.User()->OperIs(GT_STOREIND, GT_STORE_BLK))
if (blockRange.TryGetUse(node, &addrUse) && (addrUse.User()->OperIs(GT_STOREIND, GT_STORE_BLK)))
{
GenTreeIndir* const store = addrUse.User()->AsIndir();

Expand Down Expand Up @@ -1515,48 +1465,6 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
}
}

//---------------------------------------------------------------------
// fgIsTrackedRetBufferAddress - given a LCL_ADDR node, check if it is the
// return buffer definition of a call.
//
// Arguments
// range - the block range containing the LCL_ADDR
// node - the LCL_ADDR
//
bool Compiler::fgIsTrackedRetBufferAddress(LIR::Range& range, GenTree* node)
{
assert(node->OperIs(GT_LCL_ADDR));
if ((node->gtFlags & GTF_VAR_DEF) == 0)
{
return false;
}

LclVarDsc* dsc = lvaGetDesc(node->AsLclVarCommon());
if (!dsc->lvTracked)
{
return false;
}

GenTree* curNode = node;
do
{
LIR::Use use;
if (!range.TryGetUse(curNode, &use))
{
return false;
}

curNode = use.User();

if (curNode->IsCall())
{
return gtCallGetDefinedRetBufLclAddr(curNode->AsCall()) == node;
}
} while (curNode->OperIs(GT_FIELD_LIST) || curNode->OperIsPutArg());

return false;
}

//---------------------------------------------------------------------
// fgTryRemoveNonLocal - try to remove a node if it is unused and has no direct
// side effects.
Expand Down

0 comments on commit 3e294d3

Please sign in to comment.