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

Add and use a MakeSrcRegOptional that validates IsSafeToContainMem was called #77895

Merged
merged 4 commits into from
Nov 7, 2022
Merged

Add and use a MakeSrcRegOptional that validates IsSafeToContainMem was called #77895

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f4f8cb2
Add and use a MakeSrcRegOptional that validates IsSafeToContainMem wa…
tannergooding Nov 4, 2022
c5e006c
Applying formatting patch
tannergooding Nov 4, 2022
babcaf5
Revert the MakeSrcRegOptional in ContainCheckDivOrMod due to an assert
tannergooding Nov 4, 2022
c25ac01
Moving TryMakeSrcContainedOrRegOptional into the lower.cpp file
tannergooding Nov 5, 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
55 changes: 55 additions & 0 deletions src/coreclr/jit/lower.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ void Lowering::MakeSrcContained(GenTree* parentNode, GenTree* childNode) const
{
assert(!parentNode->OperIsLeaf());
assert(childNode->canBeContained());

childNode->SetContained();
assert(childNode->isContained());

Expand All @@ -60,6 +61,60 @@ void Lowering::MakeSrcContained(GenTree* parentNode, GenTree* childNode) const
#endif
}

//------------------------------------------------------------------------
// MakeSrcRegOptional: Make "childNode" a regOptional node
//
// Arguments:
// parentNode - is a non-leaf node that can regOptional its 'childNode'
// childNode - is an op that will now be regOptional to its parent.
//
void Lowering::MakeSrcRegOptional(GenTree* parentNode, GenTree* childNode) const
{
assert(!parentNode->OperIsLeaf());

childNode->SetRegOptional();
assert(childNode->IsRegOptional());

#ifdef DEBUG
// Verify caller of this method checked safety.
//
const bool isSafeToContainMem = IsSafeToContainMem(parentNode, childNode);

if (!isSafeToContainMem)
{
JITDUMP("** Unsafe regOptional of [%06u] in [%06u}\n", comp->dspTreeID(childNode), comp->dspTreeID(parentNode));
assert(isSafeToContainMem);
}
#endif
}

//------------------------------------------------------------------------
// TryMakeSrcContainedOrRegOptional: Tries to make "childNode" a contained or regOptional node
//
// Arguments:
// parentNode - is a non-leaf node that can contain or regOptional its 'childNode'
// childNode - is an op that will now be contained or regOptional to its parent.
//
void Lowering::TryMakeSrcContainedOrRegOptional(GenTree* parentNode, GenTree* childNode) const
{
// HWIntrinsic nodes should use TryGetContainableHWIntrinsicOp and its relevant handling
assert(!parentNode->OperIsHWIntrinsic());

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

if (IsContainableMemoryOp(childNode))
{
MakeSrcContained(parentNode, childNode);
}
else
{
MakeSrcRegOptional(parentNode, childNode);
}
}

//------------------------------------------------------------------------
// CheckImmedAndMakeContained: Checks if the 'childNode' is a containable immediate
// and, if so, makes it contained.
Expand Down
17 changes: 15 additions & 2 deletions src/coreclr/jit/lower.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,6 +288,7 @@ class Lowering final : public Phase
const bool op2Legal = isSafeToMarkOp2 && (operatorSize == genTypeSize(op2->TypeGet()));

GenTree* regOptionalOperand = nullptr;

if (op1Legal)
{
regOptionalOperand = op2Legal ? PreferredRegOptionalOperand(tree) : op1;
Expand All @@ -296,9 +297,10 @@ class Lowering final : public Phase
{
regOptionalOperand = op2;
}

if (regOptionalOperand != nullptr)
{
regOptionalOperand->SetRegOptional();
MakeSrcRegOptional(tree, regOptionalOperand);
}
}
#endif // defined(TARGET_XARCH)
Expand Down Expand Up @@ -447,7 +449,7 @@ class Lowering final : public Phase
bool IsContainableBinaryOp(GenTree* parentNode, GenTree* childNode) const;
#endif // TARGET_ARM64

#ifdef FEATURE_HW_INTRINSICS
#if defined(FEATURE_HW_INTRINSICS)
// Tries to get a containable node for a given HWIntrinsic
bool TryGetContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode,
GenTree** pNode,
Expand All @@ -465,6 +467,17 @@ class Lowering final : public Phase
// Makes 'childNode' contained in the 'parentNode'
void MakeSrcContained(GenTree* parentNode, GenTree* childNode) const;

