Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Preserve value numbers when morphing multiplication into shift. #3067

Merged
merged 1 commit into from
Feb 8, 2016
Merged

Preserve value numbers when morphing multiplication into shift. #3067

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 15 additions & 7 deletions src/jit/compiler.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1304,7 +1304,7 @@ inline unsigned Compiler::gtSetEvalOrderAndRestoreFPstkLevel(GenTree * t
/*****************************************************************************/

inline
void GenTree::SetOper(genTreeOps oper)
void GenTree::SetOper(genTreeOps oper, ValueNumberUpdate vnUpdate)
{
assert(((gtFlags & GTF_NODE_SMALL) != 0) !=
((gtFlags & GTF_NODE_LARGE) != 0));
Expand Down Expand Up @@ -1342,9 +1342,11 @@ void GenTree::SetOper(genTreeOps oper)
gtIntCon.gtFieldSeq = NULL;
}

// Clear the ValueNum field as well.
//
gtVNPair.SetBoth(ValueNumStore::NoVN);
if (vnUpdate == CLEAR_VN)
{
// Clear the ValueNum field as well.
gtVNPair.SetBoth(ValueNumStore::NoVN);
}
}

inline
Expand Down Expand Up @@ -1405,9 +1407,15 @@ inline
void GenTree::InitNodeSize(){}

inline
void GenTree::SetOper(genTreeOps oper)
void GenTree::SetOper(genTreeOps oper, ValueNumberUpdate vnUpdate)
{
gtOper = oper;
Copy link

Choose a reason for hiding this comment

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

I don't think we have tried building the JIT without SMALL_TREE_NODES defined in quite sometime.
But you should go ahead and add the if (clearVM) logic here as well as it appears to just be missing.
The instance field gtVNPair is available with and without SMALL_TREE_NODES.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


if (vnUpdate == CLEAR_VN)
{
// Clear the ValueNum field.
gtVNPair.SetBoth(ValueNumStore::NoVN);
}
}

inline
Expand Down Expand Up @@ -1461,11 +1469,11 @@ void GenTree::ChangeOperConst(genTreeOps oper)
}

inline
void GenTree::ChangeOper(genTreeOps oper)
void GenTree::ChangeOper(genTreeOps oper, ValueNumberUpdate vnUpdate)
{
assert(!OperIsConst(oper)); // use ChangeOperLeaf() instead

SetOper(oper);
SetOper(oper, vnUpdate);
gtFlags &= GTF_COMMON_MASK;

// Do "oper"-specific initializations...
Expand Down
12 changes: 10 additions & 2 deletions src/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1408,11 +1408,19 @@ struct GenTree
bool IsNothingNode () const;
void gtBashToNOP ();

void SetOper (genTreeOps oper); // set gtOper
// Value number update action enumeration
enum ValueNumberUpdate
{
CLEAR_VN, // Clear value number
PRESERVE_VN // Preserve value number
};

void SetOper(genTreeOps oper, ValueNumberUpdate vnUpdate = CLEAR_VN); // set gtOper
void SetOperResetFlags (genTreeOps oper); // set gtOper and reset flags

void ChangeOperConst (genTreeOps oper); // ChangeOper(constOper)
void ChangeOper (genTreeOps oper); // set gtOper and only keep GTF_COMMON_MASK flags
// set gtOper and only keep GTF_COMMON_MASK flags
void ChangeOper(genTreeOps oper, ValueNumberUpdate vnUpdate = CLEAR_VN);
void ChangeOperUnchecked (genTreeOps oper);

bool IsLocal() const
Expand Down
36 changes: 22 additions & 14 deletions src/jit/morph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10327,20 +10327,16 @@ GenTreePtr Compiler::fgMorphSmpOp(GenTreePtr tree, MorphAddrContext* ma
// IF we get here we should be changing 'oper'
assert(tree->OperGet() != oper);

ValueNumPair vnp;
vnp = tree->gtVNPair; // Save the existing ValueNumber for 'tree'

tree->SetOper(oper);
// Keep the old ValueNumber for 'tree' as the new expr
// will still compute the same value as before
tree->SetOper(oper, GenTree::PRESERVE_VN);
cns2->gtIntCon.gtIconVal = 0;

// vnStore is null before the ValueNumber phase has run
if (vnStore != nullptr)
{
// Update the ValueNumber for 'cns2', as we just changed it to 0
fgValueNumberTreeConst(cns2);
// Restore the old ValueNumber for 'tree' as the new expr
// will still compute the same value as before
tree->gtVNPair = vnp;
}

op2 = tree->gtOp.gtOp2 = gtFoldExpr(op2);
Expand Down Expand Up @@ -10779,7 +10775,8 @@ CM_OVF_OP :

size_t abs_mult = (mult >= 0) ? mult : -mult;
size_t lowestBit = genFindLowestBit(abs_mult);

bool changeToShift = false;

// is it a power of two? (positive or negative)
if (abs_mult == lowestBit)
{
Expand Down Expand Up @@ -10814,9 +10811,7 @@ CM_OVF_OP :

/* Change the multiplication into a shift by log2(val) bits */
op2->gtIntConCommon.SetIconValue(genLog2(abs_mult));
oper = GT_LSH;
tree->ChangeOper(oper);
goto DONE_MORPHING_CHILDREN;
changeToShift = true;
}
#if LEA_AVAILABLE
else if ((lowestBit > 1) && jitIsScaleIndexMul(lowestBit) && optAvoidIntMult())
Expand Down Expand Up @@ -10844,11 +10839,24 @@ CM_OVF_OP :
fgMorphTreeDone(op1);

op2->gtIntConCommon.SetIconValue(shift);
oper = GT_LSH;
tree->ChangeOper(oper);
changeToShift = true;
}
}

goto DONE_MORPHING_CHILDREN;
if (changeToShift)
{
// vnStore is null before the ValueNumber phase has run
if (vnStore != nullptr)
{
// Update the ValueNumber for 'op2', as we just changed the constant
fgValueNumberTreeConst(op2);
}
oper = GT_LSH;
// Keep the old ValueNumber for 'tree' as the new expr
// will still compute the same value as before
tree->ChangeOper(oper, GenTree::PRESERVE_VN);

goto DONE_MORPHING_CHILDREN;
}
#endif // LEA_AVAILABLE
}
Expand Down