Merge pull request #3067 from erozenfeld/MultToShiftVN

Preserve value numbers when morphing multiplication into shift.

Author: erozenfeld

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...

src/jit/gentree.h

@@ -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

src/jit/morph.cpp

@@ -10324,20 +10324,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);
@@ -10776,7 +10772,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)
             {
@@ -10811,9 +10808,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())
@@ -10841,11 +10836,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
         }