Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Commit

Permalink
Remove ARM32/64 load/store instruction size inconsistencies
Browse files Browse the repository at this point in the history 
- 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.
  • Loading branch information
@mikedn
mikedn committed Oct 20, 2018
1 parent 781dcd6 commit e3146c7
Show file tree
Hide file tree
Showing 7 changed files with 34 additions and 87 deletions.
2 changes: 1 addition & 1 deletion src/jit/codegenarm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1294,7 +1294,7 @@ void CodeGen::genCodeForStoreInd(GenTreeStoreInd* tree)
instGen_MemoryBarrier();
}

getEmitter()->emitInsLoadStoreOp(ins_Store(type), emitTypeSize(type), data->gtRegNum, tree);
getEmitter()->emitInsLoadStoreOp(ins_Store(type), emitActualTypeSize(type), data->gtRegNum, tree);
}
}

Expand Down
21 changes: 6 additions & 15 deletions src/jit/codegenarm64.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1609,9 +1609,7 @@ void CodeGen::genCodeForLclVar(GenTreeLclVar* tree)
assert(targetType != TYP_STRUCT);

instruction ins = ins_Load(targetType);
emitAttr attr = emitTypeSize(targetType);

attr = varTypeIsFloating(targetType) ? attr : emit->emitInsAdjustLoadStoreAttr(ins, attr);
emitAttr attr = emitActualTypeSize(targetType);

emit->emitIns_R_S(ins, attr, tree->gtRegNum, varNum, 0);
genProduceReg(tree);
Expand Down Expand Up @@ -1671,9 +1669,7 @@ void CodeGen::genCodeForStoreLclFld(GenTreeLclFld* tree)

instruction ins = ins_Store(targetType);

emitAttr attr = emitTypeSize(targetType);

attr = varTypeIsFloating(targetType) ? attr : emit->emitInsAdjustLoadStoreAttr(ins, attr);
emitAttr attr = emitActualTypeSize(targetType);

emit->emitIns_S_R(ins, attr, dataReg, varNum, offset);

Expand Down Expand Up @@ -1750,9 +1746,7 @@ void CodeGen::genCodeForStoreLclVar(GenTreeLclVar* tree)
inst_set_SV_var(tree);

instruction ins = ins_Store(targetType);
emitAttr attr = emitTypeSize(targetType);

attr = varTypeIsFloating(targetType) ? attr : emit->emitInsAdjustLoadStoreAttr(ins, attr);
emitAttr attr = emitActualTypeSize(targetType);

emit->emitIns_S_R(ins, attr, dataReg, varNum, /* offset */ 0);

Expand Down Expand Up @@ -3115,7 +3109,7 @@ void CodeGen::genCodeForStoreInd(GenTreeStoreInd* tree)
}
}

getEmitter()->emitInsLoadStoreOp(ins, emitTypeSize(type), dataReg, tree);
getEmitter()->emitInsLoadStoreOp(ins, emitActualTypeSize(type), dataReg, tree);
}
}

Expand Down Expand Up @@ -4558,17 +4552,14 @@ void CodeGen::genSIMDIntrinsicGetItem(GenTreeSIMD* simdNode)
{
int offset = (int)index * genTypeSize(baseType);
instruction ins = ins_Load(baseType);
baseTypeSize = varTypeIsFloating(baseType)
? baseTypeSize
: getEmitter()->emitInsAdjustLoadStoreAttr(ins, baseTypeSize);

assert(!op1->isUsedFromReg());

if (op1->OperIsLocal())
{
unsigned varNum = op1->gtLclVarCommon.gtLclNum;

getEmitter()->emitIns_R_S(ins, baseTypeSize, targetReg, varNum, offset);
getEmitter()->emitIns_R_S(ins, emitActualTypeSize(baseType), targetReg, varNum, offset);
}
else
{
Expand All @@ -4579,7 +4570,7 @@ void CodeGen::genSIMDIntrinsicGetItem(GenTreeSIMD* simdNode)
regNumber baseReg = addr->gtRegNum;

// ldr targetReg, [baseReg, #offset]
getEmitter()->emitIns_R_R_I(ins, baseTypeSize, targetReg, baseReg, offset);
getEmitter()->emitIns_R_R_I(ins, emitActualTypeSize(baseType), targetReg, baseReg, offset);
}
}
else
Expand Down
22 changes: 6 additions & 16 deletions src/jit/codegenarmarch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1623,17 +1623,7 @@ void CodeGen::genCodeForLclFld(GenTreeLclFld* tree)
unsigned varNum = tree->gtLclNum;
assert(varNum < compiler->lvaCount);

