Skip to content

Commit

Permalink
Code review feedback; add temp cache
Browse files Browse the repository at this point in the history
  • Loading branch information
@BruceForstall
BruceForstall committed Jul 2, 2022
1 parent 92c73fe commit 7a14656
Show file tree
Hide file tree
Showing 3 changed files with 113 additions and 35 deletions.
54 changes: 53 additions & 1 deletion src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4546,7 +4546,59 @@ class Compiler

bool fgMorphBlockStmt(BasicBlock* block, Statement* stmt DEBUGARG(const char* msg));

bool fgMorphArrayOpsStmt(BasicBlock* block, Statement* stmt);
class MorphMDArrayTempCache
{
private:
class TempList
{
public:
TempList(Compiler* compiler)
: m_compiler(compiler), m_first(nullptr), m_insertPtr(&m_first), m_nextAvail(nullptr)
{
}

unsigned GetTemp();

void Reset()
{
m_nextAvail = m_first;
}

private:
struct Node
{
Node(unsigned tmp) : next(nullptr), tmp(tmp)
{
}

Node* next;
unsigned tmp;
};

Compiler* m_compiler;
Node* m_first;
Node** m_insertPtr;
Node* m_nextAvail;
};

TempList intTemps; // Temps for genActualType() == TYP_INT
TempList refTemps; // Temps for TYP_REF

public:
MorphMDArrayTempCache(Compiler* compiler) : intTemps(compiler), refTemps(compiler)
{
}

unsigned GrabTemp(var_types type);

void Reset()
{
intTemps.Reset();
refTemps.Reset();
}
};

bool fgMorphArrayOpsStmt(MorphMDArrayTempCache* pTempCache, BasicBlock* block, Statement* stmt);
PhaseStatus fgMorphArrayOps();

void fgSetOptions();
Expand Down
2 changes: 0 additions & 2 deletions src/coreclr/jit/importer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6320,15 +6320,13 @@ GenTree* Compiler::impArrayAccessIntrinsic(
{
// The indices should be converted to `int` type, as they would be if the intrinsic was not expanded.
GenTree* argVal = impPopStack().val;
#ifdef DEBUG
if (impInlineRoot()->opts.compJitEarlyExpandMDArrays)
{
// This is only enabled when early MD expansion is set because it causes small
// asm diffs (only in some test cases) otherwise. The GT_ARR_ELEM lowering code "accidentally" does
// this cast, but the new code requires it to be explicit.
argVal = impImplicitIorI4Cast(argVal, TYP_INT);
}
#endif // DEBUG
inds[k - 1] = argVal;
}

Expand Down
92 changes: 60 additions & 32 deletions src/coreclr/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17532,6 +17532,40 @@ GenTree* Compiler::fgMorphReduceAddOps(GenTree* tree)
return morphed;
}

unsigned Compiler::MorphMDArrayTempCache::TempList::GetTemp()
{
if (m_nextAvail != nullptr)
{
unsigned tmp = m_nextAvail->tmp;
JITDUMP("Reusing temp V%02u\n", tmp);
m_nextAvail = m_nextAvail->next;
return tmp;
}
else
{
unsigned newTmp = m_compiler->lvaGrabTemp(true DEBUGARG("MD array shared temp"));
Node* newNode = new (m_compiler, CMK_Unknown) Node(newTmp);
assert(m_insertPtr != nullptr);
assert(*m_insertPtr == nullptr);
*m_insertPtr = newNode;
m_insertPtr = &newNode->next;
return newTmp;
}
}

unsigned Compiler::MorphMDArrayTempCache::GrabTemp(var_types type)
{
switch (genActualType(type))
{
case TYP_INT:
return intTemps.GetTemp();
case TYP_REF:
return refTemps.GetTemp();
default:
unreached();
}
}