// Makes 'childNode' regOptional in the 'parentNode'
void MakeSrcRegOptional(GenTree* parentNode, GenTree* childNode) const;

// Tries to make 'childNode' contained or regOptional in the 'parentNode'
void TryMakeSrcContainedOrRegOptional(GenTree* parentNode, GenTree* childNode) const;

#if defined(FEATURE_HW_INTRINSICS)
// Tries to make 'childNode' contained or regOptional in the 'parentNode'
void TryMakeSrcContainedOrRegOptional(GenTreeHWIntrinsic* parentNode, GenTree* childNode) const;
#endif

// Checks and makes 'childNode' contained in the 'parentNode'
bool CheckImmedAndMakeContained(GenTree* parentNode, GenTree* childNode);

Expand Down
107 changes: 41 additions & 66 deletions src/coreclr/jit/lowerxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,7 @@ void Lowering::LowerPutArgStk(GenTreePutArgStk* putArgStk)
// than spilling, but this situation is not all that common, as most cases of FIELD_LIST
// are promoted structs, which do not not have a large number of fields, and of those
// most are lclVars or copy-propagated constants.

fieldNode->SetRegOptional();
}
}
Expand Down Expand Up @@ -720,17 +721,10 @@ void Lowering::LowerPutArgStk(GenTreePutArgStk* putArgStk)
MakeSrcContained(putArgStk, src);
}
#ifdef TARGET_X86
else if ((genTypeSize(src) == TARGET_POINTER_SIZE) && IsSafeToContainMem(putArgStk, src))
else if (genTypeSize(src) == TARGET_POINTER_SIZE)
{
// We can use "src" directly from memory with "push [mem]".
if (IsContainableMemoryOp(src))
{
MakeSrcContained(putArgStk, src);
}
else
{
src->SetRegOptional();
}
TryMakeSrcContainedOrRegOptional(putArgStk, src);
}
#endif // TARGET_X86
}
Expand Down Expand Up @@ -5305,18 +5299,12 @@ void Lowering::ContainCheckCast(GenTreeCast* node)
}
}

if (srcIsContainable && IsSafeToContainMem(node, castOp))
if (srcIsContainable)
{
if (IsContainableMemoryOp(castOp))
{
MakeSrcContained(node, castOp);
}
else
{
castOp->SetRegOptional();
}
TryMakeSrcContainedOrRegOptional(node, castOp);
}
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
}

#if !defined(TARGET_64BIT)
if (varTypeIsLong(srcType))
{
Expand Down Expand Up @@ -5386,7 +5374,7 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
// IsSafeToContainMem is expensive so we call it at most once for otherOp.
// If we already called IsSafeToContainMem, it must have returned false;
// otherwise, otherOp would be contained.
otherOp->SetRegOptional();
MakeSrcRegOptional(cmp, otherOp);
}

return;
Expand All @@ -5401,14 +5389,7 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
// we can treat the MemoryOp as contained.
if (op1Type == op2Type)
{
if (IsContainableMemoryOp(op1) && IsSafeToContainMem(cmp, op1))
{
MakeSrcContained(cmp, op1);
}
else
{
op1->SetRegOptional();
}
TryMakeSrcContainedOrRegOptional(cmp, op1);
}
}
else if (op1Type == op2Type)
Expand Down Expand Up @@ -5452,7 +5433,7 @@ void Lowering::ContainCheckCompare(GenTreeOp* cmp)
: isSafeToContainOp2 && IsSafeToContainMem(cmp, op2);
if (setRegOptional)
{
regOptionalCandidate->SetRegOptional();
MakeSrcRegOptional(cmp, regOptionalCandidate);
}
}
}
Expand Down Expand Up @@ -5702,15 +5683,7 @@ void Lowering::ContainCheckBoundsChk(GenTreeBoundsChk* node)

if (node->GetIndex()->TypeGet() == node->GetArrayLength()->TypeGet())
{
if (IsContainableMemoryOp(other) && IsSafeToContainMem(node, other))
{
MakeSrcContained(node, other);
}
else
{
// We can mark 'other' as reg optional, since it is not contained.
other->SetRegOptional();
}
TryMakeSrcContainedOrRegOptional(node, other);
}
}

