Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

JIT: Generalize handling of commas in block morphing #83590

Merged
merged 4 commits into from
Mar 28, 2023
Merged

JIT: Generalize handling of commas in block morphing #83590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4512bf1
JIT: Generalize handling of commas in block morphing
jakobbotsch Mar 25, 2023
e251b2f
Run jit-format
jakobbotsch Mar 27, 2023
e6fb448
Revert RewriteSIMDIndir removal
jakobbotsch Mar 27, 2023
87aed31
Reset flags on commas taken from pool
jakobbotsch Mar 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 2 additions & 9 deletions src/coreclr/jit/fginline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,15 +393,8 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
asg = inlinee;
}

// Block morphing does not support (promoted) locals under commas, as such, instead of "COMMA(asg, lcl)" we
// do "OBJ(COMMA(asg, ADDR(LCL)))". TODO-1stClassStructs: improve block morphing and delete this workaround.
//
GenTree* lcl = m_compiler->gtNewLclvNode(lclNum, varDsc->TypeGet());
GenTree* addr = m_compiler->gtNewOperNode(GT_ADDR, TYP_I_IMPL, lcl);
addr = m_compiler->gtNewOperNode(GT_COMMA, addr->TypeGet(), asg, addr);
GenTree* obj = m_compiler->gtNewObjNode(varDsc->GetLayout(), addr);

return obj;
GenTree* lcl = m_compiler->gtNewLclvNode(lclNum, varDsc->TypeGet());
return m_compiler->gtNewOperNode(GT_COMMA, lcl->TypeGet(), asg, lcl);
}
#endif // FEATURE_MULTIREG_RET

Expand Down
279 changes: 145 additions & 134 deletions src/coreclr/jit/morphblock.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,10 @@ class MorphInitBlockHelper
return "MorphInitBlock";
}

static GenTree* MorphBlock(Compiler* comp, GenTree* tree, bool isDest);
static GenTree* MorphCommaBlock(Compiler* comp, GenTreeOp* firstComma);

private:
void TryInitFieldByField();
void TryPrimitiveInit();
void TryInitFieldByField();
void TryPrimitiveInit();
GenTree* EliminateCommas(GenTree** commaPool);

protected:
Compiler* m_comp;
Expand Down Expand Up @@ -127,6 +125,9 @@ GenTree* MorphInitBlockHelper::Morph()
{
JITDUMP("%s:\n", GetHelperName());

GenTree* commaPool;
GenTree* sideEffects = EliminateCommas(&commaPool);

PrepareDst();
PrepareSrc();
PropagateBlockAssertions();
Expand All @@ -147,12 +148,33 @@ GenTree* MorphInitBlockHelper::Morph()
{
m_result->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
}
if (m_comp->verbose)
#endif

while (sideEffects != nullptr)
{
printf("%s (after):\n", GetHelperName());
m_comp->gtDispTree(m_result);
if (commaPool != nullptr)
{
GenTree* comma = commaPool;
commaPool = commaPool->gtNext;

assert(comma->OperIs(GT_COMMA));
comma->AsOp()->gtOp1 = sideEffects;
comma->AsOp()->gtOp2 = m_result;
comma->gtFlags = (sideEffects->gtFlags | m_result->gtFlags) & GTF_ALL_EFFECT;

m_result = comma;
}
else
{
m_result = m_comp->gtNewOperNode(GT_COMMA, m_result->TypeGet(), sideEffects, m_result);
}
INDEBUG(m_result->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);

sideEffects = sideEffects->gtNext;
}
#endif // DEBUG

JITDUMP("%s (after):\n", GetHelperName());
DISPTREE(m_result);

return m_result;
}
Expand Down Expand Up @@ -317,125 +339,6 @@ void MorphInitBlockHelper::MorphStructCases()
}
}

