Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

JIT: Relax reg-optionality validity checks #81614

Merged
merged 6 commits into from
Feb 13, 2023
Merged

JIT: Relax reg-optionality validity checks #81614

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf0952b
JIT: Relax reg-optionality validity checks
jakobbotsch Feb 4, 2023
8df13a7
Fix bad refactoring
jakobbotsch Feb 4, 2023
bee4fb7
Add a couple of asserts in IsInvariantInRange
jakobbotsch Feb 4, 2023
dc7f280
Add function header
jakobbotsch Feb 4, 2023
7ac5c9c
Address feedback
jakobbotsch Feb 9, 2023
fcc9a9f
Merge branch 'main' of github.com:dotnet/runtime into relax-reg-optio…
jakobbotsch Feb 9, 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
180 changes: 138 additions & 42 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,12 @@ void Lowering::MakeSrcRegOptional(GenTree* parentNode, GenTree* childNode) const
#ifdef DEBUG
// Verify caller of this method checked safety.
//
const bool isSafeToContainMem = IsSafeToContainMem(parentNode, childNode);
const bool isSafeToMarkRegOptional = IsSafeToMarkRegOptional(parentNode, childNode);

if (!isSafeToContainMem)
if (!isSafeToMarkRegOptional)
{
JITDUMP("** Unsafe regOptional of [%06u] in [%06u}\n", comp->dspTreeID(childNode), comp->dspTreeID(parentNode));
assert(isSafeToContainMem);
assert(isSafeToMarkRegOptional);
}
#endif
}
Expand All @@ -100,16 +100,11 @@ void Lowering::TryMakeSrcContainedOrRegOptional(GenTree* parentNode, GenTree* ch
// HWIntrinsic nodes should use TryGetContainableHWIntrinsicOp and its relevant handling
assert(!parentNode->OperIsHWIntrinsic());

if (!IsSafeToContainMem(parentNode, childNode))
{
return;
}

if (IsContainableMemoryOp(childNode))
if (IsContainableMemoryOp(childNode) && IsSafeToContainMem(parentNode, childNode))
{
MakeSrcContained(parentNode, childNode);
}
else
else if (IsSafeToMarkRegOptional(parentNode, childNode))
{
MakeSrcRegOptional(parentNode, childNode);
}
Expand Down Expand Up @@ -140,32 +135,38 @@ bool Lowering::CheckImmedAndMakeContained(GenTree* parentNode, GenTree* childNod
}

//------------------------------------------------------------------------
// IsSafeToContainMem: Checks for conflicts between childNode and parentNode,
// and returns 'true' iff memory operand childNode can be contained in parentNode.
// IsInvariantInRange: Check if a node is invariant in the specified range. In
// other words, can 'node' be moved to right before 'endExclusive' without its
// computation changing values?
//
// Arguments:
// parentNode - any non-leaf node
// childNode - some node that is an input to `parentNode`
// node - The node.
// endExclusive - The exclusive end of the range to check invariance for.
//
// Return value:
// true if it is safe to make childNode a contained memory operand.
// Returns:
// True if 'node' can be evaluated at any point between its current
// location and 'endExclusive' without giving a different result; otherwise
// false.
//
bool Lowering::IsSafeToContainMem(GenTree* parentNode, GenTree* childNode) const
bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive) const
{
assert((node != nullptr) && (endExclusive != nullptr));

// Quick early-out for unary cases
//
if (childNode->gtNext == parentNode)
if (node->gtNext == endExclusive)
{
return true;
}

m_scratchSideEffects.Clear();
m_scratchSideEffects.AddNode(comp, childNode);
m_scratchSideEffects.AddNode(comp, node);

for (GenTree* node = childNode->gtNext; node != parentNode; node = node->gtNext)
for (GenTree* cur = node->gtNext; cur != endExclusive; cur = cur->gtNext)
{
assert((cur != nullptr) && "Expected first node to precede end node");
const bool strict = true;
if (m_scratchSideEffects.InterferesWith(comp, node, strict))
if (m_scratchSideEffects.InterferesWith(comp, cur, strict))
{
return false;
}
Expand All @@ -175,31 +176,36 @@ bool Lowering::IsSafeToContainMem(GenTree* parentNode, GenTree* childNode) const
}

//------------------------------------------------------------------------
// IsSafeToContainMem: Checks for conflicts between childNode and grandParentNode
// and returns 'true' iff memory operand childNode can be contained in ancestorNode
// IsInvariantInRange: Check if a node is invariant in the specified range,
// ignoring conflicts with one particular node.
//
// Arguments:
// grandParentNode - any non-leaf node
// parentNode - parent of `childNode` and an input to `grandParentNode`
// childNode - some node that is an input to `parentNode`
// node - The node.
// endExclusive - The exclusive end of the range to check invariance for.
// ignoreNode - A node to ignore interference checks with, for example
// because it will retain its relative order with 'node'.
//
// Return value:
// true if it is safe to make childNode a contained memory operand.
// Returns:
// True if 'node' can be evaluated at any point between its current location
// and 'endExclusive' without giving a different result; otherwise false.
//
bool Lowering::IsSafeToContainMem(GenTree* grandparentNode, GenTree* parentNode, GenTree* childNode) const
bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive, GenTree* ignoreNode) const
{
assert((node != nullptr) && (endExclusive != nullptr));

Copy link
Member

Choose a reason for hiding this comment

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

Is there any invariant case we should handle here? That is, the prior handles the linear order being node, endExclusive. Should this handle node, ignoreNode, endExclusive?

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'll add an additional assert about this

m_scratchSideEffects.Clear();
m_scratchSideEffects.AddNode(comp, childNode);
m_scratchSideEffects.AddNode(comp, node);

for (GenTree* node = childNode->gtNext; node != grandparentNode; node = node->gtNext)
for (GenTree* cur = node->gtNext; cur != endExclusive; cur = cur->gtNext)
{
if (node == parentNode)
assert((cur != nullptr) && "Expected first node to precede end node");
if (cur == ignoreNode)
{
continue;
}

const bool strict = true;
if (m_scratchSideEffects.InterferesWith(comp, node, strict))
if (m_scratchSideEffects.InterferesWith(comp, cur, strict))
{
return false;
}
Expand All @@ -208,6 +214,95 @@ bool Lowering::IsSafeToContainMem(GenTree* grandparentNode, GenTree* parentNode,
return true;
}

//------------------------------------------------------------------------
// IsSafeToContainMem: Checks for conflicts between childNode and parentNode,
// and returns 'true' iff memory operand childNode can be contained in parentNode.
//
// Arguments:
// parentNode - any non-leaf node
// childNode - some node that is an input to `parentNode`
//
// Return value:
// true if it is safe to make childNode a contained memory operand.
//
bool Lowering::IsSafeToContainMem(GenTree* parentNode, GenTree* childNode) const
{
return IsInvariantInRange(childNode, parentNode);
}

//------------------------------------------------------------------------
// IsSafeToContainMem: Checks for conflicts between childNode and grandParentNode
// and returns 'true' iff memory operand childNode can be contained in grandParentNode.
//
// Arguments:
// grandParentNode - any non-leaf node
// parentNode - parent of `childNode` and an input to `grandParentNode`
// childNode - some node that is an input to `parentNode`
//
// Return value:
// true if it is safe to make childNode a contained memory operand.
//
bool Lowering::IsSafeToContainMem(GenTree* grandparentNode, GenTree* parentNode, GenTree* childNode) const
{
return IsInvariantInRange(childNode, grandparentNode, parentNode);
}

//------------------------------------------------------------------------
// IsSafeToMarkRegOptional: Check whether it is safe to mark 'childNode' as
// reg-optional in 'parentNode'.
//
// Arguments:
// parentNode - parent of 'childNode'
// childNode - some node that is an input to `parentNode`
//
// Return value:
// True if it is safe to mark childNode as reg-optional; otherwise false.
//
// Remarks:
// Unlike containment, reg-optionality can only rarely introduce new
// conflicts, because reg-optionality mostly does not cause the child node
// to be evaluated at a new point in time:
//
// 1. For LIR edges (i.e. anything that isn't GT_LCL_VAR) reg-optionality
// indicates that if the edge was spilled to a temp at its def, the parent
// node can use it directly from its spill location without reloading it
// into a register first. This is always safe as as spill temps cannot
// interfere.
//
// For example, an indirection can be marked reg-optional even if there
// is interference between it and its parent; the indirection will still
// be evaluated at its original position, but if the value is spilled to
// stack, then reg-optionality can allow using the value from the spill
// location directly. Similarly, GT_LCL_FLD nodes are never register
// candidates and can be handled the same way.
//
// 2. For GT_LCL_VAR reg-optionality indicates that the node can use the
// local directly from its home location. IR invariants guarantee that the
// local is not defined between its LIR location and the parent node (see
// CheckLclVarSemanticsHelper). That means the only case where it could
// interfere is due to it being address exposed. So this is the only unsafe
// case.
//
bool Lowering::IsSafeToMarkRegOptional(GenTree* parentNode, GenTree* childNode) const
Copy link
Member

Choose a reason for hiding this comment

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

Is any of this optimization that should also apply to IsSafeToContainMem?

That is, iirc there is a "pessimization" for the InterferesWith check today with regards to locals in that any write is considered interfering, even if its to another local and therefore cannot impact the local being read.

However, based on https://github.com/dotnet/runtime/blob/main/docs/design/specs/Memory-model.md I believe we are allowed to reorder such a non-volatile read of a local assuming there are no volatile writes.

Copy link
Member

@tannergooding tannergooding Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe what I'm remembering is just the fact that we couldn't mark it reg-optional and the fix is exactly what is being handled here, not something to the general IsSafeToContainMem

Copy link
Member Author

@jakobbotsch jakobbotsch Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think most of this applies to IsSafeToContainMem, the problem there is really that it causes evaluation to happen at a new point in time, which brings along with it all of the extra checking. W.r.t the locals, from looking at AliasSet::InterferesWith it looks like we are pretty precise:

runtime/src/coreclr/jit/sideeffects.cpp

Lines 319 to 335 in 917f407

//------------------------------------------------------------------------
// AliasSet::InterferesWith:
// Returns true if the reads and writes in this alias set interfere
// with the given alias set.
//
// Two alias sets interfere under any of the following conditions:
// - Both sets write to any addressable location (e.g. the heap,
// address-exposed locals)
// - One set reads any addressable location and the other set writes
// any addressable location
// - Both sets write to the same lclVar
// - One set writes to a lclVar that is read by the other set
//
// Arguments:
// other - The other alias set.
//
bool AliasSet::InterferesWith(const AliasSet& other) const

Copy link
Member

Choose a reason for hiding this comment

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

@SingleAccretion managed to find the discussion we had it some time back on Discord

The general scenario I was thinking about is: #76273 (comment)

Many diffs are cases like the following:

- add     x0, x0, w26, UXTW
- ldrb    w0, [x0]
+ ldrb    w0, [x0, w26, UXTW #2]

These seem to mostly be due to ADDEX not being properly handled in many places while ADD always is.

There are a few small regressions such as:

+ lsl     w0, w0, #8
  ldr     x1, [fp, #0xA8]	// [V192 tmp170]
  ldrb    w1, [x1, #0x01]
- add     w1, w1, w0,  LSL #8
+ add     w1, w0, w1

Notably we're checking if childNode->gtGetOp1() (the value to be shifted) is contained, the original doesn't (but it shouldn't be anyways since we need a register for it).

We're also checking IsSafeToContainMem(parentNode, childNode) where-as the original wasn't. I expect this is the "actual" reason for the "larger diff" since we always do "strict" checking and we don't allow reordering across anything with a "side effect". That being said, there is no actual interdependence between these two and so it is technically "safe" anyways.

{
if (!childNode->OperIs(GT_LCL_VAR))
Copy link
Contributor

Choose a reason for hiding this comment

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

It is a rather subtle difference between LCL_VAR and LCL_FLD that LCL_FLDs, if marked reg optional (which they won't be under normal circumstances), will use spill temps. Something that would be good to write down explicitly I think.

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 it with the "indirection" example above? I suppose it's the exact same example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, just mentioning it would be good.

{
// LIR edges never interfere. This includes GT_LCL_FLD, see the remarks above.
return true;
}

LclVarDsc* dsc = comp->lvaGetDesc(childNode->AsLclVarCommon());
if (!dsc->IsAddressExposed())
{
// Safe by IR invariants (no assignments occur between parent and node).
return true;
}

// We expect this to have interference as otherwise we could have marked it
// contained instead of reg-optional.
return false;
}

//------------------------------------------------------------------------
// LowerNode: this is the main entry point for Lowering.
//
Expand Down Expand Up @@ -2530,15 +2625,16 @@ void Lowering::LowerCFGCall(GenTreeCall* call)
}

//------------------------------------------------------------------------
// IsInvariantInRange: Check if a node is invariant in the specified range. In
// other words, can 'node' be moved to right before 'endExclusive' without its
// computation changing values?
// IsCFGCallArgInvariantInRange: A cheap version of IsInvariantInRange to check
// if a node is invariant in the specified range. In other words, can 'node' be
// moved to right before 'endExclusive' without its computation changing
// values?
//
// Arguments:
// node - The node.
// endExclusive - The exclusive end of the range to check invariance for.
//
bool Lowering::IsInvariantInRange(GenTree* node, GenTree* endExclusive)
bool Lowering::IsCFGCallArgInvariantInRange(GenTree* node, GenTree* endExclusive)
{
assert(node->Precedes(endExclusive));

Expand Down Expand Up @@ -2603,7 +2699,7 @@ void Lowering::MoveCFGCallArg(GenTreeCall* call, GenTree* node)
GenTree* operand = node->AsOp()->gtGetOp1();
JITDUMP("Checking if we can move operand of GT_PUTARG_* node:\n");
DISPTREE(operand);
if (((operand->gtFlags & GTF_ALL_EFFECT) == 0) && IsInvariantInRange(operand, call))
if (((operand->gtFlags & GTF_ALL_EFFECT) == 0) && IsCFGCallArgInvariantInRange(operand, call))
{
JITDUMP("...yes, moving to after validator call\n");
BlockRange().Remove(operand);
Expand Down Expand Up @@ -5482,7 +5578,7 @@ bool Lowering::TryCreateAddrMode(GenTree* addr, bool isContainable, GenTree* par
{
if (index->OperIs(GT_CAST) && (scale == 1) && (offset == 0) && varTypeIsByte(targetType))
{
if (IsSafeToContainMem(parent, index))
if (IsInvariantInRange(index, parent))
{
// Check containment safety against the parent node - this will ensure that LEA with the contained
// index will itself always be contained. We do not support uncontained LEAs with contained indices.
Expand All @@ -5505,7 +5601,7 @@ bool Lowering::TryCreateAddrMode(GenTree* addr, bool isContainable, GenTree* par
if (cast->CastOp()->TypeIs(TYP_INT) && cast->TypeIs(TYP_LONG) &&
(genTypeSize(targetType) == (1U << shiftBy)) && (scale == 1) && (offset == 0))
{
if (IsSafeToContainMem(parent, index))
if (IsInvariantInRange(index, parent))
{
// Check containment safety against the parent node - this will ensure that LEA with the contained
// index will itself always be contained. We do not support uncontained LEAs with contained indices.
Expand Down Expand Up @@ -7214,7 +7310,7 @@ void Lowering::LowerStoreIndirCommon(GenTreeStoreInd* ind)
#if defined(TARGET_ARM64)
// Verify containment safety before creating an LEA that must be contained.
//
const bool isContainable = IsSafeToContainMem(ind, ind->Addr());
const bool isContainable = IsInvariantInRange(ind->Addr(), ind);
#else
const bool isContainable = true;
#endif
Expand Down Expand Up @@ -7275,7 +7371,7 @@ void Lowering::LowerIndir(GenTreeIndir* ind)
#if defined(TARGET_ARM64)
// Verify containment safety before creating an LEA that must be contained.
//
const bool isContainable = (ind->Addr() != nullptr) && IsSafeToContainMem(ind, ind->Addr());
const bool isContainable = (ind->Addr() != nullptr) && IsInvariantInRange(ind->Addr(), ind);
#else
const bool isContainable = true;
#endif
Expand Down
8 changes: 7 additions & 1 deletion src/coreclr/jit/lower.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ class Lowering final : public Phase
void LowerBlock(BasicBlock* block);
GenTree* LowerNode(GenTree* node);

bool IsInvariantInRange(GenTree* node, GenTree* endExclusive);
bool IsCFGCallArgInvariantInRange(GenTree* node, GenTree* endExclusive);

// ------------------------------
// Call Lowering
Expand Down Expand Up @@ -500,13 +500,19 @@ class Lowering final : public Phase
// Checks and makes 'childNode' contained in the 'parentNode'
bool CheckImmedAndMakeContained(GenTree* parentNode, GenTree* childNode);

bool IsInvariantInRange(GenTree* node, GenTree* endExclusive) const;
bool IsInvariantInRange(GenTree* node, GenTree* endExclusive, GenTree* ignoreNode) const;

// Checks for memory conflicts in the instructions between childNode and parentNode, and returns true if childNode
// can be contained.
bool IsSafeToContainMem(GenTree* parentNode, GenTree* childNode) const;

// Similar to above, but allows bypassing a "transparent" parent.
bool IsSafeToContainMem(GenTree* grandparentNode, GenTree* parentNode, GenTree* childNode) const;

// Check if marking an operand of a node as reg-optional is safe.
bool IsSafeToMarkRegOptional(GenTree* parentNode, GenTree* node) const;

inline LIR::Range& BlockRange() const
{
return LIR::AsRange(m_block);
Expand Down
Loading