Skip to content

Remove GTF_GLOB_REF on a few more addressing nodes #68741

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

Merged
jakobbotsch merged 3 commits into from
May 2, 2022
Merged

Remove GTF_GLOB_REF on a few more addressing nodes #68741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c5f25f3
Remove unnecessary GTF_GLOB_REF on locals
jakobbotsch May 1, 2022
840ff24
Improve assertion for ret-buf as local optimization
jakobbotsch May 1, 2022
0a74438
Readd GT_ADDR check
jakobbotsch May 1, 2022
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
4 changes: 2 additions & 2 deletions src/coreclr/jit/fgdiagnostic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3085,8 +3085,8 @@ void Compiler::fgDebugCheckFlags(GenTree* tree)
return GenTree::VisitResult::Continue;
});

// ADDR nodes break the "parent flags >= operands flags" invariant for GTF_GLOB_REF.
if (tree->OperIs(GT_ADDR) && op1->OperIs(GT_LCL_VAR, GT_LCL_FLD))
// Addresses of locals never need GTF_GLOB_REF
Copy link
Contributor

@SingleAccretion SingleAccretion May 1, 2022

Choose a reason for hiding this comment

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

Nit: could you keep the mention of the ADDR in the comment (here and in morph)?

It makes it stand out as the quirk this special handling is (and will help find places that need to be updated when ADDR is deleted).

Copy link
Member Author

@jakobbotsch jakobbotsch May 1, 2022

Choose a reason for hiding this comment

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

I've readded the check for GT_ADDR, I don't believe we need to handle other cases anyway as they will just be handled when it's their turn in morph anyway.

if (tree->OperIs(GT_ADDR) && tree->IsLocalAddrExpr())
{
expectedFlags &= ~GTF_GLOB_REF;
}
Expand Down
27 changes: 16 additions & 11 deletions src/coreclr/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1769,17 +1769,19 @@ void CallArgs::EvalArgsToTemps(Compiler* comp, GenTreeCall* call)
if (setupArg != nullptr)
{
arg.SetEarlyNode(setupArg);

// Make sure we do not break recognition of retbuf-as-local
// optimization here. If this is hit it indicates that we are
// unnecessarily creating temps for some ret buf addresses, and
// gtCallGetDefinedRetBufLclAddr relies on this not to happen.
noway_assert((arg.GetWellKnownArg() != WellKnownArg::RetBuffer) || !call->IsOptimizingRetBufAsLocal());
}

arg.SetLateNode(defArg);
*lateTail = &arg;
lateTail = &arg.LateNextRef();
}

// Make sure we did not do anything here that stops us from being able to
// find the local ret buf if we are optimizing it.
noway_assert(!call->IsOptimizingRetBufAsLocal() || (comp->gtCallGetDefinedRetBufLclAddr(call) != nullptr));

#ifdef DEBUG
if (comp->verbose)
{
Expand Down Expand Up @@ -11169,10 +11171,10 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
}
}

/* Morphing along with folding and inlining may have changed the
* side effect flags, so we have to reset them
*
* NOTE: Don't reset the exception flags on nodes that may throw */
// Morphing along with folding and inlining may have changed the
// side effect flags, so we have to reset them
//
// NOTE: Don't reset the exception flags on nodes that may throw

assert(tree->gtOper != GT_CALL);

Expand All @@ -11181,11 +11183,14 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
tree->gtFlags &= ~GTF_CALL;
}

/* Propagate the new flags */
// Propagate the new flags
tree->gtFlags |= (op1->gtFlags & GTF_ALL_EFFECT);

// &aliasedVar doesn't need GTF_GLOB_REF, though alisasedVar does
if (oper == GT_ADDR && (op1->gtOper == GT_LCL_VAR))
// addresses of locals do not need GTF_GLOB_REF, even if the child has
// it (is address exposed). Note that general addressing may still need
// GTF_GLOB_REF, for example if the subtree has a comma that involves a
// global reference.
if (tree->OperIs(GT_ADDR) && ((tree->gtFlags & GTF_GLOB_REF) != 0) && tree->IsLocalAddrExpr())
{
tree->gtFlags &= ~GTF_GLOB_REF;
}
Expand Down