//------------------------------------------------------------------------
// MorphBlock: Morph a block node preparatory to morphing a block assignment.
//
// Arguments:
// comp - a compiler instance;
// tree - a struct type node;
// isDest - true if this is the destination of an assignment;
//
// Return Value:
// Returns the possibly-morphed node. The caller is responsible for updating
// the parent of this node.
//
// static
GenTree* MorphInitBlockHelper::MorphBlock(Compiler* comp, GenTree* tree, bool isDest)
{
JITDUMP("MorphBlock for %s tree, before:\n", (isDest ? "dst" : "src"));
DISPTREE(tree);

assert(varTypeIsStruct(tree));

if (tree->OperIs(GT_COMMA))
{
// TODO-Cleanup: this block is not needed for not struct nodes, but
// TryPrimitiveCopy works wrong without this transformation.
tree = MorphCommaBlock(comp, tree->AsOp());
if (isDest)
{
tree->SetDoNotCSE();
}
}

assert(!tree->OperIsIndir() || varTypeIsI(genActualType(tree->AsIndir()->Addr())));

JITDUMP("MorphBlock after:\n");
DISPTREE(tree);
return tree;
}

//------------------------------------------------------------------------
// MorphCommaBlock: transform COMMA<struct>(X) as OBJ<STRUCT>(COMMA byref(ADDR(X)).
//
// Notes:
// In order to CSE and value number array index expressions and bounds checks,
// the commas in which they are contained need to match.
// The pattern is that the COMMA should be the address expression.
// Therefore, we insert a GT_ADDR just above the node, and wrap it in an obj or ind.
// TODO-1stClassStructs: Consider whether this can be improved.
// Example:
// before: [3] comma struct <- [2] comma struct <- [1] LCL_VAR struct
// after: [5] obj <- [3] comma byref <- [2] comma byref <- [4] addr byref <- [1] LCL_VAR struct
//
// static
GenTree* MorphInitBlockHelper::MorphCommaBlock(Compiler* comp, GenTreeOp* firstComma)
{
assert(firstComma->OperIs(GT_COMMA));

ArrayStack<GenTree*> commas(comp->getAllocator(CMK_ArrayStack));
for (GenTree* currComma = firstComma; currComma != nullptr && currComma->OperIs(GT_COMMA);
currComma = currComma->gtGetOp2())
{
commas.Push(currComma);
}

GenTree* lastComma = commas.Top();

GenTree* effectiveVal = lastComma->gtGetOp2();

if (!effectiveVal->OperIsIndir() && !effectiveVal->IsLocal())
{
return firstComma;
}

assert(effectiveVal == firstComma->gtEffectiveVal());

GenTree* effectiveValAddr = comp->gtNewOperNode(GT_ADDR, TYP_BYREF, effectiveVal);

INDEBUG(effectiveValAddr->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);

lastComma->AsOp()->gtOp2 = effectiveValAddr;

while (!commas.Empty())
{
GenTree* comma = commas.Pop();
comma->gtType = TYP_BYREF;

// The "IND(COMMA)" => "COMMA(IND)" transform may have set NO_CSEs on these COMMAs, clear them.
comma->ClearDoNotCSE();
comp->gtUpdateNodeSideEffects(comma);
}

const var_types blockType = effectiveVal->TypeGet();
GenTree* addr = firstComma;

GenTree* res;

if (blockType == TYP_STRUCT)
{
CORINFO_CLASS_HANDLE structHnd = comp->gtGetStructHandleIfPresent(effectiveVal);
if (structHnd == NO_CLASS_HANDLE)
{
// TODO-1stClassStructs: get rid of all such cases.
res = comp->gtNewIndir(blockType, addr);
}
else
{
res = comp->gtNewObjNode(structHnd, addr);
comp->gtSetObjGcInfo(res->AsObj());
}
}
else
{
res = comp->gtNewIndir(blockType, addr);
}

comp->gtUpdateNodeSideEffects(res);
INDEBUG(res->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
return res;
}

//------------------------------------------------------------------------
// InitFieldByField: Attempts to promote a local block init tree to a tree
// of promoted field initialization assignments.
Expand Down Expand Up @@ -663,6 +566,119 @@ void MorphInitBlockHelper::TryPrimitiveInit()
}
}

