Skip to content

JIT: Improve constant folding for commutative operators #656

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

Closed
EgorBo wants to merge 10 commits into from
Closed

JIT: Improve constant folding for commutative operators #656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4773349
Extend existing constant folding logic for other operators (AND, OR, …
EgorBo Dec 7, 2019
e4160d8
Add tests with T4
EgorBo Dec 7, 2019
960dab6
Fix code format
EgorBo Dec 7, 2019
a6a1afc
Remove tests
EgorBo Dec 7, 2019
8ddbe2c
Merge branch 'master' of github.com:dotnet/runtime into improve-cfold
EgorBo Jan 11, 2020
16754e3
Address feedback
EgorBo Jan 11, 2020
e16225a
Add tests
EgorBo Jan 12, 2020
3e57d3d
Code formatting
EgorBo Jan 12, 2020
409ddf6
Simplify checks
EgorBo Jan 16, 2020
be96dd0
Address feedback
EgorBo Jan 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
1 change: 1 addition & 0 deletions src/coreclr/src/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5437,6 +5437,7 @@ class Compiler
GenTree* fgMorphBlockOperand(GenTree* tree, var_types asgType, unsigned blockWidth, bool isDest);
GenTree* fgMorphCopyBlock(GenTree* tree);
GenTree* fgMorphForRegisterFP(GenTree* tree);
GenTree* fgMorphCommutative(GenTreeOp* tree);
GenTree* fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac = nullptr);
GenTree* fgMorphModToSubMulDiv(GenTreeOp* tree);
GenTree* fgMorphSmpOpOptional(GenTreeOp* tree);
Expand Down
289 changes: 178 additions & 111 deletions src/coreclr/src/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10830,6 +10830,173 @@ GenTree* Compiler::fgMorphFieldAssignToSIMDIntrinsicSet(GenTree* tree)

#endif // FEATURE_SIMD

//------------------------------------------------------------------------
// fgMorphCommutative: Fold constants in a tree with commutative operators (|, ^, &, +)
//
// Arguments:
// tree - This node will be checked for patterns where constants can be folded.
//
// Return Value:
// A GenTree* which points to the new tree with folded constants.
//