Expand All @@ -5732,15 +5705,13 @@ void Lowering::ContainCheckIntrinsic(GenTreeOp* node)
{
GenTree* op1 = node->gtGetOp1();

if ((IsContainableMemoryOp(op1) && IsSafeToContainMem(node, op1)) || op1->IsCnsNonZeroFltOrDbl())
if (op1->IsCnsNonZeroFltOrDbl())
{
MakeSrcContained(node, op1);
}
else
{
// Mark the operand as reg optional since codegen can still
// generate code if op1 is on stack.
op1->SetRegOptional();
TryMakeSrcContainedOrRegOptional(node, op1);
}
}
}
Expand Down Expand Up @@ -6251,7 +6222,20 @@ bool Lowering::TryGetContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode
}
}

*supportsRegOptional = supportsGeneralLoads;
bool isSafeToContainMem;

// Code motion safety checks
//
if (transparentParentNode != nullptr)
{
isSafeToContainMem = IsSafeToContainMem(containingNode, transparentParentNode, node);
}
else
{
isSafeToContainMem = IsSafeToContainMem(containingNode, node);
}

*supportsRegOptional = isSafeToContainMem && supportsGeneralLoads;

if (!node->OperIsHWIntrinsic())
{
Expand All @@ -6261,16 +6245,7 @@ bool Lowering::TryGetContainableHWIntrinsicOp(GenTreeHWIntrinsic* containingNode
{
if (IsContainableMemoryOp(node))
{
// Code motion safety checks
//
if (transparentParentNode != nullptr)
{
canBeContained = IsSafeToContainMem(containingNode, transparentParentNode, node);
}
else
{
canBeContained = IsSafeToContainMem(containingNode, node);
}
canBeContained = isSafeToContainMem;
}
else if (node->IsCnsNonZeroFltOrDbl())
{
Expand Down Expand Up @@ -6538,7 +6513,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}
else if (supportsRegOptional)
{
op1->SetRegOptional();
MakeSrcRegOptional(node, op1);
}
break;
}
Expand Down Expand Up @@ -6604,7 +6579,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}
else if (supportsRegOptional)
{
op2->SetRegOptional();
MakeSrcRegOptional(node, op2);

// TODO-XArch-CQ: For commutative nodes, either operand can be reg-optional.
// https://github.com/dotnet/runtime/issues/6358
Expand Down Expand Up @@ -6646,7 +6621,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}
else if (supportsRegOptional)
{
op2->SetRegOptional();
MakeSrcRegOptional(node, op2);
}
}
break;
Expand All @@ -6668,7 +6643,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}
else if (supportsRegOptional)
{
op1->SetRegOptional();
MakeSrcRegOptional(node, op1);
}
break;
}
Expand Down Expand Up @@ -6696,7 +6671,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}
else if (supportsRegOptional)
{
op1->SetRegOptional();
MakeSrcRegOptional(node, op1);
}
}
else if (TryGetContainableHWIntrinsicOp(node, &op2, &supportsRegOptional))
Expand All @@ -6705,7 +6680,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}
else if (supportsRegOptional)
{
op2->SetRegOptional();
MakeSrcRegOptional(node, op2);
}
break;
}
Expand All @@ -6718,7 +6693,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}
else if (supportsRegOptional)
{
op1->SetRegOptional();
MakeSrcRegOptional(node, op1);
}
break;
}
Expand Down Expand Up @@ -6860,16 +6835,16 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
else if (supportsOp3RegOptional)
{
assert(resultOpNum != 3);
op3->SetRegOptional();
MakeSrcRegOptional(node, op3);
}
else if (supportsOp2RegOptional)
{
assert(resultOpNum != 2);
op2->SetRegOptional();
MakeSrcRegOptional(node, op2);
}
else if (supportsOp1RegOptional)
{
op1->SetRegOptional();
MakeSrcRegOptional(node, op1);
}
}
else
Expand All @@ -6888,7 +6863,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}
else if (supportsRegOptional)
{
op2->SetRegOptional();
MakeSrcRegOptional(node, op2);
}
break;
}
Expand All @@ -6901,7 +6876,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}
else if (supportsRegOptional)
{
op3->SetRegOptional();
MakeSrcRegOptional(node, op3);
}
break;
}
Expand All @@ -6922,7 +6897,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}
else if (supportsRegOptional)
{
op2->SetRegOptional();
MakeSrcRegOptional(node, op2);
}
break;
}
Expand Down Expand Up @@ -6972,7 +6947,7 @@ void Lowering::ContainCheckHWIntrinsic(GenTreeHWIntrinsic* node)
}
else if (supportsRegOptional)
{
op2->SetRegOptional();
MakeSrcRegOptional(node, op2);
}
break;
}
Expand Down