//------------------------------------------------------------------------
// fgMorphArrayOpsStmt: Tree walk a statement to morph GT_ARR_ELEM.
//
Expand All @@ -17546,7 +17580,7 @@ GenTree* Compiler::fgMorphReduceAddOps(GenTree* tree)
// Returns:
// True if anything changed, false if the IR was unchanged.
//
bool Compiler::fgMorphArrayOpsStmt(BasicBlock* block, Statement* stmt)
bool Compiler::fgMorphArrayOpsStmt(MorphMDArrayTempCache* pTempCache, BasicBlock* block, Statement* stmt)
{
class MorphMDArrayVisitor final : public GenTreeVisitor<MorphMDArrayVisitor>
{
Expand All @@ -17556,8 +17590,8 @@ bool Compiler::fgMorphArrayOpsStmt(BasicBlock* block, Statement* stmt)
DoPostOrder = true
};

MorphMDArrayVisitor(Compiler* compiler, BasicBlock* block)
: GenTreeVisitor<MorphMDArrayVisitor>(compiler), m_changed(false), m_block(block)
MorphMDArrayVisitor(Compiler* compiler, BasicBlock* block, MorphMDArrayTempCache* pTempCache)
: GenTreeVisitor<MorphMDArrayVisitor>(compiler), m_changed(false), m_block(block), m_pTempCache(pTempCache)
{
}

Expand Down Expand Up @@ -17620,7 +17654,8 @@ bool Compiler::fgMorphArrayOpsStmt(BasicBlock* block, Statement* stmt)
else
{
// Side-effect; create a temp.
unsigned newIdxLcl = m_compiler->lvaGrabTemp(true DEBUGARG("MD array index copy"));
// unsigned newIdxLcl = m_compiler->lvaGrabTemp(true DEBUGARG("MD array index copy"));
unsigned newIdxLcl = m_pTempCache->GrabTemp(idx->TypeGet());
GenTree* newIdx = m_compiler->gtNewLclvNode(newIdxLcl, idx->TypeGet());
idxToUse[i] = newIdx;
idxToCopy[i] = newIdxLcl;
Expand All @@ -17642,7 +17677,8 @@ bool Compiler::fgMorphArrayOpsStmt(BasicBlock* block, Statement* stmt)
}
else
{
arrLcl = newArrLcl = m_compiler->lvaGrabTemp(true DEBUGARG("MD array copy"));
// arrLcl = newArrLcl = m_compiler->lvaGrabTemp(true DEBUGARG("MD array copy"));
arrLcl = newArrLcl = m_pTempCache->GrabTemp(TYP_REF);
}

GenTree* fullTree = nullptr;
Expand All @@ -17656,10 +17692,10 @@ bool Compiler::fgMorphArrayOpsStmt(BasicBlock* block, Statement* stmt)

GenTreeMDArr* const mdArrLowerBound =
m_compiler->gtNewMDArrLowerBound(m_compiler->gtNewLclvNode(arrLcl, TYP_REF), i, rank, m_block);
unsigned effIdxLcl = m_compiler->lvaGrabTemp(true DEBUGARG("MD array effective index"));
GenTree* const effIndex = m_compiler->gtNewOperNode(GT_SUB, TYP_INT, idx, mdArrLowerBound);
GenTree* const asgNode =
m_compiler->gtNewOperNode(GT_ASG, TYP_INT, m_compiler->gtNewLclvNode(effIdxLcl, TYP_INT), effIndex);
// unsigned effIdxLcl = m_compiler->lvaGrabTemp(true DEBUGARG("MD array effective index"));
unsigned effIdxLcl = m_pTempCache->GrabTemp(TYP_INT);
GenTree* const effIndex = m_compiler->gtNewOperNode(GT_SUB, TYP_INT, idx, mdArrLowerBound);
GenTree* const asgNode = m_compiler->gtNewTempAssign(effIdxLcl, effIndex);
GenTreeMDArr* const mdArrLength =
m_compiler->gtNewMDArrLen(m_compiler->gtNewLclvNode(arrLcl, TYP_REF), i, rank, m_block);
GenTreeBoundsChk* const arrBndsChk = new (m_compiler, GT_BOUNDS_CHECK)
Expand Down Expand Up @@ -17729,31 +17765,25 @@ bool Compiler::fgMorphArrayOpsStmt(BasicBlock* block, Statement* stmt)
fullExpansion = m_compiler->gtNewOperNode(GT_COMMA, fullExpansion->TypeGet(), arrLclAsg, fullExpansion);
}

// TODO: morph here? Or morph at the statement level if there are differences?

JITDUMP("fgMorphArrayOpsStmt (before remorph):\n");
DISPTREE(fullExpansion);

GenTree* morphedTree = m_compiler->fgMorphTree(fullExpansion);
DBEXEC(morphedTree != fullExpansion, morphedTree->gtDebugFlags &= ~GTF_DEBUG_NODE_MORPHED);

JITDUMP("fgMorphArrayOpsStmt (after remorph):\n");
DISPTREE(morphedTree);

*use = morphedTree;
JITDUMP("Morphing GT_ARR_ELEM (after)\n");
DISPTREE(*use);
*use = fullExpansion;
m_changed = true;

// The GT_ARR_ELEM node is no longer needed.
DEBUG_DESTROY_NODE(node);

return fgWalkResult::WALK_CONTINUE;
}

private:
bool m_changed;
BasicBlock* m_block;
bool m_changed;
BasicBlock* m_block;
MorphMDArrayTempCache* m_pTempCache;
};

MorphMDArrayVisitor morphMDArrayVisitor(this, block);
MorphMDArrayVisitor morphMDArrayVisitor(this, block, pTempCache);
morphMDArrayVisitor.WalkTree(stmt->GetRootNodePointer(), nullptr);
return morphMDArrayVisitor.Changed();
}
Expand Down Expand Up @@ -17846,24 +17876,20 @@ bool Compiler::fgMorphArrayOpsStmt(BasicBlock* block, Statement* stmt)
//
PhaseStatus Compiler::fgMorphArrayOps()
{
#ifdef DEBUG
if (!opts.compJitEarlyExpandMDArrays)
{
if (verbose)
{
printf("Early expansion of MD arrays disabled\n");
}
JITDUMP("Early expansion of MD arrays disabled\n");
return PhaseStatus::MODIFIED_NOTHING;
}
#endif

if ((optMethodFlags & OMF_HAS_MDARRAYREF) == 0)
{
JITDUMP("No multi-dimensional array references in the function\n");
return PhaseStatus::MODIFIED_NOTHING;
}

bool changed = false;
bool changed = false;
MorphMDArrayTempCache mdArrayTempCache(this);

for (BasicBlock* const block : Blocks())
{
Expand All @@ -17878,11 +17904,11 @@ PhaseStatus Compiler::fgMorphArrayOps()

for (Statement* const stmt : block->Statements())
{
if (fgMorphArrayOpsStmt(block, stmt))
if (fgMorphArrayOpsStmt(&mdArrayTempCache, block, stmt))
{
changed = true;

// REVIEW: morph again? e.g., to propagate GTF_ASG (and other?) bits up the tree?
// Morph the statement if there have been changes.

GenTree* tree = stmt->GetRootNode();
GenTree* morphedTree = fgMorphTree(tree);
Expand All @@ -17893,6 +17919,8 @@ PhaseStatus Compiler::fgMorphArrayOps()
stmt->SetRootNode(morphedTree);
}
}

mdArrayTempCache.Reset();
}

return changed ? PhaseStatus::MODIFIED_EVERYTHING : PhaseStatus::MODIFIED_NOTHING;
Expand Down

0 comments on commit 7a14656

Please sign in to comment.