if (varTypeIsFloating(targetType) || varTypeIsSIMD(targetType))
{
emit->emitIns_R_S(ins_Load(targetType), size, targetReg, varNum, offs);
}
else
{
#ifdef _TARGET_ARM64_
size = EA_SET_SIZE(size, EA_8BYTE);
#endif // _TARGET_ARM64_
emit->emitIns_R_S(ins_Move_Extend(targetType, false), size, targetReg, varNum, offs);
}
emit->emitIns_R_S(ins_Load(targetType), emitActualTypeSize(targetType), targetReg, varNum, offs);

genProduceReg(tree);
}
Expand Down Expand Up @@ -1753,7 +1743,7 @@ void CodeGen::genCodeForIndir(GenTreeIndir* tree)
}
}

getEmitter()->emitInsLoadStoreOp(ins, emitTypeSize(type), targetReg, tree);
getEmitter()->emitInsLoadStoreOp(ins, emitActualTypeSize(type), targetReg, tree);

if (emitBarrier)
{
Expand Down Expand Up @@ -1930,14 +1920,14 @@ void CodeGen::genCodeForCpBlkUnroll(GenTreeBlk* cpBlkNode)
{
if ((size & 2) != 0)
{
genCodeForLoadOffset(INS_ldrh, EA_2BYTE, tmpReg, srcAddr, offset);
genCodeForStoreOffset(INS_strh, EA_2BYTE, tmpReg, dstAddr, offset);
genCodeForLoadOffset(INS_ldrh, EA_4BYTE, tmpReg, srcAddr, offset);
genCodeForStoreOffset(INS_strh, EA_4BYTE, tmpReg, dstAddr, offset);
offset += 2;
}
if ((size & 1) != 0)
{
genCodeForLoadOffset(INS_ldrb, EA_1BYTE, tmpReg, srcAddr, offset);
genCodeForStoreOffset(INS_strb, EA_1BYTE, tmpReg, dstAddr, offset);
genCodeForLoadOffset(INS_ldrb, EA_4BYTE, tmpReg, srcAddr, offset);
genCodeForStoreOffset(INS_strb, EA_4BYTE, tmpReg, dstAddr, offset);
}
}
#endif // !_TARGET_ARM64_
Expand Down
5 changes: 1 addition & 4 deletions src/jit/codegenlinear.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -893,12 +893,9 @@ void CodeGen::genUnspillRegIfNeeded(GenTree* tree)
targetType = genActualType(varDsc->lvType);
}
instruction ins = ins_Load(targetType, compiler->isSIMDTypeLocalAligned(lcl->gtLclNum));
emitAttr attr = emitTypeSize(targetType);
emitAttr attr = emitActualTypeSize(targetType);
emitter* emit = getEmitter();

// Fixes Issue #3326
attr = varTypeIsFloating(targetType) ? attr : emit->emitInsAdjustLoadStoreAttr(ins, attr);

