Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imul instruction size misprediction #57179

Merged
merged 6 commits into from
Aug 14, 2021
Merged

imul instruction size misprediction #57179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bc192fa
Add assert
kunalspathak Aug 10, 2021
cf6efb7
Remove the assert in emitInsSizeCV
kunalspathak Aug 11, 2021
d15ea32
Add a check for includeRexPrefixSize
kunalspathak Aug 11, 2021
3f5518e
Remove the codeSize() capping code added to fix #12840
kunalspathak Aug 11, 2021
7861678
Make immediate only 4bytes long for non-mov instructions
kunalspathak Aug 11, 2021
9fc5bbc
Delete a commented code
kunalspathak Aug 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/coreclr/jit/clrjit.natvis
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,8 +175,8 @@ Documentation for VS debugger format specifiers: https://docs.microsoft.com/en-u

<!-- Emitter -->
<Type Name="insGroup">
<DisplayString Condition="igFlags &amp; 0x200">IG{igNum,d} [extend]</DisplayString>
<DisplayString>IG{igNum,d}</DisplayString>
<DisplayString Condition="igFlags &amp; 0x200">IG{igNum,d}, size={igSize,d}, offset={igOffs,d} [extend]</DisplayString>
<DisplayString>IG{igNum,d}, size={igSize,d}, offset={igOffs,d}</DisplayString>
</Type>

