Skip to content

Commit

Permalink
JIT: Stop creating "location" commas (#83814)
Browse files Browse the repository at this point in the history 
Avoid creating commas that are on the LHS of an assignment or below an ADDR node.

These primarily were created by morph which has an IND(COMMA(x, y)) -> COMMA(x, IND(y)) transformation that did not pay attention to whether it was on the left of an assignment.
The IR shape is pretty odd; the RHS of these commas are not actually evaluated in the traditional sense, but happen as part of the parent ASG, so the semantics of the construct ends up being confusing.
Additionally it complicates #83590.
  • Loading branch information
@jakobbotsch
jakobbotsch authored Mar 25, 2023
1 parent 3a82439 commit d795694
Show file tree
Hide file tree
Showing 5 changed files with 45 additions and 80 deletions.
83 changes: 19 additions & 64 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16241,7 +16241,6 @@ bool Compiler::gtSplitTree(
{
if (*use == m_splitNode)
{
bool userIsLocation = false;
bool userIsReturned = false;
// Split all siblings and ancestor siblings.
int i;
Expand All @@ -16258,26 +16257,24 @@ bool Compiler::gtSplitTree(
// that contains the split node.
if (m_useStack.BottomRef(i + 1).User == useInf.User)
{
SplitOutUse(useInf, userIsLocation, userIsReturned);
SplitOutUse(useInf, userIsReturned);
}
else
{
// This is an ancestor of the node we're splitting on.
userIsLocation = IsLocation(useInf, userIsLocation);
userIsReturned = IsReturned(useInf, userIsReturned);
}
}

assert(m_useStack.Bottom(i).Use == use);
userIsLocation = IsLocation(m_useStack.BottomRef(i), userIsLocation);
userIsReturned = IsReturned(m_useStack.BottomRef(i), userIsReturned);

// The remaining nodes should be operands of the split node.
for (i++; i < m_useStack.Height(); i++)
{
const UseInfo& useInf = m_useStack.BottomRef(i);
assert(useInf.User == *use);
SplitOutUse(useInf, userIsLocation, userIsReturned);
SplitOutUse(useInf, userIsReturned);
}

SplitNodeUse = use;
Expand All @@ -16294,25 +16291,22 @@ bool Compiler::gtSplitTree(
}

private:
bool IsLocation(const UseInfo& useInf, bool userIsLocation)
bool IsLocation(const UseInfo& useInf)
{
if (useInf.User != nullptr)
if (useInf.User == nullptr)
{
if (useInf.User->OperIs(GT_ADDR, GT_ASG) && (useInf.Use == &useInf.User->AsUnOp()->gtOp1))
{
return true;
}
return false;
}

if (useInf.User->OperIs(GT_STORE_DYN_BLK) && !(*useInf.Use)->OperIs(GT_CNS_INT, GT_INIT_VAL) &&
(useInf.Use == &useInf.User->AsStoreDynBlk()->Data()))
{
return true;
}
if (useInf.User->OperIs(GT_ADDR, GT_ASG) && (useInf.Use == &useInf.User->AsUnOp()->gtOp1))
{
return true;
}

if (userIsLocation && useInf.User->OperIs(GT_COMMA) && (useInf.Use == &useInf.User->AsOp()->gtOp2))
{
return true;
}
if (useInf.User->OperIs(GT_STORE_DYN_BLK) && !(*useInf.Use)->OperIs(GT_CNS_INT, GT_INIT_VAL) &&
(useInf.Use == &useInf.User->AsStoreDynBlk()->Data()))
{
return true;
}

return false;
Expand All @@ -16336,7 +16330,7 @@ bool Compiler::gtSplitTree(
return false;
}

void SplitOutUse(const UseInfo& useInf, bool userIsLocation, bool userIsReturned)
void SplitOutUse(const UseInfo& useInf, bool userIsReturned)
{
GenTree** use = useInf.Use;
GenTree* user = useInf.User;
Expand Down Expand Up @@ -16367,52 +16361,13 @@ bool Compiler::gtSplitTree(
return;
}

if (IsLocation(useInf, userIsLocation))
if (IsLocation(useInf))
{
if ((*use)->OperIs(GT_COMMA))
{
// We have:
// User
// COMMA
// op1
// op2
// rhs
// where the comma is a location, and we want to split it out.
//
// The first use will be the COMMA --- op1 edge, which we
// expect to be handled by simple side effect extraction in
// the recursive call.
UseInfo use1{&(*use)->AsOp()->gtOp1, *use};

// For the second use we will update the user to use op2
// directly instead of the comma so that we get the proper
// location treatment. The edge will then be the User ---
// op2 edge.
*use = (*use)->gtGetOp2();
MadeChanges = true;

UseInfo use2{use, user};

// Locations are never returned.
assert(!IsReturned(useInf, userIsReturned));
if ((*use)->IsReverseOp())
{
SplitOutUse(use2, true, false);
SplitOutUse(use1, false, false);
}
else
{
SplitOutUse(use1, false, false);
SplitOutUse(use2, true, false);
}
return;
}

// Only a handful of nodes can be location, and they are all unary or nullary.
assert((*use)->OperIs(GT_IND, GT_OBJ, GT_BLK, GT_LCL_VAR, GT_LCL_FLD));
if ((*use)->OperIsUnary())
{
SplitOutUse(UseInfo{&(*use)->AsUnOp()->gtOp1, user}, false, false);
SplitOutUse(UseInfo{&(*use)->AsUnOp()->gtOp1, user}, false);
}

return;
Expand All @@ -16425,7 +16380,7 @@ bool Compiler::gtSplitTree(
if ((user != nullptr) && user->OperIs(GT_MUL) && user->Is64RsltMul())
{
assert((*use)->OperIs(GT_CAST));
SplitOutUse(UseInfo{&(*use)->AsCast()->gtOp1, *use}, false, false);
SplitOutUse(UseInfo{&(*use)->AsCast()->gtOp1, *use}, false);
return;
}
#endif
Expand All @@ -16434,7 +16389,7 @@ bool Compiler::gtSplitTree(
{
for (GenTree** operandUse : (*use)->UseEdges())
{
SplitOutUse(UseInfo{operandUse, *use}, false, false);
SplitOutUse(UseInfo{operandUse, *use}, false);
}
return;
}
Expand Down
13 changes: 9 additions & 4 deletions src/coreclr/jit/morph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9827,12 +9827,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA

// Only do this optimization when we are in the global optimizer. Doing this after value numbering
// could result in an invalid value number for the newly generated GT_IND node.
if ((op1->OperGet() == GT_COMMA) && fgGlobalMorph)
// We skip INDs with GTF_DONT_CSE which is set if the IND is a location.
if (op1->OperIs(GT_COMMA) && fgGlobalMorph && ((tree->gtFlags & GTF_DONT_CSE) == 0))
{
// Perform the transform IND(COMMA(x, ..., z)) -> COMMA(x, ..., IND(z)).
// TBD: this transformation is currently necessary for correctness -- it might
// be good to analyze the failures that result if we don't do this, and fix them
// in other ways. Ideally, this should be optional.
GenTree* commaNode = op1;
GenTreeFlags treeFlags = tree->gtFlags;
commaNode->gtType = typ;
Expand Down Expand Up @@ -11504,6 +11502,13 @@ GenTree* Compiler::fgPropagateCommaThrow(GenTree* parent, GenTreeOp* commaThrow,
assert(fgGlobalMorph);
assert(fgIsCommaThrow(commaThrow));

bool mightBeLocation = parent->OperIsIndir() && ((parent->gtFlags & GTF_DONT_CSE) != 0);

if (mightBeLocation)
{
return nullptr;
}

if ((commaThrow->gtFlags & GTF_COLON_COND) == 0)
{
fgRemoveRestOfBlock = true;
Expand Down
18 changes: 11 additions & 7 deletions src/coreclr/jit/morphblock.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,12 +163,10 @@ GenTree* MorphInitBlockHelper::Morph()
//
void MorphInitBlockHelper::PrepareDst()
{
GenTree* origDst = m_asg->gtGetOp1();
m_dst = MorphBlock(m_comp, origDst, true);
if (m_dst != origDst)
{
m_asg->gtOp1 = m_dst;
}
m_dst = m_asg->gtGetOp1();

// Commas cannot be destinations.
assert(!m_dst->OperIs(GT_COMMA));

if (m_asg->TypeGet() != m_dst->TypeGet())
{
Expand Down Expand Up @@ -213,7 +211,7 @@ void MorphInitBlockHelper::PrepareDst()
#if defined(DEBUG)
if (m_comp->verbose)
{
printf("PrepareDst for [%06u] ", m_comp->dspTreeID(origDst));
printf("PrepareDst for [%06u] ", m_comp->dspTreeID(m_dst));
if (m_dstLclNode != nullptr)
{
printf("have found a local var V%02u.\n", m_dstLclNum);
Expand Down Expand Up @@ -1595,6 +1593,12 @@ GenTree* Compiler::fgMorphInitBlock(GenTree* tree)
//
GenTree* Compiler::fgMorphStoreDynBlock(GenTreeStoreDynBlk* tree)
{
if (!tree->Data()->OperIs(GT_CNS_INT, GT_INIT_VAL))
{
// Data is a location and required to have GTF_DONT_CSE.
tree->Data()->gtFlags |= GTF_DONT_CSE;
}

tree->Addr() = fgMorphTree(tree->Addr());
tree->Data() = fgMorphTree(tree->Data());
tree->gtDynamicSize = fgMorphTree(tree->gtDynamicSize);
Expand Down
6 changes: 4 additions & 2 deletions src/coreclr/jit/optimizer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8617,9 +8617,11 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk)

if (oper == GT_ASG)
{
GenTree* lhs = tree->AsOp()->gtOp1->gtEffectiveVal(/*commaOnly*/ true);
GenTree* lhs = tree->gtGetOp1();

if (lhs->OperGet() == GT_IND)
assert(!lhs->OperIs(GT_COMMA));

if (lhs->OperIs(GT_IND))
{
GenTree* arg = lhs->AsOp()->gtOp1->gtEffectiveVal(/*commaOnly*/ true);

Expand Down
5 changes: 2 additions & 3 deletions src/coreclr/jit/valuenum.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9839,9 +9839,8 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree)
}
}

// We have to handle the case where the LHS is a comma. In that case, we don't evaluate the comma,
// and we're really just interested in the effective value.
lhs = lhs->gtEffectiveVal();
// Locations are not allowed to be COMMAs.
assert(!lhs->OperIs(GT_COMMA));

// Now, record the new VN for an assignment (performing the indicated "state update").
// It's safe to use gtEffectiveVal here, because the non-last elements of a comma list on the
Expand Down

0 comments on commit d795694

Please sign in to comment.