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

JIT: Stop creating "location" commas #83814

Merged
merged 7 commits into from
Mar 25, 2023
Merged

JIT: Stop creating "location" commas #83814

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6af4141
Remove IND(COMMA(x, y)) -> COMMA(x, IND(y)) transform entirely
jakobbotsch Mar 23, 2023
89910ac
Revert the "entirely" removal.
jakobbotsch Mar 23, 2023
5f19c59
Just disable the transform for locations
jakobbotsch Mar 23, 2023
ce62726
Mark STORE_DYN_BLK locations as GTF_DONT_CSE
jakobbotsch Mar 23, 2023
298c2d1
Revert post-phase checks
jakobbotsch Mar 23, 2023
768c3ff
Mitigate some TP
jakobbotsch Mar 24, 2023
2ec8474
Address feedback
jakobbotsch Mar 24, 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
83 changes: 19 additions & 64 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16237,7 +16237,6 @@ bool Compiler::gtSplitTree(
{
if (*use == m_splitNode)
{
bool userIsLocation = false;
bool userIsReturned = false;
// Split all siblings and ancestor siblings.
int i;
Expand All @@ -16254,26 +16253,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 @@ -16290,25 +16287,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 @@ -16332,7 +16326,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 @@ -16363,52 +16357,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 @@ -16421,7 +16376,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 @@ -16430,7 +16385,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
Open in desktop
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->OperIsLocal() || parent->OperIsIndir()) && ((parent->gtFlags & GTF_DONT_CSE) != 0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parent->OperIsLocal looks a bit unnecessary as LCL_VAR/LCL_FLD LHSs cannot have children, and STORE_LCL_VAR/FLD (when those start appearing here) won't need to be restricted like this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed that check.


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
Open in desktop
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.
tree->Data()->gtFlags |= GTF_DONT_CSE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're relying on this NO_CSE, seems prudent to set it in import as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to leave this here only, I think we should either set it on all locations in the importer or none of them, and today we don't set it on any of them. As you probably know we rely on this flag for the similar purpose in fgMorphLocalVar as well.

I guess today only the IND form of locations is allowed for STORE_DYN_BLK, but you could imagine if we allowed locations in general then something like STORE_DYN_BLK(LCL_VAR int, LCL_VAR int, 4) would have also been incorrectly handled without this.

}

tree->Addr() = fgMorphTree(tree->Addr());
tree->Data() = fgMorphTree(tree->Data());
tree->gtDynamicSize = fgMorphTree(tree->gtDynamicSize);
Expand Down