Fix #3561: assert on RyuJIT x86 when generating shl by 1

This assert hit in the encoder when we were trying to generate an
INS_shl_N with a constant of 1, instead of using the special xarch
INS_shl_1 encoding, which saves a byte. It turns out, the assert was
and in fact amd64 does generate the suboptimal encoding currently.

The bad code occurs in the RMW case of genCodeForShift(). It turns out
that function is unnecessarily complex, unique (it doesn't use the
common RMW code paths), and has a number of other latent bugs.

To fix this, I split genCodeForShift() by leaving the non-RMW case
there, and adding a genCodeForShiftRMW() function just for the RMW case.
I rewrote the RMW case to use the existing emitInsRMW functions.

Other related cleanups along the way:
1. I changed emitHandleMemOp to consistently set the idInsFmt field,
and changed all callers to stop pre-setting or post-setting this field.
This makes the API much easier to understand. I added a big header
comment for the function. Now, it takes a "basic" insFmt (using ARD,
AWR, or ARW forms), which might be munged to a MRD/MWR/MRW form
if necessary.
2. I changed some places to always use the most derived GenTree type
for all uses. For instance, if the code has
"GenTreeIndir* mem = node->AsIndir()", then always use "mem" from then
on, and don't use "node". I changed some functions to take more derived
GenTree node types.
3. I rewrote the emitInsRMW() functions to be much simpler, and rewrote
their header comments.
4. I added GenTree predicates OperIsShift(), OperIsRotate(), and
OperIsShiftOrRotate().
5. I added function genMapShiftInsToShiftByConstantIns() to encapsulate
mapping from INS_shl to INS_shl_N or INS_shl_1 based on a constant.
This code was in 3 different places already.
6. The change in assertionprop.cpp is simply to make JitDumps readable.

In addition to fixing the bug for RyuJIT/x86, there are a small number
of x64 diffs where we now generate smaller encodings for shift by 1.

16 files changed, 489 insertions, 547 deletions

diff --git a/src/jit/assertionprop.cpp b/src/jit/assertionprop.cpp
index cb6f12b1ab..ac1a794b6c 100644
--- a/src/jit/assertionprop.cpp
+++ b/src/jit/assertionprop.cpp
@@ -732,7 +732,7 @@ void Compiler::optPrintAssertion(AssertionDsc*  curAssertion, AssertionIndex ass
 
     if (assertionIndex > 0)
     {
-        printf("index=#%02u, mask=", assertionIndex);
+        printf(" index=#%02u, mask=", assertionIndex);
         
         // This is an hack to reuse a known empty set in order to display 
         // a single bit mask.
diff --git a/src/jit/codegen.h b/src/jit/codegen.h
index 1a36c4200d..d6e69c5de6 100644
--- a/src/jit/codegen.h
+++ b/src/jit/codegen.h
@@ -1143,6 +1143,10 @@ public :
     void    __cdecl     instDisp(instruction ins, bool noNL, const char *fmt, ...);
 #endif
 
+#ifdef _TARGET_XARCH_
+    instruction         genMapShiftInsToShiftByConstantIns(instruction ins, int shiftByValue);
+#endif // _TARGET_XARCH_
+
 };
 
 /*XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
diff --git a/src/jit/codegenarm.cpp b/src/jit/codegenarm.cpp
index 4a9348c26f..53b7315ea2 100644
--- a/src/jit/codegenarm.cpp
+++ b/src/jit/codegenarm.cpp
@@ -1152,7 +1152,7 @@ CodeGen::genCodeForTreeNode(GenTreePtr treeNode)
     case GT_LSH:
     case GT_RSH:
     case GT_RSZ:
-        genCodeForShift(treeNode->gtGetOp1(), treeNode->gtGetOp2(), treeNode);
+        genCodeForShift(treeNode);
         // genCodeForShift() calls genProduceReg()
         break;
 
@@ -1746,22 +1746,17 @@ instruction CodeGen::genGetInsForOper(genTreeOps oper, var_types type)
     return ins;
 }
 
-/** Generates the code sequence for a GenTree node that
- * represents a bit shift operation (<<, >>, >>>).
- *
- * Arguments: operand:  the value to be shifted by shiftBy bits.
- *            shiftBy:  the number of bits to shift the operand.
- *            parent:   the actual bitshift node (that specifies the
- *                      type of bitshift to perform.
- *
- * Preconditions:    a) All GenTrees are register allocated.
- *                   b) Either shiftBy is a contained constant or
- *                      it's an expression sitting in RCX.
- *                   c) The actual bit shift node is not stack allocated
- *                      nor contained (not yet supported).
- */
-void CodeGen::genCodeForShift(GenTreePtr operand, GenTreePtr shiftBy,
-                              GenTreePtr parent)
+//------------------------------------------------------------------------
+// genCodeForShift: Generates the code sequence for a GenTree node that
+// represents a bit shift or rotate operation (<<, >>, >>>, rol, ror).
+//
+// Arguments:
+//    tree - the bit shift node (that specifies the type of bit shift to perform).
+//
+// Assumptions:
+//    a) All GenTrees are register allocated.
+//
+void CodeGen::genCodeForShift(GenTreePtr tree)
 {
     NYI("genCodeForShift");
 }
diff --git a/src/jit/codegenarm64.cpp b/src/jit/codegenarm64.cpp
index dc13b31746..0791c73371 100644
--- a/src/jit/codegenarm64.cpp
+++ b/src/jit/codegenarm64.cpp
@@ -2560,7 +2560,7 @@ CodeGen::genCodeForTreeNode(GenTreePtr treeNode)
     case GT_RSH:
     case GT_RSZ:
     case GT_ROR:
-        genCodeForShift(treeNode->gtGetOp1(), treeNode->gtGetOp2(), treeNode);
+        genCodeForShift(treeNode);
         // genCodeForShift() calls genProduceReg()
         break;
 
@@ -4594,46 +4594,43 @@ instruction CodeGen::genGetInsForOper(genTreeOps oper, var_types type)
     return ins;
 }
 
-/** Generates the code sequence for a GenTree node that
- * represents a bit shift operation (<<, >>, >>>).
- *
- * Arguments: operand:  the value to be shifted by shiftBy bits.
- *            shiftBy:  the number of bits to shift the operand.
- *            parent:   the actual bitshift node (that specifies the
- *                      type of bitshift to perform.
- *
- * Preconditions:    a) All GenTrees are register allocated.
- *                   b) Either shiftBy is a contained constant or
- *                      it's an expression sitting in RCX.
- *                   c) The actual bit shift node is not stack allocated
- *                      nor contained (not yet supported).
- */
-void CodeGen::genCodeForShift(GenTreePtr operand,
-                              GenTreePtr shiftBy,
-                              GenTreePtr parent)
+//------------------------------------------------------------------------
+// genCodeForShift: Generates the code sequence for a GenTree node that
+// represents a bit shift or rotate operation (<<, >>, >>>, rol, ror).
+//
+// Arguments:
+//    tree - the bit shift node (that specifies the type of bit shift to perform).
+//
+// Assumptions:
+//    a) All GenTrees are register allocated.
+//
+void CodeGen::genCodeForShift(GenTreePtr tree)
 {
-    var_types targetType = parent->TypeGet();
-    genTreeOps oper = parent->OperGet();
+    var_types targetType = tree->TypeGet();
+    genTreeOps oper = tree->OperGet();
     instruction ins = genGetInsForOper(oper, targetType);
-    emitAttr size = emitTypeSize(parent);
+    emitAttr size = emitTypeSize(tree);
 
-    assert(parent->gtRegNum != REG_NA);
+    assert(tree->gtRegNum != REG_NA);
+
+    GenTreePtr operand = tree->gtGetOp1();
     genConsumeReg(operand);
     
+    GenTreePtr shiftBy = tree->gtGetOp2();
     if (!shiftBy->IsCnsIntOrI())
     {
         genConsumeReg(shiftBy);
-        getEmitter()->emitIns_R_R_R(ins, size, parent->gtRegNum, operand->gtRegNum, shiftBy->gtRegNum);
+        getEmitter()->emitIns_R_R_R(ins, size, tree->gtRegNum, operand->gtRegNum, shiftBy->gtRegNum);
     }
     else
     {
         unsigned immWidth   = emitter::getBitWidth(size);                  // immWidth will be set to 32 or 64
         ssize_t  shiftByImm = shiftBy->gtIntCon.gtIconVal & (immWidth-1);
         
-        getEmitter()->emitIns_R_R_I(ins, size, parent->gtRegNum, operand->gtRegNum, shiftByImm);
+        getEmitter()->emitIns_R_R_I(ins, size, tree->gtRegNum, operand->gtRegNum, shiftByImm);
     }
 
-    genProduceReg(parent);
+    genProduceReg(tree);
 }
 
 // TODO-Cleanup: move to CodeGenCommon.cpp
