Skip to content

Commit

Permalink
JIT: Generalize handling of commas in block morphing
Browse files Browse the repository at this point in the history 
Eliminate commas early in block morphing, before the rest of the
transformation needs to look at it. Do this by extracting their side
effects and adding them on top of the returned result instead. This
allows us to handle arbitrary COMMAs on destination operand in a general
manner.

Prerequisite to #83388.

Fix #1699.
  • Loading branch information
@jakobbotsch
jakobbotsch committed Mar 25, 2023
1 parent d795694 commit 4512bf1
Show file tree
Hide file tree
Showing 4 changed files with 149 additions and 209 deletions.
9 changes: 1 addition & 8 deletions src/coreclr/jit/fginline.cpp
View file
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;
return m_compiler->gtNewOperNode(GT_COMMA, lcl->TypeGet(), asg, lcl);
}
#endif // FEATURE_MULTIREG_RET

Expand Down
280 changes: 146 additions & 134 deletions src/coreclr/jit/morphblock.cpp
View file
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,34 @@ 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 &= ~GTF_ALL_EFFECT;
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 +340,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 +567,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 +750,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
Loading

0 comments on commit 4512bf1

Please sign in to comment.