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.

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

[JIT] X64 - Centralize peephole optimization for removing redundant mov instructions #85780

Merged
merged 10 commits into from
May 11, 2023
Merged
Changes from 1 commit
Commits
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
Prev Previous commit
Only do the optimizations for 64-bit
TIHan committed May 10, 2023
commit a3e17a0c196363b44872de5e2b1c363ea752fdd8
19 changes: 5 additions & 14 deletions src/coreclr/jit/emitxarch.cpp
Original file line number Diff line number Diff line change
@@ -504,6 +504,7 @@ bool emitter::IsRexW1EvexInstruction(instruction ins)
return false;
}

#ifdef TARGET_64BIT
//------------------------------------------------------------------------
// AreUpperBitsZero: check if some previously emitted
// instruction set the upper bits of reg to zero.
@@ -546,36 +547,30 @@ bool emitter::AreUpperBitsZero(regNumber reg, emitAttr size)
case INS_cwde:
case INS_cdq:
case INS_movsx:
#ifdef TARGET_64BIT
case INS_movsxd:
#endif // TARGET_64BIT
return PEEPHOLE_ABORT;

case INS_movzx:
if ((size == EA_1BYTE) || (size == EA_2BYTE))
{
result = (id->idOpSize() <= size);
}
#ifdef TARGET_64BIT
// movzx always zeroes the upper 32 bits.
else if (size == EA_4BYTE)
{
result = true;
}
#endif // TARGET_64BIT
return PEEPHOLE_ABORT;

default:
break;
}

#ifdef TARGET_64BIT
// otherwise rely on operation size.
if (size == EA_4BYTE)
{
result = (id->idOpSize() == EA_4BYTE);
}
#endif // TARGET_64BIT
return PEEPHOLE_ABORT;
}
else
@@ -628,20 +623,16 @@ bool emitter::AreUpperBitsSignExtended(regNumber reg, emitAttr size)
return PEEPHOLE_ABORT;

case INS_movsx:
#ifdef TARGET_64BIT
case INS_movsxd:
#endif // TARGET_64BIT
if ((size == EA_1BYTE) || (size == EA_2BYTE))
{
result = (id->idOpSize() <= size);
}
#ifdef TARGET_64BIT
// movsx/movsxd always sign extends to 8 bytes. W-bit is set.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the W-bit matter here?

Copy link
Contributor Author

@TIHan TIHan May 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

W-bit on the encoding is what makes movsx and movsxd sign-extend to 8 bytes. We always default to using the W-bit. Technically, we could have a movsx or movsxd instruction not set the W-bit which would not sign-extend to 8 bytes (that would be a problem and make this optimization not safe), but we never actually emit those instructions that way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function cares about instructions and sizes, but not encoding, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment W-bit is set itself is just meant to inform the reason why this optimization is safe to do. It was already there before.

else if (size == EA_4BYTE)
{
result = true;
}
#endif // TARGET_64BIT
break;

default:
@@ -658,6 +649,7 @@ bool emitter::AreUpperBitsSignExtended(regNumber reg, emitAttr size)

return result;
}
#endif // TARGET_64BIT

//------------------------------------------------------------------------
// emitDoesInsModifyFlags: checks if the given instruction modifies flags
@@ -6275,6 +6267,7 @@ bool emitter::IsRedundantMov(
return true;
}

#ifdef TARGET_64BIT
switch (ins)
{
case INS_movzx:
@@ -6286,28 +6279,26 @@ bool emitter::IsRedundantMov(
break;

case INS_movsx:
#ifdef TARGET_64BIT
case INS_movsxd:
#endif // TARGET_64BIT
if (AreUpperBitsSignExtended(src, size))
{
JITDUMP("\n -- suppressing movsx or movsxd because upper bits are sign-extended.\n");
return true;
}
break;

#ifdef TARGET_64BIT
case INS_mov:
if ((size == EA_4BYTE) && AreUpperBitsZero(src, size))
{
JITDUMP("\n -- suppressing mov because upper bits are zero.\n");
return true;
}
break;
#endif // TARGET_64BIT

default:
break;
}
#endif // TARGET_64BIT
}

// TODO-XArch-CQ: Certain instructions, such as movaps vs movups, are equivalent in
2 changes: 2 additions & 0 deletions src/coreclr/jit/emitxarch.h
Original file line number Diff line number Diff line change
@@ -133,8 +133,10 @@ bool IsRedundantStackMov(instruction ins, insFormat fmt, emitAttr size, regNumbe
static bool IsJccInstruction(instruction ins);
static bool IsJmpInstruction(instruction ins);

#ifdef TARGET_64BIT
bool AreUpperBitsZero(regNumber reg, emitAttr size);
bool AreUpperBitsSignExtended(regNumber reg, emitAttr size);
#endif // TARGET_64BIT

bool IsRedundantCmp(emitAttr size, regNumber reg1, regNumber reg2);