Skip to content

Commit

Permalink
Add flag for EnableLoopHoistInNestedLoops
Browse files Browse the repository at this point in the history
  • Loading branch information
@kunalspathak
kunalspathak committed Aug 2, 2022
1 parent e415018 commit a29daa4
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 33 deletions.
1 change: 1 addition & 0 deletions src/coreclr/jit/compiler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2435,6 +2435,7 @@ void Compiler::compInitOptions(JitFlags* jitFlags)
opts.compJitAlignLoopMaxCodeSize = (unsigned short)JitConfig.JitAlignLoopMaxCodeSize();
opts.compJitHideAlignBehindJmp = JitConfig.JitHideAlignBehindJmp() == 1;
opts.compJitOptimizeStructHiddenBuffer = JitConfig.JitOptimizeStructHiddenBuffer() == 1;
opts.compJitEnableLoopHoistInNestedLoops = JitConfig.JitEnableLoopHoistInNestedLoop() == 1;
#else
opts.compJitAlignLoopAdaptive = true;
opts.compJitAlignLoopBoundary = DEFAULT_ALIGN_LOOP_BOUNDARY;
Expand Down
16 changes: 15 additions & 1 deletion src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5968,6 +5968,9 @@ class Compiler
VNSet* m_pHoistedInCurLoop;

public:
// Value numbers of expressions that have been hoisted in parent loops in the loop nest.
VNSet m_hoistedInParentLoops;

// Value numbers of expressions that have been hoisted in the current (or most recent) loop in the nest.
// Previous decisions on loop-invariance of value numbers in the current loop.
VNSet m_curLoopVnInvariantCache;
Expand All @@ -5982,6 +5985,13 @@ class Compiler
return m_pHoistedInCurLoop;
}

VNSet* ExtractHoistedInCurLoop()
{
VNSet* res = m_pHoistedInCurLoop;
m_pHoistedInCurLoop = nullptr;
return res;
}

// Return the so far collected VNs in cache for current loop and reset it.
void ResetHoistedInCurLoop()
{
Expand All @@ -5990,7 +6000,9 @@ class Compiler
}

LoopHoistContext(Compiler* comp)
: m_pHoistedInCurLoop(nullptr), m_curLoopVnInvariantCache(comp->getAllocatorLoopHoist())
: m_pHoistedInCurLoop(nullptr)
, m_hoistedInParentLoops(comp->getAllocatorLoopHoist())
, m_curLoopVnInvariantCache(comp->getAllocatorLoopHoist())
{
}
};
Expand Down Expand Up @@ -9236,6 +9248,8 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
// If set, tracks the hidden return buffer for struct arg.
bool compJitOptimizeStructHiddenBuffer;

bool compJitEnableLoopHoistInNestedLoops;