diff --git a/src/jit/codegencommon.cpp b/src/jit/codegencommon.cpp
index 7daaff4826..69b7841a78 100644
--- a/src/jit/codegencommon.cpp
+++ b/src/jit/codegencommon.cpp
@@ -11422,6 +11422,66 @@ unsigned Compiler::GetHfaSlots(CORINFO_CLASS_HANDLE hClass)
 
 #endif // _TARGET_ARM_
 
+
+#ifdef _TARGET_XARCH_
+
+//------------------------------------------------------------------------
+// genMapShiftInsToShiftByConstantIns: Given a general shift/rotate instruction,
+// map it to the specific x86/x64 shift opcode for a shift/rotate by a constant.
+// X86/x64 has a special encoding for shift/rotate-by-constant-1.
+//
+// Arguments:
+//    ins: the base shift/rotate instruction
+//    shiftByValue: the constant value by which we are shifting/rotating
+//
+instruction CodeGen::genMapShiftInsToShiftByConstantIns(instruction ins, int shiftByValue)
+{
+    assert(ins == INS_rcl  ||
+           ins == INS_rcr  ||
+           ins == INS_rol  ||
+           ins == INS_ror  ||
+           ins == INS_shl  ||
+           ins == INS_shr  ||
+           ins == INS_sar);
+
+    // Which format should we use?
+
+    instruction shiftByConstantIns;
+
+    if (shiftByValue == 1)
+    {
+        // Use the shift-by-one format.
+
+        assert(INS_rcl + 1 == INS_rcl_1);
+        assert(INS_rcr + 1 == INS_rcr_1);
+        assert(INS_rol + 1 == INS_rol_1);
+        assert(INS_ror + 1 == INS_ror_1);
+        assert(INS_shl + 1 == INS_shl_1);
+        assert(INS_shr + 1 == INS_shr_1);
+        assert(INS_sar + 1 == INS_sar_1);
+
+        shiftByConstantIns = (instruction)(ins + 1);
+    }
+    else
+    {
+        // Use the shift-by-NNN format.
+
+        assert(INS_rcl + 2 == INS_rcl_N);
+        assert(INS_rcr + 2 == INS_rcr_N);
+        assert(INS_rol + 2 == INS_rol_N);
+        assert(INS_ror + 2 == INS_ror_N);
+        assert(INS_shl + 2 == INS_shl_N);
+        assert(INS_shr + 2 == INS_shr_N);
+        assert(INS_sar + 2 == INS_sar_N);
+
+        shiftByConstantIns = (instruction)(ins + 2);
+    }
+
+    return shiftByConstantIns;
+}
+
+#endif // _TARGET_XARCH_
+
 /*****************************************************************************/
 #ifdef DEBUGGING_SUPPORT
 
diff --git a/src/jit/codegenlegacy.cpp b/src/jit/codegenlegacy.cpp
index 5a0198f806..a68ddafb1e 100644
--- a/src/jit/codegenlegacy.cpp
+++ b/src/jit/codegenlegacy.cpp
@@ -8593,9 +8593,7 @@ void                CodeGen::genCodeForShift(GenTreePtr tree,
                                              regMaskTP  destReg,
                                              regMaskTP  bestReg)
 {
-    assert(tree->OperGet() == GT_LSH ||
-           tree->OperGet() == GT_RSH ||
-           tree->OperGet() == GT_RSZ);
+    assert(tree->OperIsShift());
 
     const genTreeOps oper    = tree->OperGet();
     GenTreePtr      op1      = tree->gtOp.gtOp1;
diff --git a/src/jit/codegenlinear.h b/src/jit/codegenlinear.h
index dfc5372a02..b72fbc3571 100644
--- a/src/jit/codegenlinear.h
+++ b/src/jit/codegenlinear.h
@@ -138,9 +138,11 @@
                                              var_types      type  = TYP_INT,
                                              insFlags       flags = INS_FLAGS_DONT_CARE);
 
-    void                genCodeForShift     (GenTreePtr dst,
-                                             GenTreePtr src,
-                                             GenTreePtr treeNode);
+    void                genCodeForShift          (GenTreePtr tree);
+
+#ifdef _TARGET_XARCH_
+    void                genCodeForShiftRMW       (GenTreeStoreInd* storeInd);
+#endif // _TARGET_XARCH_
 
     void                genCodeForCpObj          (GenTreeCpObj* cpObjNode);
 
diff --git a/src/jit/codegenxarch.cpp b/src/jit/codegenxarch.cpp
index 041e321a5b..99af71dc9d 100644
--- a/src/jit/codegenxarch.cpp
+++ b/src/jit/codegenxarch.cpp
@@ -1825,7 +1825,7 @@ CodeGen::genCodeForTreeNode(GenTreePtr treeNode)
     case GT_RSZ:
     case GT_ROL:
     case GT_ROR:
-        genCodeForShift(treeNode->gtGetOp1(), treeNode->gtGetOp2(), treeNode);
+        genCodeForShift(treeNode);
         // genCodeForShift() calls genProduceReg()
         break;
 
@@ -4247,241 +4247,131 @@ instruction CodeGen::genGetInsForOper(genTreeOps oper, var_types type)
     return ins;
 }
 