GenTree* Compiler::fgMorphCommutative(GenTreeOp* tree)
{
assert(tree->OperIs(GT_OR, GT_XOR, GT_AND, GT_ADD));

GenTree* op1 = tree->gtGetOp1();
GenTree* op2 = tree->gtGetOp2();
genTreeOps oper = tree->OperGet();

// Fold (X <op> C1) <op> (Y <op> C2)
// to (X <op> Y) <op> (C1 <op> C2)
if (op1->OperIs(oper) && op2->OperIs(oper) &&
op1->AsOp()->gtGetOp2()->IsCnsIntOrI() && op2->AsOp()->gtGetOp2()->IsCnsIntOrI() &&
!gtIsActiveCSE_Candidate(op1) && !gtIsActiveCSE_Candidate(op2))
{
// Don't create a byref pointer that may point outside of the ref object.
// If a GC happens, the byref won't get updated. This can happen if one
// of the int components is negative. It also requires the address generation
// be in a fully-interruptible code region.
if (!varTypeIsGC(op1->AsOp()->gtGetOp1()->TypeGet()) && !varTypeIsGC(op2->AsOp()->gtGetOp1()->TypeGet()))
Copy link
Contributor

@sandreenko sandreenko Jan 13, 2020

Choose a reason for hiding this comment

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

here you check varTypeIsGC(x), varTypeIsGC(y), gtIsActiveCSE_Candidate(op2).

Copy link
Member Author

@EgorBo EgorBo Jan 16, 2020

Choose a reason for hiding this comment

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

Added also gtIsActiveCSE_Candidate(op1) here

{
GenTreeIntCon* cns1 = op1->AsOp()->gtGetOp2()->AsIntCon();
GenTreeIntCon* cns2 = op2->AsOp()->gtGetOp2()->AsIntCon();

if (oper == GT_ADD)
{
if (tree->gtOverflow() || op1->gtOverflow() || op2->gtOverflow())
{
return tree;
}
cns1->SetIconValue(cns1->IconValue() + cns2->IconValue());
}
else if (oper == GT_OR)
{
cns1->SetIconValue(cns1->IconValue() | cns2->IconValue());
}
else if (oper == GT_XOR)
{
cns1->SetIconValue(cns1->IconValue() ^ cns2->IconValue());
}
else if (oper == GT_AND)
{
cns1->SetIconValue(cns1->IconValue() & cns2->IconValue());
}
else
{
noway_assert(!"unexpected operator");
}
#ifdef _TARGET_64BIT_
if (cns1->TypeGet() == TYP_INT)
{
// we need to properly re-sign-extend or truncate after adding two int constants above
cns1->TruncateOrSignExtend32();
}
#endif //_TARGET_64BIT_

tree->gtOp2 = cns1;
DEBUG_DESTROY_NODE(cns2);

op1->AsOp()->gtOp2 = op2->AsOp()->gtOp1;
op1->gtFlags |= (op1->AsOp()->gtOp2->gtFlags & GTF_ALL_EFFECT);
DEBUG_DESTROY_NODE(op2);
op2 = tree->gtOp2;
}
}

if (op2->IsCnsIntOrI() && varTypeIsIntegralOrI(tree->TypeGet()))
{
// Fold (X <op> C1) <op> C2
// to (X <op> (C1 <op> C2)
if (op1->OperIs(oper) && !gtIsActiveCSE_Candidate(op1) && op1->AsOp()->gtGetOp2()->IsCnsIntOrI() &&
Copy link
Contributor

@sandreenko sandreenko Jan 13, 2020

Choose a reason for hiding this comment

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

here you check varTypeIsGC(icon1)? why does it need that check? Why don't you need to check x here as you do in the previous case?
You check gtIsActiveCSE_Candidate(op1), in the previous case you were checking gtIsActiveCSE_Candidate(op2), what is the difference?

Copy link
Member Author

@EgorBo EgorBo Jan 16, 2020

Choose a reason for hiding this comment

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

removed varTypeIsGC(icon1) and added gtIsActiveCSE_Candidate(op1) to the first expression

(op1->AsOp()->gtGetOp2()->OperGet() == op2->OperGet()) &&
!varTypeIsGC(op1->AsOp()->gtGetOp2()->TypeGet()))
{
GenTreeIntConCommon* cns1 = op1->AsOp()->gtGetOp2()->AsIntConCommon();
GenTreeIntConCommon* cns2 = op2->AsIntConCommon();

if (oper == GT_ADD)
Copy link
Contributor

@sandreenko sandreenko Jan 13, 2020

Choose a reason for hiding this comment

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

that code duplication doesn't look cool, but a function with 6 arguments GenTreeIntConCommon* fgGetCommutativeResult(oper, icon1, icon2, tree, op1, optional op2) also doesn't. I am not sure here.

Copy link
Member Author

@EgorBo EgorBo Jan 16, 2020

Choose a reason for hiding this comment

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

Neither am I, tried to simplify it a bit. Let me know if I should extract it to a separate method.

{
if (tree->gtOverflow() || op1->gtOverflow())
{
return tree;
}
cns2->SetIconValue(cns1->IconValue() + cns2->IconValue());
}
else if (oper == GT_OR)
{
cns2->SetIconValue(cns1->IconValue() | cns2->IconValue());
}
else if (oper == GT_XOR)
{
cns2->SetIconValue(cns1->IconValue() ^ cns2->IconValue());
}
else if (oper == GT_AND)
{
cns2->SetIconValue(cns1->IconValue() & cns2->IconValue());
}
else
{
noway_assert(!"unexpected operator");
}
#ifdef _TARGET_64BIT_
if (op2->TypeGet() == TYP_INT)
{
// we need to properly re-sign-extend or truncate after folding two int constants above
op2->AsIntCon()->TruncateOrSignExtend32();
}
#endif //_TARGET_64BIT_

if (cns1->OperGet() == GT_CNS_INT)
{
op2->AsIntCon()->gtFieldSeq =
GetFieldSeqStore()->Append(cns1->AsIntCon()->gtFieldSeq, op2->AsIntCon()->gtFieldSeq);
}

DEBUG_DESTROY_NODE(cns1);
tree->gtOp1 = op1->AsOp()->gtOp1;
DEBUG_DESTROY_NODE(op1);
op1 = tree->gtOp1;
}

// Fold X <op> 0
// to X
if (tree->OperIs(GT_ADD, GT_OR, GT_XOR) && (op2->AsIntConCommon()->IconValue() == 0) &&
!gtIsActiveCSE_Candidate(tree))
{
// If this addition is adding an offset to a null pointer,
// avoid the work and yield the null pointer immediately.
// Dereferencing the pointer in either case will have the
// same effect.
if (!optValnumCSE_phase && varTypeIsGC(op2->TypeGet()) && ((op1->gtFlags & GTF_ALL_EFFECT) == 0))
{
op2->gtType = tree->gtType;
DEBUG_DESTROY_NODE(op1);
DEBUG_DESTROY_NODE(tree);
return op2;
}

// Remove the addition if it won't change the tree type
// to TYP_REF.
if (!gtIsActiveCSE_Candidate(op2) && ((op1->TypeGet() == tree->TypeGet()) || (op1->TypeGet() != TYP_REF)))
{
if (fgGlobalMorph && (op2->OperGet() == GT_CNS_INT) && (op2->AsIntCon()->gtFieldSeq != nullptr) &&
(op2->AsIntCon()->gtFieldSeq != FieldSeqStore::NotAField()))
{
fgAddFieldSeqForZeroOffset(op1, op2->AsIntCon()->gtFieldSeq);
}

DEBUG_DESTROY_NODE(op2);
DEBUG_DESTROY_NODE(tree);
return op1;
}
}
}

return tree;
}