#ifdef LATE_DISASM
bool doLateDisasm; // Run the late disassembler
#endif // LATE_DISASM
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/jitconfigvalues.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ CONFIG_INTEGER(JitHideAlignBehindJmp,
CONFIG_INTEGER(JitOptimizeStructHiddenBuffer, W("JitOptimizeStructHiddenBuffer"), 1) // Track assignments to locals done
// through return buffers.

CONFIG_INTEGER(JitEnableLoopHoistInNestedLoop, W("JitEnableLoopHoistInNestedLoop"), 1)

// Print the alignment boundaries in disassembly.
CONFIG_INTEGER(JitDasmWithAlignmentBoundaries, W("JitDasmWithAlignmentBoundaries"), 0)

Expand Down
120 changes: 88 additions & 32 deletions src/coreclr/jit/optimizer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6442,7 +6442,7 @@ void Compiler::optPerformHoistExpr(GenTree* origExpr, BasicBlock* exprBb, unsign
}

//------------------------------------------------------------------------
// optHoistLoopNest: run loop hoisting for indicated loop and all contained loops
// optHoistLoopCode: run loop hoisting for indicated loop and all contained loops
//
// Returns:
// suitable phase status
Expand Down Expand Up @@ -6573,42 +6573,84 @@ bool Compiler::optHoistLoopNest(unsigned lnum, LoopHoistContext* hoistCtxt)
m_curLoopHasHoistedExpression = false;
m_loopsConsidered++;
#endif // LOOP_HOIST_STATS

BasicBlockList* firstPreHeader = nullptr;
BasicBlockList** preHeaderAppendPtr = &firstPreHeader;

bool modified = false;

if (optLoopTable[lnum].lpChild != BasicBlock::NOT_IN_LOOP)
if (!opts.compJitEnableLoopHoistInNestedLoops)
{
for (unsigned child = optLoopTable[lnum].lpChild; child != BasicBlock::NOT_IN_LOOP;
child = optLoopTable[child].lpSibling)
{
modified |= optHoistLoopNest(child, hoistCtxt);
modified |= optHoistThisLoop(lnum, hoistCtxt, nullptr);
VNSet* hoistedInCurLoop = hoistCtxt->ExtractHoistedInCurLoop();

if (optLoopTable[child].lpFlags & LPFLG_HAS_PREHEAD)
if (optLoopTable[lnum].lpChild != BasicBlock::NOT_IN_LOOP)
{
// Add the ones hoisted in "lnum" to "hoistedInParents" for any nested loops.
// TODO-Cleanup: we should have a set abstraction for loops.
if (hoistedInCurLoop != nullptr)
{
// If a pre-header is found, add it to the tracking list. Most likely, the recursive call
// to hoist from the `child` loop nest found something to hoist and created a pre-header
// where the hoisted code was placed.
for (VNSet::KeyIterator keys = hoistedInCurLoop->Begin(); !keys.Equal(hoistedInCurLoop->End()); ++keys)
{
#ifdef DEBUG
bool b;
assert(!hoistCtxt->m_hoistedInParentLoops.Lookup(keys.Get(), &b));
#endif
hoistCtxt->m_hoistedInParentLoops.Set(keys.Get(), true);
}
}

BasicBlock* preHeaderBlock = optLoopTable[child].lpHead;
JITDUMP(" PREHEADER: " FMT_BB "\n", preHeaderBlock->bbNum);
for (unsigned child = optLoopTable[lnum].lpChild; child != BasicBlock::NOT_IN_LOOP;
child = optLoopTable[child].lpSibling)
{
modified |= optHoistLoopNest(child, hoistCtxt);
}

// Here, we are arranging the blocks in reverse lexical order, so when they are removed from the
// head of this list and pushed on the stack of hoisting blocks to process, they are in
// forward lexical order when popped. Note that for child loops `i` and `j`, in order `i`
// followed by `j` in the `lpSibling` list, that `j` is actually first in lexical order
// and that makes these blocks arranged in reverse lexical order in this list.
// That is, the sibling list is in reverse lexical order.
// Note: the sibling list order should not matter to any algorithm; this order is not guaranteed.
*preHeaderAppendPtr = new (this, CMK_LoopHoist) BasicBlockList(preHeaderBlock, nullptr);
preHeaderAppendPtr = &((*preHeaderAppendPtr)->next);
// Now remove them.
// TODO-Cleanup: we should have a set abstraction for loops.
if (hoistedInCurLoop != nullptr)
{
for (VNSet::KeyIterator keys = hoistedInCurLoop->Begin(); !keys.Equal(hoistedInCurLoop->End()); ++keys)
{
// Note that we asserted when we added these that they hadn't been members, so removing is
// appropriate.
hoistCtxt->m_hoistedInParentLoops.Remove(keys.Get());
}
}
}
}
else
{
BasicBlockList* firstPreHeader = nullptr;
BasicBlockList** preHeaderAppendPtr = &firstPreHeader;

modified |= optHoistThisLoop(lnum, hoistCtxt, firstPreHeader);

if (optLoopTable[lnum].lpChild != BasicBlock::NOT_IN_LOOP)
{
for (unsigned child = optLoopTable[lnum].lpChild; child != BasicBlock::NOT_IN_LOOP;
child = optLoopTable[child].lpSibling)
{
modified |= optHoistLoopNest(child, hoistCtxt);

if (optLoopTable[child].lpFlags & LPFLG_HAS_PREHEAD)
{
// If a pre-header is found, add it to the tracking list. Most likely, the recursive call
// to hoist from the `child` loop nest found something to hoist and created a pre-header
// where the hoisted code was placed.

BasicBlock* preHeaderBlock = optLoopTable[child].lpHead;
JITDUMP(" PREHEADER: " FMT_BB "\n", preHeaderBlock->bbNum);

// Here, we are arranging the blocks in reverse lexical order, so when they are removed from the
// head of this list and pushed on the stack of hoisting blocks to process, they are in
// forward lexical order when popped. Note that for child loops `i` and `j`, in order `i`
// followed by `j` in the `lpSibling` list, that `j` is actually first in lexical order
// and that makes these blocks arranged in reverse lexical order in this list.
// That is, the sibling list is in reverse lexical order.
// Note: the sibling list order should not matter to any algorithm; this order is not guaranteed.
*preHeaderAppendPtr = new (this, CMK_LoopHoist) BasicBlockList(preHeaderBlock, nullptr);
preHeaderAppendPtr = &((*preHeaderAppendPtr)->next);
}
}
}

modified |= optHoistThisLoop(lnum, hoistCtxt, firstPreHeader);
}

return modified;
}
Expand Down Expand Up @@ -7266,9 +7308,9 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
// and the variable must be in SSA ...
isInvariant = isInvariant && m_compiler->lvaInSsa(lclNum) && lclVar->HasSsaName();
// and the SSA definition must be outside the loop we're hoisting from ...
isInvariant = isInvariant &&
!m_compiler->optLoopTable[m_loopNum].lpContains(
m_compiler->lvaGetDesc(lclNum)->GetPerSsaData(lclVar->GetSsaNum())->GetBlock());
isInvariant =
isInvariant && !m_compiler->optLoopTable[m_loopNum].lpContains(
m_compiler->lvaGetDesc(lclNum)->GetPerSsaData(lclVar->GetSsaNum())->GetBlock());
// and the VN of the tree is considered invariant as well.
//
// TODO-CQ: This VN invariance check should not be necessary and in some cases it is conservative - it
Expand Down Expand Up @@ -7582,7 +7624,8 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
{
assert(value.Node() != tree);

if (IsHoistableOverExcepSibling(value.Node(), hasExcep))
if (!m_compiler->opts.compJitEnableLoopHoistInNestedLoops ||
IsHoistableOverExcepSibling(value.Node(), hasExcep))
{
m_compiler->optHoistCandidate(value.Node(), m_currentBlock, m_loopNum, m_hoistContext);
}
Expand Down Expand Up @@ -7652,7 +7695,10 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
visitor.HoistBlock(block);
}

hoistContext->ResetHoistedInCurLoop();
if (opts.compJitEnableLoopHoistInNestedLoops)
{
hoistContext->ResetHoistedInCurLoop();
}
}

void Compiler::optHoistCandidate(GenTree* tree, BasicBlock* treeBb, unsigned lnum, LoopHoistContext* hoistCtxt)
Expand All @@ -7666,6 +7712,16 @@ void Compiler::optHoistCandidate(GenTree* tree, BasicBlock* treeBb, unsigned lnu
return;
}

if (!opts.compJitEnableLoopHoistInNestedLoops)
{
if (hoistCtxt->m_hoistedInParentLoops.Lookup(tree->gtVNPair.GetLiberal()))
{
JITDUMP(" ... already hoisted same VN in parent\n");
// already hoisted in a parent loop, so don't hoist this expression.
return;
}
}

if (hoistCtxt->GetHoistedInCurLoop(this)->Lookup(tree->gtVNPair.GetLiberal()))
{
// already hoisted this expression in the current loop, so don't hoist this expression.
Expand Down

0 comments on commit a29daa4

Please sign in to comment.