-/** Generates the code sequence for a GenTree node that
- * represents a bit shift or rotate operation (<<, >>, >>>, rol, ror).
- *
- * Arguments: operand:  the value to be shifted or rotated by shiftBy bits.
- *            shiftBy:  the number of bits to shift or rotate the operand.
- *            parent:   the actual bitshift node (that specifies the
- *                      type of bitshift to perform.
- *
- * Preconditions:    a) All GenTrees are register allocated.
- *                   b) Either shiftBy is a contained constant or
- *                      it's an expression sitting in RCX.
- */
-void CodeGen::genCodeForShift(GenTreePtr operand, GenTreePtr shiftBy,
-                              GenTreePtr parent)
+
+//------------------------------------------------------------------------
+// genCodeForShift: Generates the code sequence for a GenTree node that
+// represents a bit shift or rotate operation (<<, >>, >>>, rol, ror).
+//
+// Arguments:
+//    tree - the bit shift node (that specifies the type of bit shift to perform).
+//
+// Assumptions:
+//    a) All GenTrees are register allocated.
+//    b) The shift-by-amount in tree->gtOp.gtOp2 is either a contained constant or
+//       it's a register-allocated expression. If it is in a register that is
+//       not RCX, it will be moved to RCX (so RCX better not be in use!).
+//
+void CodeGen::genCodeForShift(GenTreePtr tree)
 {
-    var_types targetType = parent->TypeGet();
-    genTreeOps oper = parent->OperGet();
-    instruction ins = genGetInsForOper(oper, targetType);
-    GenTreePtr actualOperand = operand->gtSkipReloadOrCopy();
-    
-    bool isRMW = parent->gtOp.gtOp1->isContained();
-    assert(parent->gtRegNum != REG_NA || isRMW);
+    // Only the non-RMW case here.
+    assert(tree->OperIsShiftOrRotate());
+    assert(!tree->gtOp.gtOp1->isContained());
+    assert(tree->gtRegNum != REG_NA);
+
+    genConsumeOperands(tree->AsOp());
+
+    var_types targetType = tree->TypeGet();
+    instruction ins = genGetInsForOper(tree->OperGet(), targetType);
 
-    regNumber operandReg = REG_NA;
-    regNumber indexReg = REG_NA;
-    int offset  = 0;
-    ssize_t disp = 0;
-    emitAttr attr = EA_UNKNOWN;
-    bool isClsVarAddr = (operand->OperGet() == GT_CLS_VAR_ADDR);
-    bool isLclVarAddr = (operand->OperGet() == GT_LCL_VAR_ADDR);
-    bool isCnsIntOrIAndFitsWithinAddrBase = false;
+    GenTreePtr operand = tree->gtGetOp1();
+    regNumber operandReg = operand->gtRegNum;
 
-    if (!isRMW)
+    GenTreePtr shiftBy = tree->gtGetOp2();
+    if (shiftBy->isContainedIntOrIImmed())
     {
-        genConsumeOperands(parent->AsOp());
-        operandReg = operand->gtRegNum;
+        // First, move the operand to the destination register and
+        // later on perform the shift in-place.
+        // (LSRA will try to avoid this situation through preferencing.)
+        if (tree->gtRegNum != operandReg)
+        {
+            inst_RV_RV(INS_mov, tree->gtRegNum, operandReg, targetType);
+        }
+
+        int shiftByValue = (int)shiftBy->AsIntConCommon()->IconValue();
+        inst_RV_SH(ins, emitTypeSize(tree), tree->gtRegNum, shiftByValue);
     }
     else
     {
-        targetType = parent->gtOp.gtOp1->TypeGet();
-        attr = EA_ATTR(genTypeSize(targetType));
-
-        if (actualOperand->OperGet() == GT_LCL_VAR)
+        // We must have the number of bits to shift stored in ECX, since we constrained this node to
+        // sit in ECX. In case this didn't happen, LSRA expects the code generator to move it since it's a single
+        // register destination requirement.
+        regNumber shiftReg = shiftBy->gtRegNum;
+        if (shiftReg != REG_RCX)
         {
-            operandReg = operand->gtRegNum;
+            // Issue the mov to RCX:
+            inst_RV_RV(INS_mov, REG_RCX, shiftReg, shiftBy->TypeGet());
         }
-        else if (actualOperand->OperGet() == GT_LEA)
-        {
-            operandReg = actualOperand->gtOp.gtOp1->gtRegNum;
-            GenTreeAddrMode* addrMode = actualOperand->AsAddrMode();
-            offset = addrMode->gtOffset;
-            if(addrMode->Index() != nullptr)
-            {
-                indexReg = addrMode->Index()->gtRegNum;
 
-                // GT_LEA with an indexReg is not supported for shift by immediate
-                assert(!shiftBy->isContainedIntOrIImmed());
-            }
-        }
-        else if (actualOperand->IsCnsIntOrI())
+        // The operand to be shifted must not be in ECX
+        noway_assert(operandReg != REG_RCX);
+
+        if (tree->gtRegNum != operandReg)
         {
-            GenTreeIntConCommon* intCon = actualOperand->AsIntConCommon();
-            if (actualOperand->isContained())
-            {
-                // Contained absolute address should fit within addr base
-                assert(intCon->FitsInAddrBase(compiler));
+            inst_RV_RV(INS_mov, tree->gtRegNum, operandReg, targetType);
+        }
+        inst_RV_CL(ins, tree->gtRegNum, targetType);
+    }
 
-                // Don't expect to see GT_COPY or GT_RELOAD
-                assert(operand == actualOperand);
+    genProduceReg(tree);
+}
 
-                isCnsIntOrIAndFitsWithinAddrBase = true;
-                disp = intCon->IconValue(); 
 
-                if (intCon->AddrNeedsReloc(compiler))
-                {
-                    attr = EA_SET_FLG(attr, EA_DSP_RELOC_FLG);
-                }
-            }
-            else
-            {
-                operandReg = operand->gtRegNum;
-            }
-        }
-        else 
-        {
-            // The only other supported operands for RMW are GT_CLS_VAR_ADDR and GT_LCL_VAR_ADDR
-            assert(actualOperand->OperGet() == GT_CLS_VAR_ADDR || actualOperand->OperGet() == GT_LCL_VAR_ADDR);
+//------------------------------------------------------------------------
+// genCodeForShiftRMW: Generates the code sequence for a GT_STOREIND GenTree node that
+// represents a RMW bit shift or rotate operation (<<, >>, >>>, rol, ror), for example:
+//      GT_STOREIND( AddressTree, GT_SHL( Ind ( AddressTree ), Operand ) )
+//
+// Arguments:
+//    storeIndNode: the GT_STOREIND node.
+//
+void CodeGen::genCodeForShiftRMW(GenTreeStoreInd* storeInd)
+{
+    GenTree* data = storeInd->Data();
+    GenTree* addr = storeInd->Addr();
 
-            // We don't expect to see GT_COPY or GT_RELOAD for GT_CLS_VAR_ADDR and GT_LCL_VAR_ADDR
-            // so 'actualOperand' should be the same as 'operand' 
-            assert(operand == actualOperand);
-        }        
-    }
-        
+    assert(data->OperIsShiftOrRotate());
+
+    // This function only handles the RMW case.
+    assert(data->gtOp.gtOp1->isContained());
+    assert(data->gtOp.gtOp1->isIndir());
+    assert(Lowering::IndirsAreEquivalent(data->gtOp.gtOp1, storeInd));
+    assert(data->gtRegNum == REG_NA);
+
+    var_types targetType = data->TypeGet();
+    genTreeOps oper = data->OperGet();
+    instruction ins = genGetInsForOper(oper, targetType);
+    emitAttr attr = EA_ATTR(genTypeSize(targetType));
+
+    GenTree* shiftBy = data->gtOp.gtOp2;
     if (shiftBy->isContainedIntOrIImmed())
     {
         int shiftByValue = (int)shiftBy->AsIntConCommon()->IconValue();
-
-        if (!isRMW)
+        ins = genMapShiftInsToShiftByConstantIns(ins, shiftByValue);
+        if (shiftByValue == 1)
         {
-            // First, move the operand to the destination register and
-            // later on perform the shift in-place.
-            // (LSRA will try to avoid this situation through preferencing.)
-            if (parent->gtRegNum != operandReg)
-            {
-                inst_RV_RV(INS_mov, parent->gtRegNum, operandReg, targetType);
-            }
-
-            inst_RV_SH(ins, emitTypeSize(parent), parent->gtRegNum, shiftByValue);
+            // There is no source in this case, as the shift by count is embedded in the instruction opcode itself.
+            getEmitter()->emitInsRMW(ins, attr, storeInd);
         }
         else
         {
-            if ((isClsVarAddr || isLclVarAddr) && shiftByValue == 1)
-            {   
-                switch (ins)
-                {
-                case INS_sar:
-                    ins = INS_sar_1;
-                    break;
-                case INS_shl:
-                    ins = INS_shl_1;
-                    break;
-                case INS_shr:
-                    ins = INS_shr_1;
-                    break;
-                case INS_rol:
-                    ins = INS_rol_1;
-                    break;
-                case INS_ror:
-                    ins = INS_ror_1;
-                    break;
-                default:
-                    // leave 'ins' unchanged
-                    break;
-                }
-
-                if (isClsVarAddr)
-                {
-                getEmitter()->emitIns_C(ins, attr, operand->gtClsVar.gtClsVarHnd, 0);
-            }
-            else
-            {
-                    getEmitter()->emitIns_S(ins, attr, operand->gtLclVarCommon.gtLclNum, 0);
-                }
-            }
-            else
-            {
-                switch (ins)
-                {
-                case INS_sar:
-                    ins = INS_sar_N;
-                    break;
-                case INS_shl:
-                    ins = INS_shl_N;
-                    break;
-                case INS_shr:
-                    ins = INS_shr_N;
-                    break;
-                case INS_rol:
-                    ins = INS_rol_N;
-                    break;
-                case INS_ror:
-                    ins = INS_ror_N;
-                    break;
-                default:
-                    // leave 'ins' unchanged
-                    break;
-                }
-                if (isClsVarAddr)
-                {
-                    getEmitter()->emitIns_C_I(ins, attr, operand->gtClsVar.gtClsVarHnd, 0, shiftByValue);
-                } 
-                else if (isLclVarAddr)
-                {
-                    getEmitter()->emitIns_S(ins, attr, operand->gtLclVarCommon.gtLclNum, 0);
-                }
-                else if (isCnsIntOrIAndFitsWithinAddrBase)
-                {
-                    getEmitter()->emitIns_I_AI(ins, attr, shiftByValue, disp);
-                }
-                else
-                {
-                    getEmitter()->emitIns_I_AR(ins, attr, shiftByValue, operandReg, offset);
-                }
-            }
-
+            getEmitter()->emitInsRMW(ins, attr, storeInd, shiftBy);
         }
     }
     else
     {
-        // We must have the number of bits to shift
-        // stored in ECX, since we constrained this node to
-        // sit in ECX, in case this didn't happen, LSRA expects
-        // the code generator to move it since it's a single
+        // We must have the number of bits to shift stored in ECX, since we constrained this node to
+        // sit in ECX. In case this didn't happen, LSRA expects the code generator to move it since it's a single
         // register destination requirement.
         regNumber shiftReg = shiftBy->gtRegNum;
         if (shiftReg != REG_RCX)
         {
             // Issue the mov to RCX:
             inst_RV_RV(INS_mov, REG_RCX, shiftReg, shiftBy->TypeGet());
-            shiftReg = REG_RCX;
         }
 
-        // The operand to be shifted must not be in ECX
-        noway_assert(operandReg != REG_RCX);
-
-        if (isRMW)
-        {
-            if (isClsVarAddr)
-            {
-                getEmitter()->emitIns_C_R(ins, attr, operand->gtClsVar.gtClsVarHnd, shiftReg, 0);
-            }
-            else if (isLclVarAddr)
-            {
-                getEmitter()->emitIns_S_R(ins, attr, shiftReg, operand->gtLclVarCommon.gtLclNum, 0);
-            }
-            else if (isCnsIntOrIAndFitsWithinAddrBase)
-            {
-                getEmitter()->emitIns_AI_R(ins, attr, shiftReg, disp);
-            }
-            else
-            {
-                getEmitter()->emitIns_AR_R(ins, attr, indexReg, operandReg, (int) offset);
-            }
-        }
-        else
-        {
-            if (parent->gtRegNum != operandReg)
-            {
-                inst_RV_RV(INS_mov, parent->gtRegNum, operandReg, targetType);
-            }
-            inst_RV_CL(ins, parent->gtRegNum, targetType);
-        }
+        // The shiftBy operand is implicit, so call the unary version of emitInsRMW.
+        getEmitter()->emitInsRMW(ins, attr, storeInd);
     }
