Skip to content

Commit d795694

Browse files
jakobbotschjakobbotsch
authored
JIT: Stop creating "location" commas (#83814)
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.
1 parent 3a82439 commit d795694

File tree

5 files changed

+45
-80
lines changed

5 files changed

+45
-80
lines changed

‎src/coreclr/jit/gentree.cpp

Lines changed: 19 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -16241,7 +16241,6 @@ bool Compiler::gtSplitTree(
1624116241
{
1624216242
if (*use == m_splitNode)
1624316243
{
16244-
bool userIsLocation = false;
1624516244
bool userIsReturned = false;
1624616245
// Split all siblings and ancestor siblings.
1624716246
int i;
@@ -16258,26 +16257,24 @@ bool Compiler::gtSplitTree(
1625816257
// that contains the split node.
1625916258
if (m_useStack.BottomRef(i + 1).User == useInf.User)
1626016259
{
16261-
SplitOutUse(useInf, userIsLocation, userIsReturned);
16260+
SplitOutUse(useInf, userIsReturned);
1626216261
}
1626316262
else
1626416263
{
1626516264
// This is an ancestor of the node we're splitting on.
16266-
userIsLocation = IsLocation(useInf, userIsLocation);
1626716265
userIsReturned = IsReturned(useInf, userIsReturned);
1626816266
}
1626916267
}
1627016268

1627116269
assert(m_useStack.Bottom(i).Use == use);
16272-
userIsLocation = IsLocation(m_useStack.BottomRef(i), userIsLocation);
1627316270
userIsReturned = IsReturned(m_useStack.BottomRef(i), userIsReturned);
1627416271

1627516272
// The remaining nodes should be operands of the split node.
1627616273
for (i++; i < m_useStack.Height(); i++)
1627716274
{
1627816275
const UseInfo& useInf = m_useStack.BottomRef(i);
1627916276
assert(useInf.User == *use);
16280-
SplitOutUse(useInf, userIsLocation, userIsReturned);
16277+
SplitOutUse(useInf, userIsReturned);
1628116278
}
1628216279

1628316280
SplitNodeUse = use;
@@ -16294,25 +16291,22 @@ bool Compiler::gtSplitTree(
1629416291
}
1629516292

1629616293
private:
16297-
bool IsLocation(const UseInfo& useInf, bool userIsLocation)
16294+
bool IsLocation(const UseInfo& useInf)
1629816295
{
16299-
if (useInf.User != nullptr)
16296+
if (useInf.User == nullptr)
1630016297
{
16301-
if (useInf.User->OperIs(GT_ADDR, GT_ASG) && (useInf.Use == &useInf.User->AsUnOp()->gtOp1))
16302-
{
16303-
return true;
16304-
}
16298+
return false;
16299+
}
1630516300

16306-
if (useInf.User->OperIs(GT_STORE_DYN_BLK) && !(*useInf.Use)->OperIs(GT_CNS_INT, GT_INIT_VAL) &&
16307-
(useInf.Use == &useInf.User->AsStoreDynBlk()->Data()))
16308-
{
16309-
return true;
16310-
}
16301+
if (useInf.User->OperIs(GT_ADDR, GT_ASG) && (useInf.Use == &useInf.User->AsUnOp()->gtOp1))
16302+
{
16303+
return true;
16304+
}
1631116305

16312-
if (userIsLocation && useInf.User->OperIs(GT_COMMA) && (useInf.Use == &useInf.User->AsOp()->gtOp2))
16313-
{
16314-
return true;
16315-
}
16306+
if (useInf.User->OperIs(GT_STORE_DYN_BLK) && !(*useInf.Use)->OperIs(GT_CNS_INT, GT_INIT_VAL) &&
16307+
(useInf.Use == &useInf.User->AsStoreDynBlk()->Data()))
16308+
{
16309+
return true;
1631616310
}
1631716311

1631816312
return false;
@@ -16336,7 +16330,7 @@ bool Compiler::gtSplitTree(
1633616330
return false;
1633716331
}
1633816332

16339-
void SplitOutUse(const UseInfo& useInf, bool userIsLocation, bool userIsReturned)
16333+
void SplitOutUse(const UseInfo& useInf, bool userIsReturned)
1634016334
{
1634116335
GenTree** use = useInf.Use;
1634216336
GenTree* user = useInf.User;
@@ -16367,52 +16361,13 @@ bool Compiler::gtSplitTree(
1636716361
return;
1636816362
}
1636916363

16370-
if (IsLocation(useInf, userIsLocation))
16364+
if (IsLocation(useInf))
1637116365
{
16372-
if ((*use)->OperIs(GT_COMMA))
16373-
{
16374-
// We have:
16375-
// User
16376-
// COMMA
16377-
// op1
16378-
// op2
16379-
// rhs
16380-
// where the comma is a location, and we want to split it out.
16381-
//
16382-
// The first use will be the COMMA --- op1 edge, which we
16383-
// expect to be handled by simple side effect extraction in
16384-
// the recursive call.
16385-
UseInfo use1{&(*use)->AsOp()->gtOp1, *use};
16386-
16387-
// For the second use we will update the user to use op2
16388-
// directly instead of the comma so that we get the proper
16389-
// location treatment. The edge will then be the User ---
16390-
// op2 edge.
16391-
*use = (*use)->gtGetOp2();
16392-
MadeChanges = true;
16393-
16394-
UseInfo use2{use, user};
16395-
16396-
// Locations are never returned.
16397-
assert(!IsReturned(useInf, userIsReturned));
16398-
if ((*use)->IsReverseOp())
16399-
{
16400-
SplitOutUse(use2, true, false);
16401-
SplitOutUse(use1, false, false);
16402-
}
16403-
else
16404-
{
16405-
SplitOutUse(use1, false, false);
16406-
SplitOutUse(use2, true, false);
16407-
}
16408-
return;
16409-
}
16410-
1641116366
// Only a handful of nodes can be location, and they are all unary or nullary.
1641216367
assert((*use)->OperIs(GT_IND, GT_OBJ, GT_BLK, GT_LCL_VAR, GT_LCL_FLD));
1641316368
if ((*use)->OperIsUnary())
1641416369
{
16415-
SplitOutUse(UseInfo{&(*use)->AsUnOp()->gtOp1, user}, false, false);
16370+
SplitOutUse(UseInfo{&(*use)->AsUnOp()->gtOp1, user}, false);
1641616371
}
1641716372

1641816373
return;
@@ -16425,7 +16380,7 @@ bool Compiler::gtSplitTree(
1642516380
if ((user != nullptr) && user->OperIs(GT_MUL) && user->Is64RsltMul())
1642616381
{
1642716382
assert((*use)->OperIs(GT_CAST));
16428-
SplitOutUse(UseInfo{&(*use)->AsCast()->gtOp1, *use}, false, false);
16383+
SplitOutUse(UseInfo{&(*use)->AsCast()->gtOp1, *use}, false);
1642916384
return;
1643016385
}
1643116386
#endif
@@ -16434,7 +16389,7 @@ bool Compiler::gtSplitTree(
1643416389
{
1643516390
for (GenTree** operandUse : (*use)->UseEdges())
1643616391
{
16437-
SplitOutUse(UseInfo{operandUse, *use}, false, false);
16392+
SplitOutUse(UseInfo{operandUse, *use}, false);
1643816393
}
1643916394
return;
1644016395
}

‎src/coreclr/jit/morph.cpp

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9827,12 +9827,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optA
98279827

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

11505+
bool mightBeLocation = parent->OperIsIndir() && ((parent->gtFlags & GTF_DONT_CSE) != 0);
11506+
11507+
if (mightBeLocation)
11508+
{
11509+
return nullptr;
11510+
}
11511+
1150711512
if ((commaThrow->gtFlags & GTF_COLON_COND) == 0)
1150811513
{
1150911514
fgRemoveRestOfBlock = true;

‎src/coreclr/jit/morphblock.cpp

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -163,12 +163,10 @@ GenTree* MorphInitBlockHelper::Morph()
163163
//
164164
void MorphInitBlockHelper::PrepareDst()
165165
{
166-
GenTree* origDst = m_asg->gtGetOp1();
167-
m_dst = MorphBlock(m_comp, origDst, true);
168-
if (m_dst != origDst)
169-
{
170-
m_asg->gtOp1 = m_dst;
171-
}
166+
m_dst = m_asg->gtGetOp1();
167+
168+
// Commas cannot be destinations.
169+
assert(!m_dst->OperIs(GT_COMMA));
172170

173171
if (m_asg->TypeGet() != m_dst->TypeGet())
174172
{
@@ -213,7 +211,7 @@ void MorphInitBlockHelper::PrepareDst()
213211
#if defined(DEBUG)
214212
if (m_comp->verbose)
215213
{
216-
printf("PrepareDst for [%06u] ", m_comp->dspTreeID(origDst));
214+
printf("PrepareDst for [%06u] ", m_comp->dspTreeID(m_dst));
217215
if (m_dstLclNode != nullptr)
218216
{
219217
printf("have found a local var V%02u.\n", m_dstLclNum);
@@ -1595,6 +1593,12 @@ GenTree* Compiler::fgMorphInitBlock(GenTree* tree)
15951593
//
15961594
GenTree* Compiler::fgMorphStoreDynBlock(GenTreeStoreDynBlk* tree)
15971595
{
1596+
if (!tree->Data()->OperIs(GT_CNS_INT, GT_INIT_VAL))
1597+
{
1598+
// Data is a location and required to have GTF_DONT_CSE.
1599+
tree->Data()->gtFlags |= GTF_DONT_CSE;
1600+
}
1601+
15981602
tree->Addr() = fgMorphTree(tree->Addr());
15991603
tree->Data() = fgMorphTree(tree->Data());
16001604
tree->gtDynamicSize = fgMorphTree(tree->gtDynamicSize);

‎src/coreclr/jit/optimizer.cpp

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8617,9 +8617,11 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk)
86178617

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

8622-
if (lhs->OperGet() == GT_IND)
8622+
assert(!lhs->OperIs(GT_COMMA));
8623+
8624+
if (lhs->OperIs(GT_IND))
86238625
{
86248626
GenTree* arg = lhs->AsOp()->gtOp1->gtEffectiveVal(/*commaOnly*/ true);
86258627

‎src/coreclr/jit/valuenum.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9839,9 +9839,8 @@ void Compiler::fgValueNumberAssignment(GenTreeOp* tree)
98399839
}
98409840
}
98419841

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

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

0 commit comments

Comments
 (0)