//------------------------------------------------------------------------
// EliminateCommas: Prepare for block morphing by removing commas from the
// source operand of the assignment.
//
// Parameters:
// commaPool - [out] Pool of GT_COMMA nodes linked by their gtNext nodes that
// can be used by the caller to avoid unnecessarily creating
// new commas.
//
// Returns:
// Extracted side effects, in reverse order, linked via the gtNext fields of
// the nodes.
//
// Notes:
// We have a tree like the following (note that location-valued commas are
// illegal, so there cannot be a comma on the left):
//
// ASG
// / \.
// IND COMMA
// | / \.
// B C D
//
// We'd like downstream code to just see and be expand ASG(IND(B), D).
// We will produce:
//
// COMMA
// / \.
// ASG COMMA
// / \ / \.
// tmp B C ASG
// / \.
// IND D
// |
// tmp
//
// If the ASG has GTF_REVERSE_OPS then we will produce:
//
// COMMA
// / \.
// C ASG
// / \.
// IND D
// |
// B
//
// While keeping the GTF_REVERSE_OPS.
//
// Note that the final resulting tree is created in the caller since it also
// needs to propagate side effect flags from the decomposed assignment to all
// the created commas. Therefore this function just returns a linked list of
// the side effects to be used for that purpose.
//
GenTree* MorphInitBlockHelper::EliminateCommas(GenTree** commaPool)
{
*commaPool = nullptr;

GenTree* sideEffects = nullptr;
auto addSideEffect = [&sideEffects](GenTree* sideEff) {
sideEff->gtNext = sideEffects;
sideEffects = sideEff;
};

auto addComma = [commaPool, &addSideEffect](GenTree* comma) {
addSideEffect(comma->gtGetOp1());
comma->gtNext = *commaPool;
*commaPool = comma;
};

GenTree* lhs = m_asg->gtGetOp1();
assert(lhs->OperIsIndir() || lhs->OperIsLocal());

GenTree* rhs = m_asg->gtGetOp2();

if (m_asg->IsReverseOp())
{
while (rhs->OperIs(GT_COMMA))
{
addComma(rhs);
rhs = rhs->gtGetOp2();
}
}
else
{
if (lhs->OperIsIndir() && rhs->OperIs(GT_COMMA))
{
GenTree* addr = lhs->gtGetOp1();
if (((addr->gtFlags & GTF_ALL_EFFECT) != 0) || (((rhs->gtFlags & GTF_ASG) != 0) && !addr->IsInvariant()))
{
unsigned lhsAddrLclNum = m_comp->lvaGrabTemp(true DEBUGARG("Block morph LHS addr"));

addSideEffect(m_comp->gtNewTempAssign(lhsAddrLclNum, addr));
lhs->AsUnOp()->gtOp1 = m_comp->gtNewLclvNode(lhsAddrLclNum, genActualType(addr));
m_comp->gtUpdateNodeSideEffects(lhs);
}
}

while (rhs->OperIs(GT_COMMA))
{
addComma(rhs);
rhs = rhs->gtGetOp2();
}
}

if (sideEffects != nullptr)
{
m_asg->gtOp2 = rhs;
m_comp->gtUpdateNodeSideEffects(m_asg);
}

return sideEffects;
}

class MorphCopyBlockHelper : public MorphInitBlockHelper
{
public:
Expand Down Expand Up @@ -733,12 +749,7 @@ MorphCopyBlockHelper::MorphCopyBlockHelper(Compiler* comp, GenTree* asg) : Morph
//
void MorphCopyBlockHelper::PrepareSrc()
{
GenTree* origSrc = m_asg->gtGetOp2();
m_src = MorphBlock(m_comp, origSrc, false);
if (m_src != origSrc)
{
m_asg->gtOp2 = m_src;
}
m_src = m_asg->gtGetOp2();

if (m_src->IsLocal())
{
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/rationalize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ void Rationalizer::RewriteIndir(LIR::Use& use)
// Arguments:
// use - A use of a GT_IND node of SIMD type
//
// TODO-ADDR: delete this once block morphing stops taking addresses of locals
// under COMMAs.
// TODO-ADDR: today this only exists because the xarch backend does not handle
// IND<simd12>(LCL_VAR_ADDR/LCL_FLD_ADDR) when the address is contained correctly.
//
void Rationalizer::RewriteSIMDIndir(LIR::Use& use)
{
Expand Down