-    genProduceReg(parent);
 }
 
 void CodeGen::genUnspillRegIfNeeded(GenTree *tree)
@@ -5199,16 +5089,16 @@ void CodeGen::genStoreInd(GenTreePtr node)
     GenTreeStoreInd* storeInd = node->AsStoreInd();
     GenTree* data = storeInd->Data();
     GenTree* addr = storeInd->Addr();
-    var_types targetType = node->TypeGet();
+    var_types targetType = storeInd->TypeGet();
 
     assert(!varTypeIsFloating(targetType) || (targetType == data->TypeGet()));
 
-    GCInfo::WriteBarrierForm writeBarrierForm = gcInfo.gcIsWriteBarrierCandidate(node, data);
+    GCInfo::WriteBarrierForm writeBarrierForm = gcInfo.gcIsWriteBarrierCandidate(storeInd, data);
     if (writeBarrierForm != GCInfo::WBF_NoBarrier)
     {
         // data and addr must be in registers.
         // Consume both registers so that any copies of interfering registers are taken care of.
-        genConsumeOperands(node->AsOp());
+        genConsumeOperands(storeInd->AsOp());
 
         if (genEmitOptimizedGCWriteBarrier(writeBarrierForm, addr, data))
             return;
@@ -5229,11 +5119,11 @@ void CodeGen::genStoreInd(GenTreePtr node)
             inst_RV_RV(INS_mov, REG_ARG_1, data->gtRegNum, data->TypeGet());
         }
 
-        genGCWriteBarrier(node, writeBarrierForm);
+        genGCWriteBarrier(storeInd, writeBarrierForm);
     }
     else
     {
-        bool reverseOps = ((node->gtFlags & GTF_REVERSE_OPS) != 0);
+        bool reverseOps = ((storeInd->gtFlags & GTF_REVERSE_OPS) != 0);
         bool dataIsUnary = false;
         bool isRMWMemoryOp = storeInd->IsRMWMemoryOp();
         GenTree* rmwSrc = nullptr;
@@ -5259,7 +5149,7 @@ void CodeGen::genStoreInd(GenTreePtr node)
                 if (storeInd->IsRMWDstOp1())
                 {
                     rmwDst = data->gtGetOp1();
-                    rmwSrc = data->gtGetOp2();                            
+                    rmwSrc = data->gtGetOp2();
                 }
                 else
                 {
@@ -5279,7 +5169,7 @@ void CodeGen::genStoreInd(GenTreePtr node)
 
             assert(rmwSrc != nullptr);
             assert(rmwDst != nullptr);
-            assert(Lowering::IndirsAreEquivalent(rmwDst, node));
+            assert(Lowering::IndirsAreEquivalent(rmwDst, storeInd));
 
             genConsumeRegs(rmwSrc);
         }
@@ -5298,29 +5188,28 @@ void CodeGen::genStoreInd(GenTreePtr node)
             if (dataIsUnary)
             {
                 // generate code for unary RMW memory ops like neg/not
-                getEmitter()->emitInsRMW(genGetInsForOper(data->OperGet(), data->TypeGet()), emitTypeSize(node), node);
+                getEmitter()->emitInsRMW(genGetInsForOper(data->OperGet(), data->TypeGet()), emitTypeSize(storeInd), storeInd);
             }
             else
             {
-                if (data->OperGet() == GT_LSH ||
-                    data->OperGet() == GT_RSH ||
-                    data->OperGet() == GT_RSZ ||
-                    data->OperGet() == GT_ROL ||
-                    data->OperGet() == GT_ROR)
+                if (data->OperIsShiftOrRotate())
                 {
-                    // generate code for shift RMW memory ops
-                    genCodeForShift(addr, rmwSrc, data);
+                    // Generate code for shift RMW memory ops.
+                    // The data address needs to be op1 (it must be [addr] = [addr] <shift> <amount>, not [addr] = <amount> <shift> [addr]).
+                    assert(storeInd->IsRMWDstOp1());
+                    assert(rmwSrc == data->gtGetOp2());
+                    genCodeForShiftRMW(storeInd);
                 }
                 else
                 {
                     // generate code for remaining binary RMW memory ops like add/sub/and/or/xor
-                    getEmitter()->emitInsRMW(genGetInsForOper(data->OperGet(), data->TypeGet()), emitTypeSize(node), node, rmwSrc);
+                    getEmitter()->emitInsRMW(genGetInsForOper(data->OperGet(), data->TypeGet()), emitTypeSize(storeInd), storeInd, rmwSrc);
                 }
             }
         }
         else
         {
-            getEmitter()->emitInsMov(ins_Store(data->TypeGet()), emitTypeSize(node), node);
+            getEmitter()->emitInsMov(ins_Store(data->TypeGet()), emitTypeSize(storeInd), storeInd);
         }
     }
 }
diff --git a/src/jit/emit.h b/src/jit/emit.h
index 39241278a5..95dac33536 100644
--- a/src/jit/emit.h
+++ b/src/jit/emit.h
@@ -1511,7 +1511,9 @@ private:
     regNumber            emitInsBinary  (instruction ins, emitAttr attr, GenTree* dst, GenTree* src);
     regNumber            emitInsTernary (instruction ins, emitAttr attr, GenTree* dst, GenTree* src1, GenTree* src2);
     void                 emitInsMov(instruction ins, emitAttr attr, GenTree *node);
-    void                 emitHandleMemOp(GenTree *mem, instrDesc *id, bool isSrc);
+    insFormat            emitMapFmtForIns(insFormat fmt, instruction ins);
+    insFormat            emitMapFmtAtoM(insFormat fmt);
+    void                 emitHandleMemOp(GenTreeIndir* indir, instrDesc* id, insFormat fmt, instruction ins);
     void                 spillIntArgRegsToShadowSlots();
 #endif // !LEGACY_BACKEND
 
diff --git a/src/jit/emitxarch.cpp b/src/jit/emitxarch.cpp
index 2b0a3141ec..3991b8aa01 100644
--- a/src/jit/emitxarch.cpp
+++ b/src/jit/emitxarch.cpp
@@ -1876,13 +1876,14 @@ UNATIVE_OFFSET      emitter::emitInsSizeAM(instrDesc* id, size_t code)
     // ideally these should really be the only idInsFmts that we see here
     //  but we have some outliers to deal with:
     //     emitIns_R_L adds IF_RWR_LABEL and calls emitInsSizeAM
-    //     emitInsRMW adds IF_MRW_CNS, IF_MRW_RRD and calls emitInsSizeAM
+    //     emitInsRMW adds IF_MRW_CNS, IF_MRW_RRD, IF_MRW_SHF, and calls emitInsSizeAM
 
     switch (id->idInsFmt())
     {
     case IF_RWR_LABEL:
     case IF_MRW_CNS:
     case IF_MRW_RRD:
+    case IF_MRW_SHF:
         reg = REG_NA;
         rgx = REG_NA;
         break;
@@ -2448,10 +2449,108 @@ void emitter::emitIns(instruction ins, emitAttr attr)
 }
 
 
