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.

1 files changed, 13 insertions, 67 deletions

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_