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 - Extend emitter peephole optimization of eliminating unnecessary mov instructions #79381

Merged
merged 88 commits into from
Feb 25, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
88 commits
Select commit Hold shift + click to select a range
f5feaeb
Added regUpper32BitsZeroLookup
TIHan Dec 7, 2022
958a84f
Minor refactoring
TIHan Dec 7, 2022
755461e
Refactoring
TIHan Dec 7, 2022
95c7d09
Refactoring
TIHan Dec 7, 2022
12e3c4f
Refactoring
TIHan Dec 7, 2022
b42682c
Added comments, more refactoring
TIHan Dec 8, 2022
2d806a5
Added some logs
TIHan Dec 8, 2022
2d5e07f
Added some logs
TIHan Dec 8, 2022
4d06e05
Added some logs
TIHan Dec 8, 2022
0c1599a
Added some logs
TIHan Dec 8, 2022
53ecd41
Remove logs. Fixed an issue.
TIHan Dec 8, 2022
0f42667
Expanding peephole opt even more. Trying to improve tp.
TIHan Dec 8, 2022
2de6089
Some refactoring
TIHan Dec 8, 2022
bd3b211
Trying to make this safe
TIHan Dec 8, 2022
9792d0f
work
TIHan Dec 8, 2022
0ed30e9
more work
TIHan Dec 8, 2022
3e452fd
Fixing a few issues
TIHan Dec 8, 2022
126d6b5
Fixing a bug
TIHan Dec 9, 2022
80ac0d7
Switching to a list of instrs
TIHan Dec 9, 2022
9497d5e
Adding simple peephole API
TIHan Dec 10, 2022
4ee489b
minor change
TIHan Dec 10, 2022
7d599df
Trying to improve tp
TIHan Dec 11, 2022
11a1709
Trying to improve tp
TIHan Dec 11, 2022
e33147a
Fixed a minor issue
TIHan Dec 11, 2022
99cba5a
Small correctness issue
TIHan Dec 11, 2022
0356348
Minor change
TIHan Dec 11, 2022
2c10d21
Using emitCurIGinsCnt
TIHan Dec 12, 2022
189afe0
Added EMIT_MAX_IG_INS_COUNT
TIHan Dec 12, 2022
f0e626a
Formatting
TIHan Dec 12, 2022
f1e7c07
Feedback
TIHan Dec 12, 2022
ca8813a
Feedback
TIHan Dec 12, 2022
c533d54
Feedback
TIHan Dec 12, 2022
e0df38e
Minor change
TIHan Dec 12, 2022
1f5501a
More comments, minor change
TIHan Dec 12, 2022
e34367f
Get rid of these asserts
TIHan Dec 12, 2022
b2c0414
Improving TP even further
TIHan Dec 13, 2022
5431a53
Added CalculateChecksumTest
TIHan Jan 4, 2023
07ec7fb
Only allow GPR
TIHan Jan 4, 2023
7e0079f
Update src/coreclr/jit/emitxarch.cpp
TIHan Jan 6, 2023
0c71902
Feedback
TIHan Jan 7, 2023
25cb4fb
Feedback
TIHan Jan 7, 2023
7d6d03d
Merge remote-tracking branch 'upstream/main' into mov-elim-1
TIHan Jan 10, 2023
ee349d3
Updating last instructions when saving IG. Using 'min(emitCurIGinsCnt…
TIHan Jan 10, 2023
a2538ca
Added 'emitCurLastInsCnt'
TIHan Jan 10, 2023
6fb2694
Remove commented code
TIHan Jan 10, 2023
7e97320
Using 'emitCurLastInsCnt' for the ring buffer
TIHan Jan 10, 2023
896a32a
Formatting
TIHan Jan 11, 2023
49c3c07
Fix last instructions if the next IG is extended
TIHan Jan 11, 2023
f229bc0
Update emit.cpp
TIHan Jan 12, 2023
920fc7c
Update emit.cpp
TIHan Jan 12, 2023
7380971
Merged
TIHan Jan 17, 2023
a4e92c7
Fix build
TIHan Jan 17, 2023
fa4f6e5
Fix build
TIHan Jan 17, 2023
0fd25ca
Fix build
TIHan Jan 17, 2023
2ff99ed
Formatting
TIHan Jan 17, 2023
d2307d5
Fix assertion
TIHan Jan 18, 2023
f478d53
merging
TIHan Feb 9, 2023
0afbc01
Fixed minor issue
TIHan Feb 9, 2023
140fd11
Merging complete
TIHan Feb 9, 2023
423a72f
Fix build
TIHan Feb 9, 2023
c6c5711
Added canPeepholeCrossIGBoundaries
TIHan Feb 9, 2023
cab9705
Cleanup
TIHan Feb 9, 2023
987e50c
More cleanup
TIHan Feb 9, 2023
453e00b
More cleanup
TIHan Feb 9, 2023
710b039
More cleanup
TIHan Feb 9, 2023
cadbf4d
More cleanup
TIHan Feb 9, 2023
a68cd3f
More cleanup
TIHan Feb 9, 2023
26cb484
Fix build
TIHan Feb 9, 2023
a88103a
Add count back
TIHan Feb 9, 2023
5fe41e2
Formatting
TIHan Feb 9, 2023
c1b612f
Merge remote-tracking branch 'upstream/main' into mov-elim-1
TIHan Feb 14, 2023
9f42790
Merge branch 'main' into mov-elim-1
TIHan Feb 15, 2023
84f076c
Added back comments
TIHan Feb 15, 2023
12ea52f
Turn off backwards nav for arm
TIHan Feb 15, 2023
cb3b2e8
Added debug check for backward nav checks
TIHan Feb 15, 2023
a007c3b
Remove debug check
TIHan Feb 15, 2023
5e33fad
Merge remote-tracking branch 'upstream/main' into mov-elim-1
TIHan Feb 15, 2023
b35c10e
Feedback. Allow looking back at conditional ops.
TIHan Feb 22, 2023
7576c94
Update src/coreclr/jit/emit.h
TIHan Feb 22, 2023
6816216
Feedback
TIHan Feb 22, 2023
3a4adfb
Feedback
TIHan Feb 22, 2023
5a3acc5
Fix ordering
TIHan Feb 22, 2023
d48e429
Added regression test. div,idiv,mul,imul should be noted as write to …
TIHan Feb 22, 2023
90479c5
Added another regression test
TIHan Feb 22, 2023
f7224c2
Actually added the regression test
TIHan Feb 22, 2023
36ffb3a
Fix test
TIHan Feb 23, 2023
d1d7814
Merged
TIHan Feb 23, 2023
8162a38
Fix merge
TIHan Feb 23, 2023
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
32 changes: 17 additions & 15 deletions src/coreclr/jit/emit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1072,8 +1072,8 @@ insGroup* emitter::emitSavIG(bool emitAdd)
// being adding causes a new EXTEND IG to be created. Also, emitLastIns might not be in this IG
// at all if this IG is empty.