// Load local variable from its home location.
inst_RV_TT(ins, dstReg, unspillTree, 0, attr);
#elif defined(_TARGET_ARM_)
Expand Down
24 changes: 12 additions & 12 deletions src/jit/emitarm.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2247,14 +2247,14 @@ void emitter::emitIns_R_R(
break;

case INS_tbb:
assert(size == EA_1BYTE);
assert(size == EA_4BYTE);
assert(insDoesNotSetFlags(flags));
fmt = IF_T2_C9;
sf = INS_FLAGS_NOT_SET;
break;

case INS_tbh:
assert(size == EA_2BYTE);
assert(size == EA_4BYTE);
assert(insDoesNotSetFlags(flags));
fmt = IF_T2_C9;
sf = INS_FLAGS_NOT_SET;
Expand All @@ -2268,15 +2268,15 @@ void emitter::emitIns_R_R(

case INS_ldrexb:
case INS_strexb:
assert(size == EA_1BYTE);
assert(size == EA_4BYTE);
assert(insDoesNotSetFlags(flags));
fmt = IF_T2_E1;
sf = INS_FLAGS_NOT_SET;
break;

case INS_ldrexh:
case INS_strexh:
assert(size == EA_2BYTE);
assert(size == EA_4BYTE);
assert(insDoesNotSetFlags(flags));
fmt = IF_T2_E1;
sf = INS_FLAGS_NOT_SET;
Expand Down Expand Up @@ -2701,7 +2701,7 @@ void emitter::emitIns_R_R_I(instruction ins,

case INS_ldrb:
case INS_strb:
assert(size == EA_1BYTE);
assert(size == EA_4BYTE);
assert(insDoesNotSetFlags(flags));

if (isLowRegister(reg1) && isLowRegister(reg2) && insOptsNone(opt) && ((imm & 0x001f) == imm))
Expand All @@ -2713,12 +2713,12 @@ void emitter::emitIns_R_R_I(instruction ins,
goto COMMON_THUMB2_LDST;

case INS_ldrsb:
assert(size == EA_1BYTE);
assert(size == EA_4BYTE);
goto COMMON_THUMB2_LDST;

case INS_ldrh:
case INS_strh:
assert(size == EA_2BYTE);
assert(size == EA_4BYTE);
assert(insDoesNotSetFlags(flags));

if (isLowRegister(reg1) && isLowRegister(reg2) && insOptsNone(opt) && ((imm & 0x003e) == imm))
Expand All @@ -2730,7 +2730,7 @@ void emitter::emitIns_R_R_I(instruction ins,
goto COMMON_THUMB2_LDST;

case INS_ldrsh:
assert(size == EA_2BYTE);
assert(size == EA_4BYTE);
goto COMMON_THUMB2_LDST;

case INS_vldr:
Expand Down Expand Up @@ -3015,13 +3015,13 @@ void emitter::emitIns_R_R_R(instruction ins,
case INS_ldrb:
case INS_strb:
case INS_ldrsb:
assert(size == EA_1BYTE);
assert(size == EA_4BYTE);
goto COMMON_THUMB1_LDST;

case INS_ldrsh:
case INS_ldrh:
case INS_strh:
assert(size == EA_2BYTE);
assert(size == EA_4BYTE);
goto COMMON_THUMB1_LDST;

case INS_ldr:
Expand Down Expand Up @@ -3269,13 +3269,13 @@ void emitter::emitIns_R_R_R_I(instruction ins,
case INS_ldrb:
case INS_ldrsb:
case INS_strb:
assert(size == EA_1BYTE);
assert(size == EA_4BYTE);
goto COMMON_THUMB2_LDST;

case INS_ldrh:
case INS_ldrsh:
case INS_strh:
assert(size == EA_2BYTE);
assert(size == EA_4BYTE);
goto COMMON_THUMB2_LDST;

case INS_ldr:
Expand Down
46 changes: 8 additions & 38 deletions src/jit/emitarm64.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -960,34 +960,6 @@ bool emitter::emitInsMayWriteMultipleRegs(instrDesc* id)
}
}

// For the small loads/store instruction we adjust the size 'attr'
// depending upon whether we have a load or a store
//
emitAttr emitter::emitInsAdjustLoadStoreAttr(instruction ins, emitAttr attr)
{
if (EA_SIZE(attr) < EA_4BYTE)
{
if (emitInsIsLoad(ins))
{
// The value of 'ins' encodes the size to load
// we use EA_8BYTE here because it is the size we will write (into dataReg)
//
attr = EA_8BYTE;
}
else
{
assert(emitInsIsStore(ins));

// The value of 'ins' encodes the size to store
// we use EA_4BYTE here because it is the size of the register
// that we want to display when storing small values
//
attr = EA_4BYTE;
}
}
return attr;
}

// Takes an instrDesc 'id' and uses the instruction 'ins' to determine the
// size of the target register that is written or read by the instruction.
// Note that even if EA_4BYTE is returned a load instruction will still
Expand Down Expand Up @@ -11590,8 +11562,6 @@ void emitter::emitDispFrameRef(int varx, int disp, int offs, bool asmfm)
//
void emitter::emitInsLoadStoreOp(instruction ins, emitAttr attr, regNumber dataReg, GenTreeIndir* indir)
{
emitAttr ldstAttr = isVectorRegister(dataReg) ? attr : emitInsAdjustLoadStoreAttr(ins, attr);

GenTree* addr = indir->Addr();

if (addr->isContained())
Expand Down Expand Up @@ -11640,7 +11610,7 @@ void emitter::emitInsLoadStoreOp(instruction ins, emitAttr attr, regNumber dataR
noway_assert(emitInsIsLoad(ins) || (tmpReg != dataReg));

// Then load/store dataReg from/to [tmpReg + offset]
emitIns_R_R_I(ins, ldstAttr, dataReg, tmpReg, offset);
emitIns_R_R_I(ins, attr, dataReg, tmpReg, offset);
}
else // large offset
{
Expand All @@ -11654,29 +11624,29 @@ void emitter::emitInsLoadStoreOp(instruction ins, emitAttr attr, regNumber dataR
noway_assert(tmpReg != index->gtRegNum);

// Then load/store dataReg from/to [tmpReg + index*scale]
emitIns_R_R_R_I(ins, ldstAttr, dataReg, tmpReg, index->gtRegNum, lsl, INS_OPTS_LSL);
emitIns_R_R_R_I(ins, attr, dataReg, tmpReg, index->gtRegNum, lsl, INS_OPTS_LSL);
}
}
else // (offset == 0)
{
if (lsl > 0)
{
// Then load/store dataReg from/to [memBase + index*scale]
emitIns_R_R_R_I(ins, ldstAttr, dataReg, memBase->gtRegNum, index->gtRegNum, lsl, INS_OPTS_LSL);
emitIns_R_R_R_I(ins, attr, dataReg, memBase->gtRegNum, index->gtRegNum, lsl, INS_OPTS_LSL);
}
else // no scale
{
// Then load/store dataReg from/to [memBase + index]
emitIns_R_R_R(ins, ldstAttr, dataReg, memBase->gtRegNum, index->gtRegNum);
emitIns_R_R_R(ins, attr, dataReg, memBase->gtRegNum, index->gtRegNum);
}
}
}
else // no Index register
{
if (emitIns_valid_imm_for_ldst_offset(offset, EA_SIZE(attr)))
if (emitIns_valid_imm_for_ldst_offset(offset, emitTypeSize(indir->TypeGet())))
{
// Then load/store dataReg from/to [memBase + offset]
emitIns_R_R_I(ins, ldstAttr, dataReg, memBase->gtRegNum, offset);
emitIns_R_R_I(ins, attr, dataReg, memBase->gtRegNum, offset);
}
else
{
Expand All @@ -11687,14 +11657,14 @@ void emitter::emitInsLoadStoreOp(instruction ins, emitAttr attr, regNumber dataR
codeGen->instGen_Set_Reg_To_Imm(EA_PTRSIZE, tmpReg, offset);

// Then load/store dataReg from/to [memBase + tmpReg]
emitIns_R_R_R(ins, ldstAttr, dataReg, memBase->gtRegNum, tmpReg);
emitIns_R_R_R(ins, attr, dataReg, memBase->gtRegNum, tmpReg);
}
}
}
else // addr is not contained, so we evaluate it into a register
{
// Then load/store dataReg from/to [addrReg]
emitIns_R_R(ins, ldstAttr, dataReg, addr->gtRegNum);
emitIns_R_R(ins, attr, dataReg, addr->gtRegNum);
}
}

Expand Down
1 change: 0 additions & 1 deletion src/jit/emitarm64.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ bool emitInsIsCompare(instruction ins);
bool emitInsIsLoad(instruction ins);
bool emitInsIsStore(instruction ins);
bool emitInsIsLoadOrStore(instruction ins);
emitAttr emitInsAdjustLoadStoreAttr(instruction ins, emitAttr attr);
emitAttr emitInsTargetRegSize(instrDesc* id);
emitAttr emitInsLoadStoreSize(instrDesc* id);

Expand Down

0 comments on commit e3146c7

Please sign in to comment.