/*****************************************************************************
*
* Transform the given GTK_SMPOP tree for code generation.
Expand Down Expand Up @@ -11871,7 +12038,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
*/

GenTree* temp;
GenTree* cns1;
GenTree* cns2;
size_t ival1, ival2;
GenTree* lclVarTree;
Expand Down Expand Up @@ -12524,119 +12690,20 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
op2 = tree->AsOp()->gtOp2;
}

/* See if we can fold GT_ADD nodes. */

if (oper == GT_ADD)
// See if we can fold constants for commutative operators.
if (tree->OperIs(GT_OR, GT_XOR, GT_AND, GT_ADD))
{
/* Fold "((x+icon1)+(y+icon2)) to ((x+y)+(icon1+icon2))" */

if (op1->gtOper == GT_ADD && op2->gtOper == GT_ADD && !gtIsActiveCSE_Candidate(op2) &&
op1->AsOp()->gtOp2->gtOper == GT_CNS_INT && op2->AsOp()->gtOp2->gtOper == GT_CNS_INT &&
!op1->gtOverflow() && !op2->gtOverflow())
{
// Don't create a byref pointer that may point outside of the ref object.
// If a GC happens, the byref won't get updated. This can happen if one
// of the int components is negative. It also requires the address generation
// be in a fully-interruptible code region.
if (!varTypeIsGC(op1->AsOp()->gtOp1->TypeGet()) && !varTypeIsGC(op2->AsOp()->gtOp1->TypeGet()))
{
cns1 = op1->AsOp()->gtOp2;
cns2 = op2->AsOp()->gtOp2;
cns1->AsIntCon()->gtIconVal += cns2->AsIntCon()->gtIconVal;
#ifdef _TARGET_64BIT_
if (cns1->TypeGet() == TYP_INT)
{
// we need to properly re-sign-extend or truncate after adding two int constants above
cns1->AsIntCon()->TruncateOrSignExtend32();
}
#endif //_TARGET_64BIT_

tree->AsOp()->gtOp2 = cns1;
DEBUG_DESTROY_NODE(cns2);

op1->AsOp()->gtOp2 = op2->AsOp()->gtOp1;
op1->gtFlags |= (op1->AsOp()->gtOp2->gtFlags & GTF_ALL_EFFECT);
DEBUG_DESTROY_NODE(op2);
op2 = tree->AsOp()->gtOp2;
}
}

if (op2->IsCnsIntOrI() && varTypeIsIntegralOrI(typ))
tree = fgMorphCommutative(tree->AsOp());
if (!tree->OperIsBinary()) // e.g. fgMorphCommutative optimized `x + 0` into `x`
{
/* Fold "((x+icon1)+icon2) to (x+(icon1+icon2))" */
CLANG_FORMAT_COMMENT_ANCHOR;

if (op1->gtOper == GT_ADD && //
!gtIsActiveCSE_Candidate(op1) && //
!op1->gtOverflow() && //
op1->AsOp()->gtOp2->IsCnsIntOrI() && //
(op1->AsOp()->gtOp2->OperGet() == op2->OperGet()) && //
(op1->AsOp()->gtOp2->TypeGet() != TYP_REF) && // Don't fold REFs
(op2->TypeGet() != TYP_REF)) // Don't fold REFs
{
cns1 = op1->AsOp()->gtOp2;
op2->AsIntConCommon()->SetIconValue(cns1->AsIntConCommon()->IconValue() +
op2->AsIntConCommon()->IconValue());
#ifdef _TARGET_64BIT_
if (op2->TypeGet() == TYP_INT)
{
// we need to properly re-sign-extend or truncate after adding two int constants above
op2->AsIntCon()->TruncateOrSignExtend32();
}
#endif //_TARGET_64BIT_

if (cns1->OperGet() == GT_CNS_INT)
{
op2->AsIntCon()->gtFieldSeq =
GetFieldSeqStore()->Append(cns1->AsIntCon()->gtFieldSeq, op2->AsIntCon()->gtFieldSeq);
}
DEBUG_DESTROY_NODE(cns1);

tree->AsOp()->gtOp1 = op1->AsOp()->gtOp1;
DEBUG_DESTROY_NODE(op1);
op1 = tree->AsOp()->gtOp1;
}

// Fold (x + 0).

if ((op2->AsIntConCommon()->IconValue() == 0) && !gtIsActiveCSE_Candidate(tree))
{

// If this addition is adding an offset to a null pointer,
// avoid the work and yield the null pointer immediately.
// Dereferencing the pointer in either case will have the
// same effect.

if (!optValnumCSE_phase && varTypeIsGC(op2->TypeGet()) &&
((op1->gtFlags & GTF_ALL_EFFECT) == 0))
{
op2->gtType = tree->gtType;
DEBUG_DESTROY_NODE(op1);
DEBUG_DESTROY_NODE(tree);
return op2;
}

// Remove the addition iff it won't change the tree type
// to TYP_REF.

if (!gtIsActiveCSE_Candidate(op2) &&
((op1->TypeGet() == tree->TypeGet()) || (op1->TypeGet() != TYP_REF)))
{
if (fgGlobalMorph && (op2->OperGet() == GT_CNS_INT) &&
(op2->AsIntCon()->gtFieldSeq != nullptr) &&
(op2->AsIntCon()->gtFieldSeq != FieldSeqStore::NotAField()))
{
fgAddFieldSeqForZeroOffset(op1, op2->AsIntCon()->gtFieldSeq);
}

DEBUG_DESTROY_NODE(op2);
DEBUG_DESTROY_NODE(tree);

return op1;
}
}
return tree;
}
op1 = tree->gtGetOp1();
op2 = tree->AsOp()->gtGetOp2();
oper = tree->OperGet();
typ = tree->TypeGet();
}

/* See if we can fold GT_MUL by const nodes */
else if (oper == GT_MUL && op2->IsCnsIntOrI() && !optValnumCSE_phase)
{
Expand Down Expand Up @@ -12761,7 +12828,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac)
goto DONE_MORPHING_CHILDREN;
}
}
else if (fgOperIsBitwiseRotationRoot(oper))
if (fgOperIsBitwiseRotationRoot(oper))
{
tree = fgRecognizeAndMorphBitwiseRotation(tree);

Expand Down
Loading