-// fill in all the fields
-void emitter::emitHandleMemOp(GenTree* mem, instrDesc* id, bool isSrc)
+//------------------------------------------------------------------------
+// emitMapFmtForIns: map the instruction format based on the instruction.
+// Shift-by-a-constant instructions have a special format.
+//
+// Arguments:
+//    fmt - the instruction format to map
+//    ins - the instruction
+//
+// Returns:
+//    The mapped instruction format.
+//
+emitter::insFormat emitter::emitMapFmtForIns(insFormat fmt, instruction ins)
+{
+    switch (ins)
+    {
+    case INS_rol_N:
+    case INS_ror_N:
+    case INS_rcl_N:
+    case INS_rcr_N:
+    case INS_shl_N:
+    case INS_shr_N:
+    case INS_sar_N:
+        {
+            switch (fmt)
+            {
+            case IF_RRW_CNS: return IF_RRW_SHF;
+            case IF_MRW_CNS: return IF_MRW_SHF;
+            case IF_SRW_CNS: return IF_SRW_SHF;
+            case IF_ARW_CNS: return IF_ARW_SHF;
+            default: unreached();
+            }
+        }
+
+    default:
+        return fmt;
+    }
+}
+
+
+//------------------------------------------------------------------------
+// emitMapFmtAtoM: map the address mode formats ARD, ARW, and AWR to their direct address equivalents.
+//
+// Arguments:
+//    fmt - the instruction format to map
+//
+// Returns:
+//    The mapped instruction format.
+// 
+emitter::insFormat emitter::emitMapFmtAtoM(insFormat fmt)
 {
-    GenTreeIndir* indir = mem->AsIndir();
+    switch (fmt)
+    {
+    case IF_ARD:        return IF_MRD;
+    case IF_AWR:        return IF_MWR;
+    case IF_ARW:        return IF_MRW;
+
+    case IF_RRD_ARD:    return IF_RRD_MRD;
+    case IF_RWR_ARD:    return IF_RWR_MRD;
+    case IF_RRW_ARD:    return IF_RRW_MRD;
+
+    case IF_ARD_RRD:    return IF_MRD_RRD;
+    case IF_AWR_RRD:    return IF_MWR_RRD;
+    case IF_ARW_RRD:    return IF_MRW_RRD;
+
+    case IF_ARD_CNS:    return IF_MRD_CNS;
+    case IF_AWR_CNS:    return IF_MWR_CNS;
+    case IF_ARW_CNS:    return IF_MRW_CNS;
+
+    case IF_ARW_SHF:    return IF_MRW_SHF;
+
+    default: unreached();
+    }
+}
+
+
+//------------------------------------------------------------------------
+// emitHandleMemOp: For a memory operand, fill in the relevant fields of the instrDesc.
+//
+// Arguments:
+//    indir - the memory operand.
+//    id - the instrDesc to fill in.
+//    fmt - the instruction format to use. This must be one of the ARD, AWR, or ARW formats. If necessary (such as for
+//          GT_CLS_VAR_ADDR), this function will map it to the correct format.
+//    ins - the instruction we are generating. This might affect the instruction format we choose.
+//
+// Assumptions:
+//    The correctly sized instrDesc must already be created, e.g., via emitNewInstrAmd() or emitNewInstrAmdCns();
+//
+// Post-conditions:
+//    For base address of int constant:
+//        -- the caller must have added the int constant base to the instrDesc when creating it via emitNewInstrAmdCns().
+//    For simple address modes (base + scale * index + offset):
+//        -- the base register, index register, and scale factor are set.
+//        -- the caller must have added the addressing mode offset int constant to the instrDesc when creating it via emitNewInstrAmdCns().
+//
+//    The instruction format is set.
+//
+//    idSetIsDspReloc() is called if necessary.
+// 
+void emitter::emitHandleMemOp(GenTreeIndir* indir, instrDesc* id, insFormat fmt, instruction ins)
+{
+    assert(fmt != IF_NONE);
 
     GenTree* memBase = indir->Base();
 
@@ -2474,14 +2573,7 @@ void emitter::emitHandleMemOp(GenTree* mem, instrDesc* id, bool isSrc)
         }
 
         id->idAddr()->iiaFieldHnd = fldHnd;
-        if (isSrc)
-        {
-            id->idInsFmt(IF_RRD_MRD);
-        }
-        else
-        {
-            id->idInsFmt(IF_MRD_RRD);
-        }
+        id->idInsFmt(emitMapFmtForIns(emitMapFmtAtoM(fmt), ins));
     }
     else if ((memBase != nullptr) && memBase->IsCnsIntOrI() && memBase->isContained())
     {
@@ -2496,18 +2588,13 @@ void emitter::emitHandleMemOp(GenTree* mem, instrDesc* id, bool isSrc)
             id->idSetIsDspReloc();
         }
 
-        if (isSrc)
-        {
-            id->idInsFmt(IF_RRD_ARD);
-        }
-        else
-        {
-            id->idInsFmt(IF_ARD_RRD);
-        }
-
         id->idAddr()->iiaAddrMode.amBaseReg = REG_NA;
         id->idAddr()->iiaAddrMode.amIndxReg = REG_NA;
+        id->idAddr()->iiaAddrMode.amScale   = emitter::OPSZ1; // for completeness
 
+        id->idInsFmt(emitMapFmtForIns(fmt, ins));
+
+        // Absolute address must have already been set in the instrDesc constructor.
         assert(emitGetInsAmdAny(id) == memBase->AsIntConCommon()->IconValue());
     }
     else
@@ -2531,7 +2618,9 @@ void emitter::emitHandleMemOp(GenTree* mem, instrDesc* id, bool isSrc)
         }
         id->idAddr()->iiaAddrMode.amScale   = emitEncodeScale(indir->Scale());
 
-        // already set disp in ctor
+        id->idInsFmt(emitMapFmtForIns(fmt, ins));
+
+        // disp must have already been set in the instrDesc constructor.
         assert(emitGetInsAmdAny(id) == ssize_t(indir->Offset())); // make sure "disp" is stored properly
     }
 }
