Preserve value numbers when morphing multiplication into shift.

This change fixes a morpher transformation of multiplication to shift
to preserve the value number of the tree since the new shift expression
will compute the same value as the old multiplication expression.
Without that change we were getting  asserts in fgMoveOpsLeft,
which expects to see value numbers on trees it transforms.

Closes #2920.

diff --git a/src/jit/compiler.hpp b/src/jit/compiler.hpp
index 10ad0c8..0f398a6 100644 (file)
--- a/src/jit/compiler.hpp
+++ b/src/jit/compiler.hpp
@@ -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));
@@ -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
@@ -1405,9 +1407,15 @@ inline
 void                GenTree::InitNodeSize(){}
 
 inline
-void                GenTree::SetOper(genTreeOps oper)
+void                GenTree::SetOper(genTreeOps oper, ValueNumberUpdate vnUpdate)
 {
     gtOper = oper;
+
+    if (vnUpdate == CLEAR_VN)
+    {
+        // Clear the ValueNum field.
+        gtVNPair.SetBoth(ValueNumStore::NoVN);
+    }
 }
 
 inline
@@ -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...

diff --git a/src/jit/gentree.h b/src/jit/gentree.h
index 0d1f538..beee960 100644 (file)
--- a/src/jit/gentree.h
+++ b/src/jit/gentree.h
@@ -1408,11 +1408,19 @@ public:
     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

diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp
index a3c613b..60fcd05 100644 (file)
--- a/src/jit/morph.cpp
+++ b/src/jit/morph.cpp
@@ -10327,10 +10327,9 @@ SET_OPER:
                         // 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
@@ -10338,9 +10337,6 @@ SET_OPER:
                         {
                             // 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);
@@ -10779,7 +10775,8 @@ CM_ADD_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)
             {
@@ -10814,9 +10811,7 @@ CM_ADD_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())
@@ -10844,11 +10839,24 @@ CM_ADD_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
         }