<Type Name="emitter::instrDesc">
Expand Down
10 changes: 0 additions & 10 deletions src/coreclr/jit/emit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -884,16 +884,6 @@ class emitter
}
void idCodeSize(unsigned sz)
{
if (sz > 15)
kunalspathak marked this conversation as resolved.
Show resolved Hide resolved
{
// This is a temporary workaround for non-precise instr size
// estimator on XARCH. It often overestimates sizes and can
// return value more than 15 that doesn't fit in 4 bits _idCodeSize.
// If somehow we generate instruction that needs more than 15 bytes we
// will fail on another assert in emit.cpp: noway_assert(id->idCodeSize() >= csz).
// Issue https://github.com/dotnet/runtime/issues/12840.
sz = 15;
}
assert(sz <= 15); // Intel decoder limit.
_idCodeSize = sz;
assert(sz == _idCodeSize);
Expand Down
87 changes: 58 additions & 29 deletions src/coreclr/jit/emitxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1177,15 +1177,22 @@ unsigned emitter::emitGetAdjustedSize(instruction ins, emitAttr attr, code_t cod
return adjustedSize;
}

// Get size of rex or vex prefix emitted in code
unsigned emitter::emitGetPrefixSize(code_t code)
//
//------------------------------------------------------------------------
// emitGetPrefixSize: Get size of rex or vex prefix emitted in code
//
// Arguments:
// code -- The current opcode and any known prefixes
// includeRexPrefixSize -- If Rex Prefix size should be included or not
//
unsigned emitter::emitGetPrefixSize(code_t code, bool includeRexPrefixSize)
{
if (hasVexPrefix(code))
{
return 3;
}

if (hasRexPrefix(code))
if (includeRexPrefixSize && hasRexPrefix(code))
{
return 1;
}
Expand Down Expand Up @@ -1946,16 +1953,18 @@ bool emitter::emitVerifyEncodable(instruction ins, emitAttr size, regNumber reg1
return true;
}

/*****************************************************************************
*
* Estimate the size (in bytes of generated code) of the given instruction.
*/

inline UNATIVE_OFFSET emitter::emitInsSize(code_t code)
//------------------------------------------------------------------------
// emitInsSize: Estimate the size (in bytes of generated code) of the given instruction.
//
// Arguments:
// code -- The current opcode and any known prefixes
// includeRexPrefixSize -- If Rex Prefix size should be included or not
//
inline UNATIVE_OFFSET emitter::emitInsSize(code_t code, bool includeRexPrefixSize)
{
UNATIVE_OFFSET size = (code & 0xFF000000) ? 4 : (code & 0x00FF0000) ? 3 : 2;
#ifdef TARGET_AMD64
size += emitGetPrefixSize(code);
size += emitGetPrefixSize(code, includeRexPrefixSize);
#endif
return size;
}
Expand All @@ -1973,17 +1982,19 @@ inline UNATIVE_OFFSET emitter::emitInsSizeRR(instrDesc* id, code_t code)
instruction ins = id->idIns();
emitAttr attr = id->idOpSize();

UNATIVE_OFFSET sz = emitInsSize(code);

sz += emitGetAdjustedSize(ins, attr, code);
UNATIVE_OFFSET sz = emitGetAdjustedSize(ins, attr, code);

bool includeRexPrefixSize = true;
// REX prefix
if (TakesRexWPrefix(ins, attr) || IsExtendedReg(id->idReg1(), attr) || IsExtendedReg(id->idReg2(), attr) ||
(!id->idIsSmallDsc() && (IsExtendedReg(id->idReg3(), attr) || IsExtendedReg(id->idReg4(), attr))))
{
sz += emitGetRexPrefixSize(ins);
includeRexPrefixSize = !IsAVXInstruction(ins);
}

sz += emitInsSize(code, includeRexPrefixSize);

return sz;
}

Expand Down Expand Up @@ -2034,42 +2045,42 @@ inline UNATIVE_OFFSET emitter::emitInsSizeRR(instruction ins, regNumber reg1, re
{
emitAttr size = EA_SIZE(attr);

UNATIVE_OFFSET sz;

// If Byte 4 (which is 0xFF00) is zero, that's where the RM encoding goes.
// Otherwise, it will be placed after the 4 byte encoding, making the total 5 bytes.
// This would probably be better expressed as a different format or something?
code_t code = insCodeRM(ins);

if ((code & 0xFF00) != 0)
{
sz = IsSSEOrAVXInstruction(ins) ? emitInsSize(code) : 5;
}
else
{
sz = emitInsSize(insEncodeRMreg(ins, code));
}

sz += emitGetAdjustedSize(ins, size, insCodeRM(ins));
UNATIVE_OFFSET sz = emitGetAdjustedSize(ins, size, insCodeRM(ins));

bool includeRexPrefixSize = true;
// REX prefix
if (!hasRexPrefix(code))
{
if ((TakesRexWPrefix(ins, size) && ((ins != INS_xor) || (reg1 != reg2))) || IsExtendedReg(reg1, attr) ||
IsExtendedReg(reg2, attr))
{
sz += emitGetRexPrefixSize(ins);
includeRexPrefixSize = false;
}
}

if ((code & 0xFF00) != 0)
{
sz += IsSSEOrAVXInstruction(ins) ? emitInsSize(code, includeRexPrefixSize) : 5;
}
else
{
sz += emitInsSize(insEncodeRMreg(ins, code), includeRexPrefixSize);
}

return sz;
}

/*****************************************************************************/

inline UNATIVE_OFFSET emitter::emitInsSizeSV(code_t code, int var, int dsp)
{
UNATIVE_OFFSET size = emitInsSize(code);
UNATIVE_OFFSET size = emitInsSize(code, /* includeRexPrefixSize */ true);
UNATIVE_OFFSET offs;
bool offsIsUpperBound = true;
bool EBPbased = true;
Expand Down Expand Up @@ -2605,14 +2616,17 @@ inline UNATIVE_OFFSET emitter::emitInsSizeCV(instrDesc* id, code_t code)

size += emitGetAdjustedSize(ins, attrSize, code);

bool includeRexPrefixSize = true;

// 64-bit operand instructions will need a REX.W prefix
if (TakesRexWPrefix(ins, attrSize) || IsExtendedReg(id->idReg1(), attrSize) ||
IsExtendedReg(id->idReg2(), attrSize))
{
size += emitGetRexPrefixSize(ins);
includeRexPrefixSize = false;
}

return size + emitInsSize(code);
return size + emitInsSize(code, includeRexPrefixSize);
}

inline UNATIVE_OFFSET emitter::emitInsSizeCV(instrDesc* id, code_t code, int val)
Expand All @@ -2621,7 +2635,14 @@ inline UNATIVE_OFFSET emitter::emitInsSizeCV(instrDesc* id, code_t code, int val
UNATIVE_OFFSET valSize = EA_SIZE_IN_BYTES(id->idOpSize());
bool valInByte = ((signed char)val == val) && (ins != INS_mov) && (ins != INS_test);

#ifndef TARGET_AMD64
#ifdef TARGET_AMD64
// 64-bit immediates are only supported on mov r64, imm64
// As per manual:
// Support for 64-bit immediate operands is accomplished by expanding
// the semantics of the existing move (MOV reg, imm16/32) instructions.
if ((valSize > sizeof(INT32)) && (ins != INS_mov))
valSize = sizeof(INT32);
#else
// occasionally longs get here on x86
if (valSize > sizeof(INT32))
valSize = sizeof(INT32);
Expand Down Expand Up @@ -4035,7 +4056,15 @@ void emitter::emitIns_R_I(instruction ins, emitAttr attr, regNumber reg, ssize_t
{
if (IsSSEOrAVXInstruction(ins))
{
sz = emitInsSize(insCodeMI(ins));
bool includeRexPrefixSize = true;
// Do not get the RexSize() but just decide if it will be included down further and if yes,
// do not include it again.
if (IsExtendedReg(reg, attr) || TakesRexWPrefix(ins, size) || instrIsExtendedReg3opImul(ins))
{
includeRexPrefixSize = false;
}

sz = emitInsSize(insCodeMI(ins), includeRexPrefixSize);
sz += 1;
}
else if (size == EA_1BYTE && reg == REG_EAX && !instrIs3opImul(ins))
Expand Down
4 changes: 2 additions & 2 deletions src/coreclr/jit/emitxarch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ struct CnsVal
bool cnsReloc;
};

UNATIVE_OFFSET emitInsSize(code_t code);
UNATIVE_OFFSET emitInsSize(code_t code, bool includeRexPrefixSize);
UNATIVE_OFFSET emitInsSizeSV(code_t code, int var, int dsp);
UNATIVE_OFFSET emitInsSizeSV(instrDesc* id, code_t code, int var, int dsp);
UNATIVE_OFFSET emitInsSizeSV(instrDesc* id, code_t code, int var, int dsp, int val);
Expand Down Expand Up @@ -67,7 +67,7 @@ BYTE* emitOutputLJ(insGroup* ig, BYTE* dst, instrDesc* id);
unsigned emitOutputRexOrVexPrefixIfNeeded(instruction ins, BYTE* dst, code_t& code);
unsigned emitGetRexPrefixSize(instruction ins);
unsigned emitGetVexPrefixSize(instruction ins, emitAttr attr);
unsigned emitGetPrefixSize(code_t code);
unsigned emitGetPrefixSize(code_t code, bool includeRexPrefixSize);
unsigned emitGetAdjustedSize(instruction ins, emitAttr attr, code_t code);

unsigned insEncodeReg012(instruction ins, regNumber reg, emitAttr size, code_t* code);
Expand Down