Skip to content

2 small CQ optimizations. #37967

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
merged 2 commits into from
Jun 16, 2020
Merged

2 small CQ optimizations. #37967

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9195bed
`REF(0)==INT(0)` for reg reuse.
Jun 16, 2020
1652984
Avoid field by field init when not profitable.
Jun 16, 2020
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
10 changes: 6 additions & 4 deletions src/coreclr/src/jit/lsra.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2733,8 +2733,10 @@ bool LinearScan::isMatchingConstant(RegRecord* physRegRecord, RefPosition* refPo
switch (otherTreeNode->OperGet())
{
case GT_CNS_INT:
if ((refPosition->treeNode->AsIntCon()->IconValue() == otherTreeNode->AsIntCon()->IconValue()) &&
(varTypeGCtype(refPosition->treeNode) == varTypeGCtype(otherTreeNode)))
{
ssize_t v1 = refPosition->treeNode->AsIntCon()->IconValue();
ssize_t v2 = otherTreeNode->AsIntCon()->IconValue();
if ((v1 == v2) && (varTypeGCtype(refPosition->treeNode) == varTypeGCtype(otherTreeNode) || v1 == 0))
{
#ifdef TARGET_64BIT
// If the constant is negative, only reuse registers of the same type.
Expand All @@ -2743,14 +2745,14 @@ bool LinearScan::isMatchingConstant(RegRecord* physRegRecord, RefPosition* refPo
// This doesn't apply to a 32-bit system, on which long values occupy multiple registers.
// (We could sign-extend, but we would have to always sign-extend, because if we reuse more
// than once, we won't have access to the instruction that originally defines the constant).
if ((refPosition->treeNode->TypeGet() == otherTreeNode->TypeGet()) ||
(refPosition->treeNode->AsIntCon()->IconValue() >= 0))
if ((refPosition->treeNode->TypeGet() == otherTreeNode->TypeGet()) || (v1 >= 0))
#endif // TARGET_64BIT
{
return true;
}
}
break;
}
case GT_CNS_DBL:
{
// For floating point constants, the values must be identical, not simply compare
Expand Down
32 changes: 22 additions & 10 deletions src/coreclr/src/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10600,8 +10600,20 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
// If we passed the above checks, then we will check these two
if (!requiresCopyBlock)
{
// It is not always profitable to do field by field init for structs that are allocated to memory.
Copy link
Contributor Author

@sandreenko sandreenko Jun 16, 2020

Choose a reason for hiding this comment

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

found after Carol's similar change for block init #36146

// A struct with 8 bool fields will require 8 moves instead of one if we do this transformation.
// A simple heuristic when field by field copy is prefered:
// - if fields can be enregistered;
// - if the struct has GCPtrs (block copy would be done via helper that is expensive);
// - if the struct has only one field.
bool dstFldIsProfitable =
((destLclVar != nullptr) &&
(!destLclVar->lvDoNotEnregister || destLclVar->HasGCPtr() || (destLclVar->lvFieldCnt == 1)));
bool srcFldIsProfitable =
((srcLclVar != nullptr) &&
(!srcLclVar->lvDoNotEnregister || srcLclVar->HasGCPtr() || (srcLclVar->lvFieldCnt == 1)));
// Are both dest and src promoted structs?
if (destDoFldAsg && srcDoFldAsg)
if (destDoFldAsg && srcDoFldAsg && (dstFldIsProfitable || srcFldIsProfitable))
{
// Both structs should be of the same type, or have the same number of fields of the same type.
// If not we will use a copy block.
Expand Down Expand Up @@ -10633,13 +10645,7 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
}
}
}
// Are neither dest or src promoted structs?
else if (!destDoFldAsg && !srcDoFldAsg)
{
requiresCopyBlock = true; // Leave as a CopyBlock
JITDUMP(" with no promoted structs");
}
else if (destDoFldAsg)
else if (destDoFldAsg && dstFldIsProfitable)
{
// Match the following kinds of trees:
// fgMorphTree BB01, stmt 9 (before)
Expand Down Expand Up @@ -10683,9 +10689,8 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
}
}
}
else
else if (srcDoFldAsg && srcFldIsProfitable)
{
assert(srcDoFldAsg);
// Check for the symmetric case (which happens for the _pointer field of promoted spans):
//
// [000240] -----+------ /--* lclVar struct(P) V18 tmp9
Expand All @@ -10707,6 +10712,13 @@ GenTree* Compiler::fgMorphCopyBlock(GenTree* tree)
}
}
}
// Are neither dest or src promoted structs?
else
{
assert(!(destDoFldAsg && dstFldIsProfitable) && !(srcDoFldAsg && srcFldIsProfitable));
requiresCopyBlock = true; // Leave as a CopyBlock
JITDUMP(" with no promoted structs");
}
}

// If we require a copy block the set both of the field assign bools to false
Expand Down