Fix #3561: assert on RyuJIT x86 when generating shl by 1
authorBruce Forstall <brucefo@microsoft.com>
Thu, 21 Apr 2016 22:37:06 +0000 (15:37 -0700)
committerBruce Forstall <brucefo@microsoft.com>
Sat, 23 Apr 2016 05:44:06 +0000 (22:44 -0700)
commit9a67c5824baa6887fa2a2cd26da59e26037cf883
tree2b2e76a4c407dadabf9ab5491cd699c631d11daf
parent25c0a14c4db495ab452186f5b93b891977d71294
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:
src/jit/assertionprop.cpp
src/jit/codegen.h
src/jit/codegenarm.cpp
src/jit/codegenarm64.cpp
src/jit/codegencommon.cpp
src/jit/codegenlegacy.cpp
src/jit/codegenlinear.h
src/jit/codegenxarch.cpp
src/jit/emit.h
src/jit/emitxarch.cpp
src/jit/emitxarch.h
src/jit/gentree.h
src/jit/instr.cpp
src/jit/lclvars.cpp
src/jit/lowerxarch.cpp
src/jit/morph.cpp