Sort out ARM load/store instruction size issues (#20126)
authormikedn <onemihaid@hotmail.com>
Thu, 30 May 2019 00:53:11 +0000 (03:53 +0300)
committerCarol Eidt <carol.eidt@microsoft.com>
Thu, 30 May 2019 00:53:11 +0000 (17:53 -0700)
commitc6824d52873d0f6382b4f733c50f510491d37b12
treed2c5a86433cb7fab51be6bc4bb9fccfc958cc373
parent0950e5200eaefab9ac75edd59d23ef69eaccf539
Sort out ARM load/store instruction size issues (#20126)

* Avoid using ins_Load/ins_Store with constant type

* Use ldr to load array lengths/lower bounds

genCodeForArrIndex and genCodeForArrOffset emit a ldrsw on ARM64 but that's not necessary. Array lengths are always positive. Lower bounds are signed but then the subsequent subtract is anyway EA_4BYTE so sign extension isn't actually needed. Just use ldr on both ARM32 and ARM64.

* Use ldr to load array length (part 2)

genCodeForIndexAddr's range check generation is messed up:
- It uses ldrsw to load array length on ARM64. Not needed, the length is positive.
- It uses sxtw to sign etxend the array length if the index is native int. But it was already extended by the load.
- It creates IND and LEA nodes out of thin air. Just call the appropiate emitter functions.
- It always generates a TYP_I_IMPL cmp, even when the index is TYP_INT. Well, that's just bogus.

* Stop the using the instruction size for immediate scaling on ARM32

The scaling of immediate operands is a property of the instruction and its encoding. It doesnt' make sense to throw the instruction size (emitAttr) into the mix, that's a codegen/emitter specific concept. On ARM32 it's also almost meaningless, at least in the case of integer types - all instructions are really EA_4BYTE, even ldrb, ldrh etc.

The ARM64 emitter already extracts the scaling factor from the instruction. It can't use the instruction size as on ARM64 the size is used to select between 32 bit and 64 bit instructions so it's never EA_1BYTE/EA_2BYTE.

* Stop using ldrsw for TYP_INT loads

ARM64's ins_Load returns INS_ldrsw for TYP_INT but there's nothing in the JIT type system that requires sign extending TYP_INT values on load. The fact that an indir node has TYP_INT doesn't even imply that the value to load is signed, it may be unsigned and indir nodes will never have type TYP_UINT nor have the GTF_UNSIGNED flag set.

XARCH uses a mov (rather than movsxd, the equivalent of ldrsw) so it zero extends. There's no reason for ARM64 to behave differently and doing so makes it more difficult to share codegen logic between XARCH and ARM64

Other ARM64 compilers also use ldr rather than ldrsw.

This requires patching up emitInsAdjustLoadStoreAttr so EA_4BYTE loads don't end up using EA_8BYTE, which ldrsw requires.

* Cleanup genCodeForIndir/genCodeForStoreInd

In particular, cleanup selection of acquire/release instructions. The existing code starts by selecting a "normal" instruction, only to throw it away and redo the type/size logic in the volatile case. And get it wrong in the process, it required that "targetType" be TYP_UINT or TYP_LONG to use ldar. But there are no TYP_UINT indirs.

Also rename "targetType" to "type", using "target" is misleading. The real target type would be genActualType(tree->TypeGet()).

* Remove ARM32/64 load/store instruction size inconsistencies

- Require EA_4BYTE for byte/halfword instructions on ARM32.
- Remove emitInsAdjustLoadStoreAttr on ARM64. Getting the correct instruction size simply requires using emitActualTypeSize, that will provide the correct size on both ARM32 and ARM64.
- Replace emitTypeSize with emitActualTypeSize as needed.

* Remove unnecessary insUnscaleImm parameter
src/jit/codegenarm.cpp
src/jit/codegenarm64.cpp
src/jit/codegenarmarch.cpp
src/jit/codegencommon.cpp
src/jit/codegenlinear.cpp
src/jit/emitarm.cpp
src/jit/emitarm.h
src/jit/emitarm64.cpp
src/jit/emitarm64.h
src/jit/instr.cpp