@@ -2585,42 +2674,30 @@ void emitter::emitInsMov(instruction ins, emitAttr attr, GenTree* node)
     case GT_IND:
         {
             GenTreeIndir* mem = node->AsIndir();
+            GenTreePtr addr = mem->Addr();
             
-            if (mem->Addr()->OperGet() == GT_CLS_VAR_ADDR)
+            if (addr->OperGet() == GT_CLS_VAR_ADDR)
             {
-                emitIns_R_C(ins, attr, node->gtRegNum, mem->Addr()->gtClsVar.gtClsVarHnd, 0);
+                emitIns_R_C(ins, attr, mem->gtRegNum, addr->gtClsVar.gtClsVarHnd, 0);
                 return;
             }
-            else if (mem->Addr()->OperGet() == GT_LCL_VAR_ADDR)
+            else if (addr->OperGet() == GT_LCL_VAR_ADDR)
             {
-                GenTreeLclVarCommon* varNode = mem->Addr()->AsLclVarCommon();
-                emitIns_R_S(ins, attr, node->gtRegNum, varNode->GetLclNum(), 0);
+                GenTreeLclVarCommon* varNode = addr->AsLclVarCommon();
+                emitIns_R_S(ins, attr, mem->gtRegNum, varNode->GetLclNum(), 0);
                 codeGen->genUpdateLife(varNode);
                 return;
             }
             else
             {
-                GenTreePtr addr = mem->Addr();
-
                 assert (addr->OperIsAddrMode() ||
                         (addr->IsCnsIntOrI() && addr->isContained()) ||
                         !addr->isContained());
                 size_t offset = mem->Offset();
                 id = emitNewInstrAmd(attr, offset);
                 id->idIns(ins);
-                id->idReg1(node->gtRegNum);
-                id->idInsFmt(IF_RWR_ARD);
-                emitHandleMemOp(node, id, true); // may overwrite format
-
-                if (addr->IsCnsIntOrI() && addr->isContained())
-                {
-                    // Absolute addresses marked as contained should fit within the base of addr mode.
-                    assert(addr->AsIntConCommon()->FitsInAddrBase(emitComp));
-
-                    // Case of "ins re, [disp]" and should use IF_RWR_ARD as format
-                    id->idInsFmt(IF_RWR_ARD);
-                }
-
+                id->idReg1(mem->gtRegNum);
+                emitHandleMemOp(mem, id, IF_RWR_ARD, ins);
                 sz = emitInsSizeAM(id, insCodeRM(ins));
                 id->idCodeSize(sz);
             }
@@ -2629,68 +2706,55 @@ void emitter::emitInsMov(instruction ins, emitAttr attr, GenTree* node)
 
     case GT_STOREIND:
         {
-            GenTreeIndir* mem = node->AsIndir();
-            GenTree* memBase = mem->Base();
+            GenTreeStoreInd* mem = node->AsStoreInd();
+            GenTreePtr addr = mem->Addr();
             size_t offset = mem->Offset();
-            GenTree* data = node->gtOp.gtOp2;
+            GenTree* data = mem->Data();
 
-            if (mem->Addr()->OperGet() == GT_CLS_VAR_ADDR)
+            if (addr->OperGet() == GT_CLS_VAR_ADDR)
             {
-                if (data->isContained())
+                if (data->isContainedIntOrIImmed())
                 {
-                    emitIns_C_I(ins, attr, mem->Addr()->gtClsVar.gtClsVarHnd, 0, (int) data->AsIntConCommon()->IconValue());
+                    emitIns_C_I(ins, attr, addr->gtClsVar.gtClsVarHnd, 0, (int) data->AsIntConCommon()->IconValue());
                 }
                 else
                 {
-                    emitIns_C_R(ins, attr, mem->Addr()->gtClsVar.gtClsVarHnd, data->gtRegNum, 0);
+                    assert(!data->isContained());
+                    emitIns_C_R(ins, attr, addr->gtClsVar.gtClsVarHnd, data->gtRegNum, 0);
                 }
                 return;
             }
-
-            if (mem->Addr()->OperGet() == GT_LCL_VAR_ADDR)
+            else if (addr->OperGet() == GT_LCL_VAR_ADDR)
             {
-                GenTreeLclVarCommon* varNode = memBase->AsLclVarCommon();
+                GenTreeLclVarCommon* varNode = addr->AsLclVarCommon();
                 if (data->isContainedIntOrIImmed())
                 {
                     emitIns_S_I(ins, attr, varNode->GetLclNum(), 0, (int) data->AsIntConCommon()->IconValue());
-                    codeGen->genUpdateLife(varNode);
                 }
                 else
                 {
                     assert(!data->isContained());
                     emitIns_S_R(ins, attr, data->gtRegNum, varNode->GetLclNum(), 0);
-                    codeGen->genUpdateLife(varNode);
                 }
+                codeGen->genUpdateLife(varNode);
                 return;
             }
-
-            if (data->isContainedIntOrIImmed())
+            else if (data->isContainedIntOrIImmed())
             {
                 int icon = (int) data->AsIntConCommon()->IconValue();
                 id = emitNewInstrAmdCns(attr, offset, icon);
                 id->idIns(ins);
-                id->idInsFmt(IF_AWR_CNS);
-                emitHandleMemOp(node, id, false); // may overwrite format
-
-                if ((memBase != nullptr) && memBase->IsCnsIntOrI() && memBase->isContained())
-                {                    
-                    // Absolute addresses marked as contained should fit within the base of addr mode.
-                    assert(memBase->AsIntConCommon()->FitsInAddrBase(emitComp));
-
-                    // Case of "ins [disp], immed " and should use IF_AWR_CNS as format
-                    id->idInsFmt(IF_AWR_CNS);
-                }
-
+                emitHandleMemOp(mem, id, IF_AWR_CNS, ins);
                 sz = emitInsSizeAM(id, insCodeMI(ins), icon);
                 id->idCodeSize(sz);
             }
             else
             {
+                assert(!data->isContained());
                 id = emitNewInstrAmd(attr, offset);
                 id->idIns(ins);
-                id->idInsFmt(IF_AWR_RRD);
-                emitHandleMemOp(node, id, false); // may overwrite format
-                id->idReg1(node->gtOp.gtOp2->gtRegNum);
+                emitHandleMemOp(mem, id, IF_AWR_RRD, ins);
+                id->idReg1(data->gtRegNum);
                 sz = emitInsSizeAM(id, insCodeMR(ins));
                 id->idCodeSize(sz);
             }
@@ -2700,22 +2764,20 @@ void emitter::emitInsMov(instruction ins, emitAttr attr, GenTree* node)
     case GT_STORE_LCL_VAR:
         {
             GenTreeLclVarCommon* varNode = node->AsLclVarCommon();
-
-            GenTree* data = node->gtOp.gtOp1->gtEffectiveVal();
+            GenTree* data = varNode->gtOp.gtOp1->gtEffectiveVal();
             codeGen->inst_set_SV_var(varNode);
             assert(varNode->gtRegNum == REG_NA); // stack store
 
             if (data->isContainedIntOrIImmed())
             {
                 emitIns_S_I(ins, attr, varNode->GetLclNum(), 0, (int) data->AsIntConCommon()->IconValue());
-                codeGen->genUpdateLife(varNode);
             }
             else
             {
                 assert(!data->isContained());
                 emitIns_S_R(ins, attr, data->gtRegNum, varNode->GetLclNum(), 0);
-                codeGen->genUpdateLife(varNode);
             }
+            codeGen->genUpdateLife(varNode);
         }
         return;
 
@@ -2990,8 +3052,6 @@ regNumber emitter::emitInsBinary(instruction ins, emitAttr attr, GenTree* dst, G
 
     id->idIns(ins);    // Set the instruction.
 
-    emitHandleMemOp(mem, id, mem == src);
-
     // Determine the instruction format
     //
     insFormat fmt = IF_NONE;
@@ -3019,8 +3079,7 @@ regNumber emitter::emitInsBinary(instruction ins, emitAttr attr, GenTree* dst, G
         }
     }
     assert(fmt != IF_NONE);
-
-    id->idInsFmt(fmt);
+    emitHandleMemOp(mem, id, fmt, ins);
 
     // Determine the instruction size
     //
@@ -3063,88 +3122,65 @@ regNumber emitter::emitInsBinary(instruction ins, emitAttr attr, GenTree* dst, G
     return result;
 }
 
-/** Emit logic for Read-Modify-Write Instructions
- *  Responsible for encoding a single instruction that will perform an operation of the form
- *  *addr op= operand for example
- *  ADD [RAX], RCX
- *
- *  Preconditions:  Lowering has taken care of recognizing the StoreInd pattern of
- *                  StoreInd( AddressTree, BinOp( Ind ( AddressTree ), Operand )
- *                  The address to store is already sitting in a register.
- *
- *                  This is a no-produce operation, meaning that no register output will
- *                  be produced for future use in the code stream.
- *
- * The caller is responsible for calling genConsumeReg() on all source registers, and
- * genProduceReg on the target register, if any.
- */
-void emitter::emitInsRMW(instruction ins, emitAttr attr, GenTree* dstAddr, GenTree* src)
+//------------------------------------------------------------------------
+// emitInsRMW: Emit logic for Read-Modify-Write binary instructions.
+//
+// Responsible for emitting a single instruction that will perform an operation of the form:
+//      *addr = *addr <BinOp> src
+// For example:
+//      ADD [RAX], RCX
+//
+// Arguments:
+//    ins - instruction to generate
+//    attr - emitter attribute for instruction
+//    storeInd - indir for RMW addressing mode
+//    src - source operand of instruction
+//
+// Assumptions:
+//    Lowering has taken care of recognizing the StoreInd pattern of:
+//          StoreInd( AddressTree, BinOp( Ind ( AddressTree ), Operand ) )
+//    The address to store is already sitting in a register.
+//
+// Notes:
+//    This is a no-produce operation, meaning that no register output will
+//    be produced for future use in the code stream.
+//
+void emitter::emitInsRMW(instruction ins, emitAttr attr, GenTreeStoreInd* storeInd, GenTree* src)
 {
-    assert(dstAddr->isIndir());
-    GenTreeIndir* indir = dstAddr->AsIndir();
-    GenTreePtr mem = indir->Addr();
-    assert(mem->gtSkipReloadOrCopy()->OperGet() == GT_LCL_VAR ||
-           mem->gtSkipReloadOrCopy()->OperGet() == GT_LCL_VAR_ADDR ||
-           mem->gtSkipReloadOrCopy()->OperGet() == GT_LEA ||
-           mem->gtSkipReloadOrCopy()->OperGet() == GT_CLS_VAR_ADDR ||
-           mem->gtSkipReloadOrCopy()->OperGet() == GT_CNS_INT);
+    GenTreePtr addr = storeInd->Addr();
+    addr = addr->gtSkipReloadOrCopy();
+    assert(addr->OperGet() == GT_LCL_VAR ||
+           addr->OperGet() == GT_LCL_VAR_ADDR ||
+           addr->OperGet() == GT_LEA ||
+           addr->OperGet() == GT_CLS_VAR_ADDR ||
+           addr->OperGet() == GT_CNS_INT);
 
     instrDesc* id = nullptr;
-    UNATIVE_OFFSET  sz;
+    UNATIVE_OFFSET sz;
 
     size_t offset = 0;
-    if (mem->gtSkipReloadOrCopy()->OperGet() != GT_CLS_VAR_ADDR)
+    if (addr->OperGet() != GT_CLS_VAR_ADDR)
     {
-        offset = indir->Offset();
+        offset = storeInd->Offset();
     }
 
-    // find immed (if any) - it cannot be a dst
-    GenTreeIntConCommon* intConst = nullptr;
     if (src->isContainedIntOrIImmed())
     {
-        intConst = src->AsIntConCommon();
-    }
-
-    if (intConst != nullptr)
-    {
+        GenTreeIntConCommon* intConst = src->AsIntConCommon();
         id = emitNewInstrAmdCns(attr, offset, (int) intConst->IconValue());
+        emitHandleMemOp(storeInd, id, IF_ARW_CNS, ins);
+        id->idIns(ins);
+        sz = emitInsSizeAM(id, insCodeMI(ins), (int) intConst->IconValue());
     }
     else
     {
+        assert(!src->isContained()); // there must be one non-contained src
+
         // ind, reg
         id = emitNewInstrAmd(attr, offset);
-        // there must be one non-contained src
-        assert(!src->isContained());
+        emitHandleMemOp(storeInd, id, IF_ARW_RRD, ins);
         id->idReg1(src->gtRegNum);
-        // fmt is set below
-    }
-
-    id->idIns(ins);
-
-    emitHandleMemOp(dstAddr, id, false);
-
-    if (src->isContainedIntOrIImmed())
-    {
-        if (mem->gtSkipReloadOrCopy()->OperGet() == GT_CLS_VAR_ADDR)
-        {
-            id->idInsFmt(IF_MRW_CNS);
-        }
-        else
-        {
-            id->idInsFmt(IF_ARW_CNS);
-        }
-        sz = emitInsSizeAM(id, insCodeMI(ins), (int) intConst->IconValue());
-    }
-    else
-    {
-        if (mem->gtSkipReloadOrCopy()->OperGet() == GT_CLS_VAR_ADDR)
-        {
-            id->idInsFmt(IF_MRW_RRD);
-        }
-        else
-        {
-            id->idInsFmt(IF_ARW_RRD);
-        }
+        id->idIns(ins);
         sz = emitInsSizeAM(id, insCodeMR(ins));
     }
 
@@ -3155,51 +3191,47 @@ void emitter::emitInsRMW(instruction ins, emitAttr attr, GenTree* dstAddr, GenTr
 }
 
 
-/** Emit logic for Read-Modify-Write Instructions on unary operators
- *  Responsible for encoding a single instruction that will perform an operation of the form
- *  *addr = UnaryOp *addr operand for example
- *  NOT [RAX]
- *
- *  Preconditions:  Lowering has taken care of recognizing the StoreInd pattern of
- *                  StoreInd( AddressTree, UnOp( Ind ( AddressTree ) )
- *                  The address to store is already sitting in a register.
- *
- *                  This is a no-produce operation, meaning that no register output will
- *                  be produced for future use in the code stream.
- */
-void emitter::emitInsRMW(instruction ins, emitAttr attr, GenTree* dstAddr)
-{
-    assert(ins == INS_not || ins == INS_neg);
-    assert(dstAddr->isIndir());
-    GenTreeIndir* indir = dstAddr->AsIndir();
-    GenTreePtr mem = indir->Addr();
-    assert(mem->gtSkipReloadOrCopy()->OperGet() == GT_LCL_VAR ||
-           mem->gtSkipReloadOrCopy()->OperGet() == GT_LCL_VAR_ADDR ||
-           mem->gtSkipReloadOrCopy()->OperGet() == GT_CLS_VAR_ADDR ||
-           mem->gtSkipReloadOrCopy()->OperGet() == GT_LEA ||
-           mem->gtSkipReloadOrCopy()->OperGet() == GT_CNS_INT);
+//------------------------------------------------------------------------
+// emitInsRMW: Emit logic for Read-Modify-Write unary instructions.
+//
+// Responsible for emitting a single instruction that will perform an operation of the form:
+//      *addr = UnaryOp *addr
+// For example:
+//      NOT [RAX]
+//
+// Arguments:
+//    ins - instruction to generate
+//    attr - emitter attribute for instruction
+//    storeInd - indir for RMW addressing mode
+//
+// Assumptions:
+//    Lowering has taken care of recognizing the StoreInd pattern of:
+//          StoreInd( AddressTree, UnaryOp( Ind ( AddressTree ) ) )
+//    The address to store is already sitting in a register.
+//
+// Notes:
+//    This is a no-produce operation, meaning that no register output will
+//    be produced for future use in the code stream.
+//
+void emitter::emitInsRMW(instruction ins, emitAttr attr, GenTreeStoreInd* storeInd)
+{
+    GenTreePtr addr = storeInd->Addr();
+    addr = addr->gtSkipReloadOrCopy();
+    assert(addr->OperGet() == GT_LCL_VAR ||
+           addr->OperGet() == GT_LCL_VAR_ADDR ||
+           addr->OperGet() == GT_CLS_VAR_ADDR ||
+           addr->OperGet() == GT_LEA ||
+           addr->OperGet() == GT_CNS_INT);
 
     size_t offset = 0;
-    if (mem->gtSkipReloadOrCopy()->OperGet() != GT_CLS_VAR_ADDR)
+    if (addr->OperGet() != GT_CLS_VAR_ADDR)
     {
-        offset = indir->Offset();
+        offset = storeInd->Offset();
     }
 
     instrDesc* id = emitNewInstrAmd(attr, offset);
-
-    emitHandleMemOp(dstAddr, id, true);
-
+    emitHandleMemOp(storeInd, id, IF_ARW, ins);
     id->idIns(ins);
-
-    if(mem->OperGet() == GT_CLS_VAR_ADDR)
-    {
-        id->idInsFmt(IF_MRW);
-    }
-    else
-    {
-        id->idInsFmt(IF_ARW);
-    }
-
     UNATIVE_OFFSET sz = emitInsSizeAM(id, insCodeMR(ins));
     id->idCodeSize(sz);
 
@@ -4205,9 +4237,7 @@ void                emitter::emitIns_I_AR  (instruction ins,
     case INS_shl_N:
     case INS_shr_N:
     case INS_sar_N:
-#ifdef _TARGET_X86_
         assert(val != 1);
-#endif
         fmt  = IF_ARW_SHF;
         val &= 0x7F;
         break;
diff --git a/src/jit/emitxarch.h b/src/jit/emitxarch.h
index 594d0d0996..29ba156305 100644
--- a/src/jit/emitxarch.h
+++ b/src/jit/emitxarch.h
@@ -233,9 +233,9 @@ public:
 
     void            emitIns        (instruction ins, emitAttr attr);
 
-    void            emitInsRMW     (instruction inst, emitAttr attr, GenTreePtr dst, GenTreePtr src);
+    void            emitInsRMW     (instruction inst, emitAttr attr, GenTreeStoreInd* storeInd, GenTreePtr src);
 
-    void            emitInsRMW     (instruction inst, emitAttr attr, GenTreePtr dst);
+    void            emitInsRMW     (instruction inst, emitAttr attr, GenTreeStoreInd* storeInd);
 
     void            emitIns_Nop    (unsigned    size);
 
diff --git a/src/jit/gentree.h b/src/jit/gentree.h
index f306d81bda..550a5799fb 100644
--- a/src/jit/gentree.h
+++ b/src/jit/gentree.h
@@ -1097,6 +1097,43 @@ public:
         return  (OperKind(gtOper) & GTK_LOGOP  ) != 0;
     }
 
+    static
+    bool            OperIsShift(genTreeOps gtOper)
+    {
+        return (gtOper == GT_LSH) ||
+               (gtOper == GT_RSH) ||
+               (gtOper == GT_RSZ);
+    }
+
+    bool            OperIsShift() const
+    {
+        return OperIsShift(OperGet());
+    }
+
+    static
+    bool            OperIsRotate(genTreeOps gtOper)
+    {
+        return (gtOper == GT_ROL) ||
+               (gtOper == GT_ROR);
+    }
+
+    bool            OperIsRotate() const
+    {
+        return OperIsRotate(OperGet());
+    }
+
+    static
+    bool            OperIsShiftOrRotate(genTreeOps gtOper)
+    {
+        return OperIsShift(gtOper) ||
+               OperIsRotate(gtOper);
+    }
+
+    bool            OperIsShiftOrRotate() const
+    {
+        return OperIsShiftOrRotate(OperGet());
+    }
+
     int             OperIsArithmetic() const
     {
         genTreeOps op = OperGet();
@@ -1113,12 +1150,7 @@ public:
                 || op==GT_XOR
                 || op==GT_AND
 
-                || op==GT_LSH
-                || op==GT_RSH
-                || op==GT_RSZ
-
-                || op==GT_ROL
-                || op==GT_ROR;
+                || OperIsShiftOrRotate(op);
     }
 
     static
diff --git a/src/jit/instr.cpp b/src/jit/instr.cpp
index 85b8a44769..3c6339ca9f 100644
--- a/src/jit/instr.cpp
+++ b/src/jit/instr.cpp
@@ -2640,46 +2640,15 @@ void        CodeGen::inst_RV_SH(instruction  ins,
     assert(val < 256);
 #endif
 
-    assert(ins == INS_rcl  ||
-           ins == INS_rcr  ||
-           ins == INS_rol  ||
-           ins == INS_ror  ||
-           ins == INS_shl  ||
-           ins == INS_shr  ||
-           ins == INS_sar);
-
-    /* Which format should we use? */
+    ins = genMapShiftInsToShiftByConstantIns(ins, val);
 
     if  (val == 1)
     {
-        /* Use the shift-by-one format */
-
-        assert(INS_rcl + 1 == INS_rcl_1);
-        assert(INS_rcr + 1 == INS_rcr_1);
-        assert(INS_rol + 1 == INS_rol_1);
-        assert(INS_ror + 1 == INS_ror_1);
-        assert(INS_shl + 1 == INS_shl_1);
-        assert(INS_shr + 1 == INS_shr_1);
-        assert(INS_sar + 1 == INS_sar_1);
-
-        getEmitter()->emitIns_R((instruction)(ins+1), size, reg);
+        getEmitter()->emitIns_R(ins, size, reg);
     }
     else
     {
-        /* Use the shift-by-NNN format */
-
-        assert(INS_rcl + 2 == INS_rcl_N);
-        assert(INS_rcr + 2 == INS_rcr_N);
-        assert(INS_rol + 2 == INS_rol_N);
-        assert(INS_ror + 2 == INS_ror_N);
-        assert(INS_shl + 2 == INS_shl_N);
-        assert(INS_shr + 2 == INS_shr_N);
-        assert(INS_sar + 2 == INS_sar_N);
-
-        getEmitter()->emitIns_R_I((instruction)(ins+2),
-                                 size,
-                                 reg,
-                                 val);
+        getEmitter()->emitIns_R_I(ins, size, reg, val);
     }
 
 #else
@@ -2698,43 +2667,20 @@ void                CodeGen::inst_TT_SH(instruction   ins,
                                         unsigned      offs)
 {
 #ifdef _TARGET_XARCH_
-    /* Which format should we use? */
-
-    switch (val)
+    if (val == 0)
     {
-    case 1:
-
-        /* Use the shift-by-one format */
-
-        assert(INS_rcl + 1 == INS_rcl_1);
-        assert(INS_rcr + 1 == INS_rcr_1);
-        assert(INS_shl + 1 == INS_shl_1);
-        assert(INS_shr + 1 == INS_shr_1);
-        assert(INS_sar + 1 == INS_sar_1);
-
-        inst_TT((instruction)(ins+1), tree, offs, 0, emitTypeSize(tree->TypeGet()));
-
-        break;
-
-    case 0:
-
         // Shift by 0 - why are you wasting our precious time????
-
         return;
+    }
 
-    default:
-
-        /* Use the shift-by-NNN format */
-
-        assert(INS_rcl + 2 == INS_rcl_N);
-        assert(INS_rcr + 2 == INS_rcr_N);
-        assert(INS_shl + 2 == INS_shl_N);
-        assert(INS_shr + 2 == INS_shr_N);
-        assert(INS_sar + 2 == INS_sar_N);
-
-        inst_TT((instruction)(ins+2), tree, offs, val, emitTypeSize(tree->TypeGet()));
-
-        break;
+    ins = genMapShiftInsToShiftByConstantIns(ins, val);
+    if (val == 1)
+    {
+        inst_TT(ins, tree, offs, 0, emitTypeSize(tree->TypeGet()));
+    }
+    else
+    {
+        inst_TT(ins, tree, offs, val, emitTypeSize(tree->TypeGet()));
     }
 #endif // _TARGET_XARCH_
 
diff --git a/src/jit/lclvars.cpp b/src/jit/lclvars.cpp
index d5f760deee..207e02838d 100644
--- a/src/jit/lclvars.cpp
+++ b/src/jit/lclvars.cpp
@@ -2967,11 +2967,7 @@ void                Compiler::lvaMarkLclRefs(GenTreePtr tree)
 #ifdef _TARGET_XARCH_
     /* Special case: integer shift node by a variable amount */
 
-    if  (tree->gtOper == GT_LSH ||
-         tree->gtOper == GT_RSH ||
-         tree->gtOper == GT_RSZ ||
-         tree->gtOper == GT_ROL ||
-         tree->gtOper == GT_ROR)
+    if  (tree->OperIsShiftOrRotate())
     {
         if  (tree->gtType == TYP_INT)
         {
diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp
index 4103b843b0..59e681e597 100644
--- a/src/jit/lowerxarch.cpp
+++ b/src/jit/lowerxarch.cpp
@@ -3096,7 +3096,7 @@ bool Lowering::IsBinOpInRMWStoreInd(GenTreePtr tree)
  // to be correct and functional.
  //     IndirsAreEquivalent()
  //     NodesAreEquivalentLeaves()
- //     Codegen of GT_STOREIND and genCodeForShift()
+ //     Codegen of GT_STOREIND and genCodeForShiftRMW()
  //     emitInsRMW()
  //     
  //  TODO-CQ: Enable support for more complex indirections (if needed) or use the value numbering
@@ -3194,21 +3194,13 @@ bool Lowering::IsRMWMemOpRootedAtStoreInd(GenTreePtr tree, GenTreePtr *outIndirC
             oper != GT_AND &&
             oper != GT_OR  &&
             oper != GT_XOR &&
-            oper != GT_LSH &&
-            oper != GT_RSH &&
-            oper != GT_RSZ &&
-            oper != GT_ROL &&
-            oper != GT_ROR)
+            !GenTree::OperIsShiftOrRotate(oper))
         {
             storeInd->SetRMWStatus(STOREIND_RMW_UNSUPPORTED_OPER);
             return false;
         }
 
-        if ((oper == GT_LSH ||
-            oper == GT_RSH ||
-            oper == GT_RSZ ||
-            oper == GT_ROL ||
-            oper == GT_ROR) &&
+        if (GenTree::OperIsShiftOrRotate(oper) &&
             varTypeIsSmall(storeInd))
         {
             // In ldind, Integer values smaller than 4 bytes, a boolean, or a character converted to 4 bytes
diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp
index 567c569223..1366ebd373 100644
--- a/src/jit/morph.cpp
+++ b/src/jit/morph.cpp
@@ -10105,7 +10105,7 @@ DONE_MORPHING_CHILDREN:
     // could mark the trees just before argument processing, but it would require a full
     // tree walk of the argument tree, so we just do it here, instead, even though we'll
     // mark non-argument trees (that will still get converted to calls, anyway).
-    if ((oper == GT_LSH || oper == GT_RSH || oper == GT_RSZ) &&
+    if (GenTree::OperIsShift(oper) &&
         (tree->TypeGet() == TYP_LONG) &&
         (op2->OperGet() != GT_CNS_INT))
     {
@@ -11819,8 +11819,7 @@ CM_ADD_OP:
                 /* for the shift nodes the type of op2 can differ from the tree type */
                 if ((typ == TYP_LONG) && (genActualType(op2->gtType) == TYP_INT))
                 {
-                    noway_assert((oper == GT_LSH) || (oper == GT_RSH) || (oper == GT_RSZ) ||
-                                 (oper == GT_ROL) || (oper == GT_ROR));
+                    noway_assert(GenTree::OperIsShiftOrRotate(oper));
 
                     GenTreePtr commaOp2 = op2->gtOp.gtOp2;
 
@@ -12992,7 +12991,7 @@ GenTreePtr Compiler::fgRecognizeAndMorphBitwiseRotation(GenTreePtr tree)
 
         if (rotateIndex != nullptr)
         {
-            noway_assert((rotateOp == GT_ROL) || (rotateOp == GT_ROR));
+            noway_assert(GenTree::OperIsRotate(rotateOp));
 
             unsigned inputTreeEffects = tree->gtFlags & GTF_ALL_EFFECT;