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: Make effect handling in lowering less conservative #92710

Merged
merged 5 commits into from
Sep 29, 2023
Merged

JIT: Make effect handling in lowering less conservative #92710

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
68321b2
JIT: Make effect handling in lowering less conservative
jakobbotsch Sep 27, 2023
25324a2
Fix build
jakobbotsch Sep 27, 2023
d28c644
Address some feedback
jakobbotsch Sep 28, 2023
a4d40cd
Add GT_NULLCHECK to OperSupportsOrderingSideEffect
jakobbotsch Sep 28, 2023
52e0247
Add GenTree::SetHasOrderingSideEffect
jakobbotsch Sep 28, 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
161 changes: 158 additions & 3 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6731,7 +6731,7 @@ GenTree* GenTree::gtGetParent(GenTree*** pUse)
// of the children's flags.
//

bool GenTree::OperRequiresAsgFlag()
bool GenTree::OperRequiresAsgFlag() const
{
switch (OperGet())
{
Expand Down Expand Up @@ -6769,8 +6769,7 @@ bool GenTree::OperRequiresAsgFlag()
// OperRequiresCallFlag : Check whether the operation requires GTF_CALL flag regardless
// of the children's flags.
//

bool GenTree::OperRequiresCallFlag(Compiler* comp)
bool GenTree::OperRequiresCallFlag(Compiler* comp) const
{
switch (gtOper)
{
Expand Down Expand Up @@ -7026,6 +7025,153 @@ bool GenTree::OperMayThrow(Compiler* comp)
return OperExceptions(comp) != ExceptionSetFlags::None;
}

//------------------------------------------------------------------------------
// OperRequiresGlobRefFlag : Check whether the operation requires GTF_GLOB_REF
// flag regardless of the children's flags.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//
//
// Globally visible stores and loads, as well as some equivalently modeled
// operations, require the GLOB_REF flag to be set on the node.
//

Copy link
Member Author

Choose a reason for hiding this comment

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

Expanded on this a bit in the remarks.

// Arguments:
// comp - Compiler instance
//
// Return Value:
// True if the given operator requires GTF_GLOB_REF
//
// Remarks:
// Only valid after local morph.
//
bool GenTree::OperRequiresGlobRefFlag(Compiler* comp) const
{
switch (OperGet())
{
case GT_LCL_VAR:
case GT_LCL_FLD:
case GT_STORE_LCL_VAR:
case GT_STORE_LCL_FLD:
return comp->lvaGetDesc(AsLclVarCommon())->IsAddressExposed();

case GT_IND:
case GT_BLK:
if (AsIndir()->IsInvariantLoad())
{
return false;
}
FALLTHROUGH;

case GT_STOREIND:
case GT_STORE_BLK:
case GT_STORE_DYN_BLK:
case GT_XADD:
case GT_XORR:
case GT_XAND:
case GT_XCHG:
case GT_LOCKADD:
case GT_CMPXCHG:
case GT_MEMORYBARRIER:
case GT_KEEPALIVE:
return true;

case GT_CALL:
return AsCall()->HasSideEffects(comp, /* ignoreExceptions */ true);
Comment on lines +7078 to +7079
Copy link
Contributor

@SingleAccretion SingleAccretion Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This was apparently in my second commit - GT_ALLOCOBJ should be here as well with return AsAllocObj()->gtHelperHasSideEffects;


#if defined(FEATURE_HW_INTRINSICS)
case GT_HWINTRINSIC:
return AsHWIntrinsic()->OperRequiresGlobRefFlag();
#endif // FEATURE_HW_INTRINSICS

default:
assert(!OperRequiresCallFlag(comp) || OperIs(GT_INTRINSIC));
assert((!OperIsIndir() || OperIs(GT_NULLCHECK)) && !OperRequiresAsgFlag());
return false;
}
}

//------------------------------------------------------------------------------
// OperSupportsOrderingSideEffect : Check whether the operation supports the
// GTF_ORDER_SIDEEFF flag.
//
// Return Value:
// True if the given operator supports GTF_ORDER_SIDEEFF.
//
// Remarks:
// A node will still have this flag set if an operand has it set, even if the
// parent does not support it. This situation indicates that reordering the
// parent may be ok as long as it does not break ordering dependencies of the
// operand.
//
bool GenTree::OperSupportsOrderingSideEffect() const
Copy link
Contributor

@SingleAccretion SingleAccretion Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do you have ideas on how to make sure that this method does not silently drop ordering effects when we start using it for new nodes?

For example, say someone is implementing non-null assertion propagation for GT_ARR_LENGTH family of operators. They will set the flag, just as current code does for indirs, and it will show up in dumps and all, but the effect will be silently dropped.

Perhaps add a setter for GTF_ORDER_SIDEEFF with an assert (there is SetAllEffectsFlags & Co already, but they are used to propagate flags upwards, so, not very suitable)?

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 can add assert in fgdiagnostic to ensure that GTF_ORDER_SIDEEFF implies that OperSupportsOrderingSideEffect() || (any operand has the flag).

Copy link
Member Author

Choose a reason for hiding this comment

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

Doing this requires making sure we propagate/unset these flags in all the same places as the other flags... That's probably a good thing to do down the road, but I think for now I will skip it and think about doing that separately.
Let me add something like SetHasOrderingSideEffect() with the assert in it for this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doing this did make me notice that I missed GT_NULLCHECK in OperSupportsOrderingSideEffect though, so definitely worth to have something.

{
if (TypeIs(TYP_BYREF))
{
// Forming byrefs may only be legal due to previous checks.
return true;
}

switch (OperGet())
{
case GT_BOUNDS_CHECK:
case GT_IND:
case GT_BLK:
case GT_STOREIND:
case GT_STORE_BLK:
case GT_STORE_DYN_BLK:
case GT_XADD:
case GT_XORR:
case GT_XAND:
case GT_XCHG:
case GT_LOCKADD:
case GT_CMPXCHG:
case GT_MEMORYBARRIER:
case GT_CATCH_ARG:
#if defined(FEATURE_HW_INTRINSICS)
case GT_HWINTRINSIC:
#endif
jakobbotsch marked this conversation as resolved.
Show resolved Hide resolved
return true;
default:
return false;
}
}

//------------------------------------------------------------------------------
// OperEffects: Compute effect flags that are relevant to this node only,
// excluding its children.
//
// Arguments:
// comp - Compiler instance
//
// Return Value:
// The effect flags.
//
GenTreeFlags GenTree::OperEffects(Compiler* comp)
{
GenTreeFlags flags = gtFlags & GTF_ALL_EFFECT;

if (((flags & GTF_ASG) != 0) && !OperRequiresAsgFlag())
{
flags &= ~GTF_ASG;
}

if (((flags & GTF_CALL) != 0) && !OperRequiresCallFlag(comp))
{
flags &= ~GTF_CALL;
}

if (((flags & GTF_EXCEPT) != 0) && !OperMayThrow(comp))
{
flags &= ~GTF_EXCEPT;
}

if (((flags & GTF_GLOB_REF) != 0) && !OperRequiresGlobRefFlag(comp))
{
flags &= ~GTF_GLOB_REF;
}

if ((flags & GTF_ORDER_SIDEEFF) != 0 && !OperSupportsOrderingSideEffect())
jakobbotsch marked this conversation as resolved.
Show resolved Hide resolved
{
flags &= ~GTF_ORDER_SIDEEFF;
}

return flags;
}

//-----------------------------------------------------------------------------------
// GetFieldCount: Return the register count for a multi-reg lclVar.
//
Expand Down Expand Up @@ -25345,6 +25491,15 @@ bool GenTreeHWIntrinsic::OperRequiresCallFlag() const
return false;
}

//------------------------------------------------------------------------------
// OperRequiresCallFlag : Check whether the operation requires GTF_GLOB_REF flag regardless
jakobbotsch marked this conversation as resolved.
Show resolved Hide resolved
// of the children's flags.
//
bool GenTreeHWIntrinsic::OperRequiresGlobRefFlag() const
{
return OperIsMemoryLoad() || OperRequiresAsgFlag() || OperRequiresCallFlag();
}

//------------------------------------------------------------------------
// GetLayout: Get the layout for this TYP_STRUCT HWI node.
//
Expand Down
20 changes: 17 additions & 3 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1852,12 +1852,19 @@ struct GenTree
// Returns true if it is a GT_COPY or GT_RELOAD of a multi-reg call node
inline bool IsCopyOrReloadOfMultiRegCall() const;

bool OperRequiresAsgFlag();
bool OperRequiresAsgFlag() const;

bool OperRequiresCallFlag(Compiler* comp);
bool OperRequiresCallFlag(Compiler* comp) const;

bool OperMayThrow(Compiler* comp);
ExceptionSetFlags OperExceptions(Compiler* comp);
bool OperMayThrow(Compiler* comp);

bool OperRequiresGlobRefFlag(Compiler* comp) const;

bool OperSupportsOrderingSideEffect() const;

// Compute effect flags that only pertain to this node excluding its children.
jakobbotsch marked this conversation as resolved.
Show resolved Hide resolved
GenTreeFlags OperEffects(Compiler* comp);

unsigned GetScaleIndexMul();
unsigned GetScaleIndexShf();
Expand Down Expand Up @@ -6249,6 +6256,7 @@ struct GenTreeHWIntrinsic : public GenTreeJitIntrinsic

bool OperRequiresAsgFlag() const;
bool OperRequiresCallFlag() const;
bool OperRequiresGlobRefFlag() const;

unsigned GetResultOpNumForRmwIntrinsic(GenTree* use, GenTree* op1, GenTree* op2, GenTree* op3);

Expand Down Expand Up @@ -7177,6 +7185,12 @@ struct GenTreeIndir : public GenTreeOp
return (gtFlags & GTF_IND_UNALIGNED) != 0;
}

// True if this indirection is invariant.
bool IsInvariantLoad() const
{
return (gtFlags & GTF_IND_INVARIANT) != 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

I also had an assert(!isInvariant || OperIs(GT_IND, GT_BLK)); here to exclude stores. Worth adding?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added it

}

#if DEBUGGABLE_GENTREE
// Used only for GenTree::GetVtableForOper()
GenTreeIndir() : GenTreeOp()
Expand Down
14 changes: 1 addition & 13 deletions src/coreclr/jit/rationalize.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,24 +342,12 @@ Compiler::fgWalkResult Rationalizer::RewriteNode(GenTree** useEdge, Compiler::Ge
}
else
{
if (((node->gtFlags & GTF_ASG) != 0) && !node->OperRequiresAsgFlag())
{
// Clear the GTF_ASG flag for all nodes that do not require it
node->gtFlags &= ~GTF_ASG;
}

if (!node->IsCall())
{
// Clear the GTF_CALL flag for all nodes but calls
node->gtFlags &= ~GTF_CALL;
}

if (node->IsValue() && use.IsDummyUse())
{
node->SetUnusedValue();
}

if (node->TypeGet() == TYP_LONG)
if (node->TypeIs(TYP_LONG))
{
comp->compLongUsed = true;
}
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/sideeffects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ SideEffectSet::SideEffectSet(Compiler* compiler, GenTree* node) : m_sideEffectFl
//
void SideEffectSet::AddNode(Compiler* compiler, GenTree* node)
{
m_sideEffectFlags |= (node->gtFlags & GTF_ALL_EFFECT);
m_sideEffectFlags |= node->OperEffects(compiler);
m_aliasSet.AddNode(compiler, node);
}

Expand Down Expand Up @@ -571,7 +571,7 @@ bool SideEffectSet::InterferesWith(const SideEffectSet& other, bool strict) cons
//
bool SideEffectSet::InterferesWith(Compiler* compiler, GenTree* node, bool strict) const
{
return InterferesWith((node->gtFlags & GTF_ALL_EFFECT), AliasSet::NodeInfo(compiler, node), strict);
return InterferesWith(node->OperEffects(compiler), AliasSet::NodeInfo(compiler, node), strict);
}

//------------------------------------------------------------------------
Expand Down
Loading