assert((emitLastIns == nullptr) == (emitLastInsIG == nullptr));
if ((emitLastIns != nullptr) && (sz != 0))
assert(emitHasLastIns() == (emitLastInsIG != nullptr));
if (emitHasLastIns() && (sz != 0))
{
// If we get here, emitLastIns must be in the current IG we are saving.
assert(emitLastInsIG == emitCurIG);
Expand Down Expand Up @@ -1499,8 +1499,6 @@ size_t emitter::emitGenEpilogLst(size_t (*fp)(void*, unsigned), void* cp)

void* emitter::emitAllocAnyInstr(size_t sz, emitAttr opsz)
{
instrDesc* id;

#ifdef DEBUG
// Under STRESS_EMITTER, put every instruction in its own instruction group.
// We can't do this for a prolog, epilog, funclet prolog, or funclet epilog,
Expand Down Expand Up @@ -1557,18 +1555,18 @@ void* emitter::emitAllocAnyInstr(size_t sz, emitAttr opsz)
assert(IsCodeAligned(emitCurIGsize));

// Make sure we have enough space for the new instruction.
// `igInsCnt` is currently a byte, so we can't have more than 255 instructions in a single insGroup.

size_t fullSize = sz + m_debugInfoSize;

if ((emitCurIGfreeNext + fullSize >= emitCurIGfreeEndp) || emitForceNewIG || (emitCurIGinsCnt >= 255))
if ((emitCurIGfreeNext + fullSize >= emitCurIGfreeEndp) || emitForceNewIG ||
(emitCurIGinsCnt >= (EMIT_MAX_IG_INS_COUNT - 1)))
{
emitNxtIG(true);
}

/* Grab the space for the instruction */

emitLastIns = id = (instrDesc*)(emitCurIGfreeNext + m_debugInfoSize);
instrDesc* id = emitLastIns = (instrDesc*)(emitCurIGfreeNext + m_debugInfoSize);

#if EMIT_BACKWARDS_NAVIGATION
emitCurIG->igLastIns = id;
Expand Down Expand Up @@ -1606,14 +1604,18 @@ void* emitter::emitAllocAnyInstr(size_t sz, emitAttr opsz)
if (m_debugInfoSize > 0)
{
instrDescDebugInfo* info = (instrDescDebugInfo*)emitGetMem(sizeof(*info));
info->idNum = emitInsCount;
info->idSize = sz;
info->idVarRefOffs = 0;
info->idMemCookie = 0;
info->idFlags = GTF_EMPTY;
info->idFinallyCall = false;
info->idCatchRet = false;
info->idCallSig = nullptr;
memset(info, 0, sizeof(instrDescDebugInfo));

// These fields should have been zero-ed by the above
assert(info->idVarRefOffs == 0);
assert(info->idMemCookie == 0);
assert(info->idFlags == GTF_EMPTY);
assert(info->idFinallyCall == false);
assert(info->idCatchRet == false);
assert(info->idCallSig == nullptr);

info->idNum = emitInsCount;
info->idSize = sz;
id->idDebugOnlyInfo(info);
}

Expand Down
110 changes: 97 additions & 13 deletions src/coreclr/jit/emit.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,11 @@
#define EMIT_INSTLIST_VERBOSE (emitComp->verbose)
#endif

#define EMIT_BACKWARDS_NAVIGATION 0 // If 1, enable backwards navigation code for MIR (insGroup/instrDesc).
#ifdef TARGET_XARCH
#define EMIT_BACKWARDS_NAVIGATION 1 // If 1, enable backwards navigation code for MIR (insGroup/instrDesc).
#else
#define EMIT_BACKWARDS_NAVIGATION 0
#endif

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

Expand Down Expand Up @@ -2288,31 +2292,111 @@ class emitter
return (emitCurIG && emitCurIGfreeNext > emitCurIGfreeBase);
}

#define EMIT_MAX_IG_INS_COUNT 256
TIHan marked this conversation as resolved.
Show resolved Hide resolved

#if EMIT_BACKWARDS_NAVIGATION
#define EMIT_MAX_PEEPHOLE_INS_COUNT 32 // The max number of previous instructions to navigate through for peepholes.
#endif // EMIT_BACKWARDS_NAVIGATION

instrDesc* emitLastIns;
insGroup* emitLastInsIG;

#if EMIT_BACKWARDS_NAVIGATION
unsigned emitLastInsFullSize;
#endif // EMIT_BACKWARDS_NAVIGATION

// Check to see if the last instruction is available.
inline bool emitHasLastIns() const
{
return (emitLastIns != nullptr);
}

// Checks to see if we can cross between the two given IG boundaries.
TIHan marked this conversation as resolved.
Show resolved Hide resolved
//
// We have the following checks:
// 1. Looking backwards across an IG boundary can only be done if we're in an extension IG.
// 2. The IG of the previous instruction must have the same GC interrupt status as the current IG.
inline bool isInsIGSafeForPeepholeOptimization(insGroup* prevInsIG, insGroup* curInsIG) const
{
if (prevInsIG == curInsIG)
{
return true;
}
else
{
return (curInsIG->igFlags & IGF_EXTEND) &&
((prevInsIG->igFlags & IGF_NOGCINTERRUPT) == (curInsIG->igFlags & IGF_NOGCINTERRUPT));
}
}

// Check if a peephole optimization involving emitLastIns is safe.
//
// We have the following checks:
// 1. There must be a non-null emitLastIns to consult (thus, we have a known "last" instruction).
// 2. `emitForceNewIG` is not set: this prevents peepholes from crossing nogc boundaries where
// the next instruction is forced to create a new IG.
// 3. Looking backwards across an IG boundary can only be done if we're in an extension IG.
// 4. The IG of the previous instruction must have the same GC interrupt status as the current IG.
// This is related to #2; it disallows peephole when the previous IG is GC and the current is NOGC.
bool emitCanPeepholeLastIns()
{
assert((emitLastIns == nullptr) == (emitLastInsIG == nullptr));

return (emitLastIns != nullptr) && !emitForceNewIG &&
((emitCurIGinsCnt > 0) || // we're not at the start of a new IG
((emitCurIG->igFlags & IGF_EXTEND) != 0)) && // or we are at the start of a new IG,
// and it's an extension IG
((emitLastInsIG->igFlags & IGF_NOGCINTERRUPT) == (emitCurIG->igFlags & IGF_NOGCINTERRUPT));
bool emitCanPeepholeLastIns() const
TIHan marked this conversation as resolved.
Show resolved Hide resolved
{
assert(emitHasLastIns() == (emitLastInsIG != nullptr));

return emitHasLastIns() && // there is an emitLastInstr
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this is for a separate PR, but I think we need to prevent peephole optimizations when we're in the prolog or epilog. I.e.,

 if (emitIGisInProlog(emitCurIG) || emitIGisInEpilog(emitCurIG))
    {
        return false;
    }
#ifdef FEATURE_EH_FUNCLETS
    if (emitIGisInFuncletProlog(emitCurIG) || emitIGisInFuncletEpilog(emitCurIG))
    {
        return false;
    }
#endif

There is too much special handling in the prolog/epilog (e.g., unwinding) to allow peeps to kick in. There may be very specific cases where they are ok, but that requires some careful thinking.

!emitForceNewIG && // and we're not about to start a new IG.
isInsIGSafeForPeepholeOptimization(emitLastInsIG, emitCurIG);
}

enum emitPeepholeResult
{
PEEPHOLE_CONTINUE,
PEEPHOLE_ABORT
};

// Visits the last emitted instructions.
// Must be safe to do - use emitCanPeepholeLastIns for checking.
// Action is a function type: instrDesc* -> emitPeepholeResult
template <typename Action>
TIHan marked this conversation as resolved.
Show resolved Hide resolved
void emitPeepholeIterateLastInstrs(Action action)
{
assert(emitCanPeepholeLastIns());

#if EMIT_BACKWARDS_NAVIGATION
insGroup* curInsIG;
instrDesc* id;

if (!emitGetLastIns(&curInsIG, &id))
return;

for (unsigned i = 0; i < EMIT_MAX_PEEPHOLE_INS_COUNT; i++)
{
assert(id != nullptr);

switch (action(id))
{
case PEEPHOLE_ABORT:
return;
case PEEPHOLE_CONTINUE:
{
insGroup* savedInsIG = curInsIG;
if (emitPrevID(curInsIG, id))
{
if (isInsIGSafeForPeepholeOptimization(curInsIG, savedInsIG))
{
continue;
}
else
{
return;
}
}
return;
}

default:
unreached();
}
}
#else // EMIT_BACKWARDS_NAVIGATION
action(emitLastIns);
#endif // !EMIT_BACKWARDS_NAVIGATION
}

#ifdef TARGET_ARMARCH
Expand Down
3 changes: 2 additions & 1 deletion src/coreclr/jit/emitarm64.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -16030,7 +16030,8 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN

if (canOptimize && // Don't optimize if unsafe.
(emitLastIns->idIns() == INS_mov) && // Don't optimize if last instruction was not 'mov'.
(emitLastIns->idOpSize() == size)) // Don't optimize if operand size is different than previous instruction.
(emitLastIns->idOpSize() == size)) // Don't optimize if operand size is different than previous
// instruction.
{
// Check if we did same move in prev instruction except dst/src were switched.
regNumber prevDst = emitLastIns->idReg1();
Expand Down
Loading