From 31f9cca81a2baba30ff9385eff3d172c5d22b5d2 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 19 Jun 2020 12:50:33 -0700 Subject: [PATCH 01/13] Added IsOppositeOfPrevMov() Added IsOppositeOfPrevMov() that will skip generating redundant mov. --- src/coreclr/src/jit/emitarm64.cpp | 36 +++++++++++++++++++++++++++++++ src/coreclr/src/jit/emitarm64.h | 2 ++ 2 files changed, 38 insertions(+) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index ced2297cbda2a..1719cd19679f3 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -4025,6 +4025,11 @@ void emitter::emitIns_R_R( } } + if (IsOppositeOfPrevMov(ins, reg1, reg2)) + { + return; + } + // Check for the 'mov' aliases for the vector registers if (isVectorRegister(reg1)) { @@ -14882,4 +14887,35 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins #endif // defined(DEBUG) || defined(LATE_DISASM) +//---------------------------------------------------------------------------------------- +// IsOppositeOfPrevMov: +// Check if previous instruction was a mov where dst was current src and src was current dst. +// +// Arguments: +// ins - The current instruction +// dst - The current destination +// src - The current source +// +// Return Value: +// true if previous instruction moved from current dst to src. + +bool emitter::IsOppositeOfPrevMov(instruction ins, regNumber dst, regNumber src) +{ + assert(ins == INS_mov); + + if (emitLastIns != nullptr && emitLastIns->idIns() == INS_mov) + { + // Check if we did same move in prev instruction except dst/src were switched. + regNumber prevDst = emitLastIns->idReg1(); + regNumber prevSrc = emitLastIns->idReg2(); + + if ((prevDst == src) && (prevSrc == dst)) + { + JITDUMP("\n -- suppressing mov as previous instruction did opposite mov.\n", dst, src, + this->emitComp->info.compMethodName, prevDst, prevSrc); + return true; + } + } + return false; +} #endif // defined(TARGET_ARM64) diff --git a/src/coreclr/src/jit/emitarm64.h b/src/coreclr/src/jit/emitarm64.h index 9c365c005d1fd..c8a4e15c15c33 100644 --- a/src/coreclr/src/jit/emitarm64.h +++ b/src/coreclr/src/jit/emitarm64.h @@ -48,6 +48,8 @@ void emitDispExtendReg(regNumber reg, insOpts opt, ssize_t imm); void emitDispAddrRI(regNumber reg, insOpts opt, ssize_t imm); void emitDispAddrRRExt(regNumber reg1, regNumber reg2, insOpts opt, bool isScaled, emitAttr size); +bool IsOppositeOfPrevMov(instruction ins, regNumber dst, regNumber src); + void emitDispIns(instrDesc* id, bool isNew, bool doffs, From dcf463edca8677065bc55df321848d65b87a09a0 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 19 Jun 2020 18:28:06 -0700 Subject: [PATCH 02/13] review comments --- src/coreclr/src/jit/emitarm64.cpp | 15 ++++++++------- src/coreclr/src/jit/emitarm64.h | 6 ++++-- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index 1719cd19679f3..079260717147e 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -4025,7 +4025,7 @@ void emitter::emitIns_R_R( } } - if (IsOppositeOfPrevMov(ins, reg1, reg2)) + if (IsMovRedundantToPrevMov(ins, reg1, reg2)) { return; } @@ -14888,8 +14888,9 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins #endif // defined(DEBUG) || defined(LATE_DISASM) //---------------------------------------------------------------------------------------- -// IsOppositeOfPrevMov: -// Check if previous instruction was a mov where dst was current src and src was current dst. +// IsMovRedundantToPrevMov: +// Check if previous instruction was a 'mov' where dst was current src and src was current dst. +// Or if previous `mov` moved between same src/dst. // // Arguments: // ins - The current instruction @@ -14899,7 +14900,7 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins // Return Value: // true if previous instruction moved from current dst to src. -bool emitter::IsOppositeOfPrevMov(instruction ins, regNumber dst, regNumber src) +bool emitter::IsMovRedundantToPrevMov(instruction ins, regNumber dst, regNumber src) { assert(ins == INS_mov); @@ -14909,10 +14910,10 @@ bool emitter::IsOppositeOfPrevMov(instruction ins, regNumber dst, regNumber src) regNumber prevDst = emitLastIns->idReg1(); regNumber prevSrc = emitLastIns->idReg2(); - if ((prevDst == src) && (prevSrc == dst)) + if (((prevDst == src) && (prevSrc == dst)) || // opposite movs + (prevDst == dst) && (prevSrc == src)) // similar movs { - JITDUMP("\n -- suppressing mov as previous instruction did opposite mov.\n", dst, src, - this->emitComp->info.compMethodName, prevDst, prevSrc); + JITDUMP("\n -- suppressing mov as previous mov instruction was sufficient.\n"); return true; } } diff --git a/src/coreclr/src/jit/emitarm64.h b/src/coreclr/src/jit/emitarm64.h index c8a4e15c15c33..f509e189e08d7 100644 --- a/src/coreclr/src/jit/emitarm64.h +++ b/src/coreclr/src/jit/emitarm64.h @@ -48,8 +48,6 @@ void emitDispExtendReg(regNumber reg, insOpts opt, ssize_t imm); void emitDispAddrRI(regNumber reg, insOpts opt, ssize_t imm); void emitDispAddrRRExt(regNumber reg1, regNumber reg2, insOpts opt, bool isScaled, emitAttr size); -bool IsOppositeOfPrevMov(instruction ins, regNumber dst, regNumber src); - void emitDispIns(instrDesc* id, bool isNew, bool doffs, @@ -115,6 +113,10 @@ static UINT64 NOT_helper(UINT64 value, unsigned width); // A helper method to perform a bit Replicate operation static UINT64 Replicate_helper(UINT64 value, unsigned width, emitAttr size); +// Method to do peephole optimization by checking if mov is redundant to +// immediately preceding mov and if yes, omit current mov. +bool IsMovRedundantToPrevMov(instruction ins, regNumber dst, regNumber src); + /************************************************************************ * * This union is used to to encode/decode the special ARM64 immediate values From 172cb9d86cdd2ab54f4da59b22456102aecbcfd2 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 22 Jun 2020 15:35:53 -0700 Subject: [PATCH 03/13] Refactor code into IsRedundantMov() --- src/coreclr/src/jit/emitarm64.cpp | 84 ++++++++++++++++++++++--------- src/coreclr/src/jit/emitarm64.h | 6 +-- 2 files changed, 63 insertions(+), 27 deletions(-) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index 079260717147e..64c96477fa14c 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -4009,23 +4009,9 @@ void emitter::emitIns_R_R( { case INS_mov: assert(insOptsNone(opt)); - // Is the mov even necessary? - if (reg1 == reg2) - { - // A mov with a EA_4BYTE has the side-effect of clearing the upper bits - // So only eliminate mov instructions that are not clearing the upper bits - // - if (isGeneralRegisterOrSP(reg1) && (size == EA_8BYTE)) - { - return; - } - else if (isVectorRegister(reg1) && (size == EA_16BYTE)) - { - return; - } - } - if (IsMovRedundantToPrevMov(ins, reg1, reg2)) + // Is the mov even necessary? + if (emitComp->opts.OptimizationEnabled() && IsRedundantMov(ins, size, reg1, reg2)) { return; } @@ -14888,9 +14874,23 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins #endif // defined(DEBUG) || defined(LATE_DISASM) //---------------------------------------------------------------------------------------- -// IsMovRedundantToPrevMov: -// Check if previous instruction was a 'mov' where dst was current src and src was current dst. -// Or if previous `mov` moved between same src/dst. +// IsRedundantMov: +// Check if the current `mov` instruction is redundant and can be omitted. +// A `mov` is redundant in following 3 cases: +// +// 1. Move to same register +// +// mov Rx, Rx +// +// 2. Move that is identical to last instruction emitted. +// +// mov Rx, Rx # <-- last instruction +// mov Rx, Rx # <-- current instruction can be omitted. +// +// 3. Opposite Move as that of last instruction emitted. +// +// mov Rx, Ry # <-- last instruction +// mov Ry, Rx # <-- current instruction can be omitted. // // Arguments: // ins - The current instruction @@ -14900,23 +14900,59 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins // Return Value: // true if previous instruction moved from current dst to src. -bool emitter::IsMovRedundantToPrevMov(instruction ins, regNumber dst, regNumber src) +bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regNumber src) { + bool isRedundant = false; +#ifdef DEBUG + const char* reason = nullptr; +#endif // DEBUG + assert(ins == INS_mov); + if (dst == src) + { + // A mov with a EA_4BYTE has the side-effect of clearing the upper bits + // So only eliminate mov instructions that are not clearing the upper bits + // + if (isGeneralRegisterOrSP(dst) && (size == EA_8BYTE)) + { + reason = "\n -- suppressing mov because src and dst is same 8-byte register.\n"; + isRedundant = true; + } + else if (isVectorRegister(dst) && (size == EA_16BYTE)) + { + reason = "\n -- suppressing mov because src and dst is same 16-byte register.\n"; + isRedundant = true; + } + } + if (emitLastIns != nullptr && emitLastIns->idIns() == INS_mov) { // Check if we did same move in prev instruction except dst/src were switched. regNumber prevDst = emitLastIns->idReg1(); regNumber prevSrc = emitLastIns->idReg2(); - if (((prevDst == src) && (prevSrc == dst)) || // opposite movs - (prevDst == dst) && (prevSrc == src)) // similar movs + if ((prevDst == dst) && (prevSrc == src)) { - JITDUMP("\n -- suppressing mov as previous mov instruction was sufficient.\n"); + reason = "\n -- suppressing mov because previous instruction already moved from src to dst register.\n"; + isRedundant = true; + } + + // A mov with a EA_4BYTE has the side-effect of clearing the upper bits + // So only eliminate mov instructions that are not clearing the upper bits + // + if (((size == EA_8BYTE) || (size == EA_16BYTE)) && (prevDst == src) && (prevSrc == dst)) + { + JITDUMP("\n -- suppressing mov because previous instruction already did an opposite move from dst to src register.\n"); return true; } } - return false; + + if (isRedundant) + { + JITDUMP(reason); + } + + return isRedundant; } #endif // defined(TARGET_ARM64) diff --git a/src/coreclr/src/jit/emitarm64.h b/src/coreclr/src/jit/emitarm64.h index f509e189e08d7..614df95dee975 100644 --- a/src/coreclr/src/jit/emitarm64.h +++ b/src/coreclr/src/jit/emitarm64.h @@ -113,9 +113,9 @@ static UINT64 NOT_helper(UINT64 value, unsigned width); // A helper method to perform a bit Replicate operation static UINT64 Replicate_helper(UINT64 value, unsigned width, emitAttr size); -// Method to do peephole optimization by checking if mov is redundant to -// immediately preceding mov and if yes, omit current mov. -bool IsMovRedundantToPrevMov(instruction ins, regNumber dst, regNumber src); +// Method to do peephole optimization by checking if mov is redundant with +// respect to the last instruction. If yes, then will omit current mov instruction. +bool IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regNumber src); /************************************************************************ * From de0f7113b6a8316278c0f8655c7db14ce5de53fe Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 22 Jun 2020 18:22:10 -0700 Subject: [PATCH 04/13] dont optimize beyong IG boundary --- src/coreclr/src/jit/emitarm64.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index 64c96477fa14c..6143ef5c41fde 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -14926,7 +14926,8 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN } } - if (emitLastIns != nullptr && emitLastIns->idIns() == INS_mov) + // Don't look back across IG boundary or if the last instruction was not mov + if (emitCurIGinsCnt > 0 && emitLastIns != nullptr && emitLastIns->idIns() == INS_mov) { // Check if we did same move in prev instruction except dst/src were switched. regNumber prevDst = emitLastIns->idReg1(); From a9cbba24c9aca9489ac46c89b379d2757ffe9409 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 22 Jun 2020 23:49:43 -0700 Subject: [PATCH 05/13] add check to not cross IG boundary --- src/coreclr/src/jit/emitarm64.cpp | 27 +++++++++------------------ 1 file changed, 9 insertions(+), 18 deletions(-) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index 6143ef5c41fde..4b211eedbdc52 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -14902,11 +14902,6 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regNumber src) { - bool isRedundant = false; -#ifdef DEBUG - const char* reason = nullptr; -#endif // DEBUG - assert(ins == INS_mov); if (dst == src) @@ -14916,13 +14911,13 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN // if (isGeneralRegisterOrSP(dst) && (size == EA_8BYTE)) { - reason = "\n -- suppressing mov because src and dst is same 8-byte register.\n"; - isRedundant = true; + JITDUMP("\n -- suppressing mov because src and dst is same 8-byte register.\n"); + return true; } else if (isVectorRegister(dst) && (size == EA_16BYTE)) { - reason = "\n -- suppressing mov because src and dst is same 16-byte register.\n"; - isRedundant = true; + JITDUMP("\n -- suppressing mov because src and dst is same 16-byte register.\n"); + return true; } } @@ -14935,8 +14930,8 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN if ((prevDst == dst) && (prevSrc == src)) { - reason = "\n -- suppressing mov because previous instruction already moved from src to dst register.\n"; - isRedundant = true; + JITDUMP("\n -- suppressing mov because previous instruction already moved from src to dst register.\n"); + return true; } // A mov with a EA_4BYTE has the side-effect of clearing the upper bits @@ -14944,16 +14939,12 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN // if (((size == EA_8BYTE) || (size == EA_16BYTE)) && (prevDst == src) && (prevSrc == dst)) { - JITDUMP("\n -- suppressing mov because previous instruction already did an opposite move from dst to src register.\n"); + JITDUMP("\n -- suppressing mov because previous instruction already did an opposite move from dst to src " + "register.\n"); return true; } } - if (isRedundant) - { - JITDUMP(reason); - } - - return isRedundant; + return false; } #endif // defined(TARGET_ARM64) From 0d4e8e1c847e3c0e4a4d4b269da937e98af19646 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Wed, 24 Jun 2020 23:33:38 -0700 Subject: [PATCH 06/13] Add more checks: - Change IG boundary check to exclude extended IGs. - Check the operand size before removing redundant movs. --- src/coreclr/src/jit/emitarm64.cpp | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index 4b211eedbdc52..99ecdd2801e33 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -14884,8 +14884,8 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins // // 2. Move that is identical to last instruction emitted. // -// mov Rx, Rx # <-- last instruction -// mov Rx, Rx # <-- current instruction can be omitted. +// mov Rx, Ry # <-- last instruction +// mov Rx, Ry # <-- current instruction can be omitted. // // 3. Opposite Move as that of last instruction emitted. // @@ -14921,8 +14921,10 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN } } - // Don't look back across IG boundary or if the last instruction was not mov - if (emitCurIGinsCnt > 0 && emitLastIns != nullptr && emitLastIns->idIns() == INS_mov) + bool isFirstInstrInIG = (emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0); + + // Don't look back across IG boundary or if the last instruction was not mov + if (!isFirstInstrInIG && emitLastIns != nullptr && emitLastIns->idIns() == INS_mov) { // Check if we did same move in prev instruction except dst/src were switched. regNumber prevDst = emitLastIns->idReg1(); @@ -14930,14 +14932,14 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN if ((prevDst == dst) && (prevSrc == src)) { + assert(emitLastIns->idOpSize() == size); JITDUMP("\n -- suppressing mov because previous instruction already moved from src to dst register.\n"); return true; } - // A mov with a EA_4BYTE has the side-effect of clearing the upper bits - // So only eliminate mov instructions that are not clearing the upper bits - // - if (((size == EA_8BYTE) || (size == EA_16BYTE)) && (prevDst == src) && (prevSrc == dst)) + if (((size == EA_8BYTE) || (size == EA_16BYTE)) && // Only optimize instructions that don't clear upper bits + (emitLastIns->idOpSize() == size) && // Only optimize if operand size is same as previous instruction + (prevDst == src) && (prevSrc == dst)) // Only optimize if this is an opposite mov. { JITDUMP("\n -- suppressing mov because previous instruction already did an opposite move from dst to src " "register.\n"); From 18a68b8e393cdba89c7b65dc07f19ca267d3e8f1 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 26 Jun 2020 11:50:13 -0700 Subject: [PATCH 07/13] fix the check --- src/coreclr/src/jit/emitarm64.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index 99ecdd2801e33..a2538277bc81b 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -14921,10 +14921,12 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN } } - bool isFirstInstrInIG = (emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0); + bool isFirstInstrInIG = (emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0); - // Don't look back across IG boundary or if the last instruction was not mov - if (!isFirstInstrInIG && emitLastIns != nullptr && emitLastIns->idIns() == INS_mov) + if (!isFirstInstrInIG && // Don't optimize if instruction is not the first instruction in IG. + (emitLastIns != nullptr && + 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. { // Check if we did same move in prev instruction except dst/src were switched. regNumber prevDst = emitLastIns->idReg1(); @@ -14937,9 +14939,8 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN return true; } - if (((size == EA_8BYTE) || (size == EA_16BYTE)) && // Only optimize instructions that don't clear upper bits - (emitLastIns->idOpSize() == size) && // Only optimize if operand size is same as previous instruction - (prevDst == src) && (prevSrc == dst)) // Only optimize if this is an opposite mov. + if (((size == EA_8BYTE) || (size == EA_16BYTE)) && // Only optimize instructions that don't clear upper bits. + (prevDst == src) && (prevSrc == dst)) // Only optimize if this is an opposite mov. { JITDUMP("\n -- suppressing mov because previous instruction already did an opposite move from dst to src " "register.\n"); From f94951ee086d6e68123eac6441d1279755a5b645 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 26 Jun 2020 15:44:26 -0700 Subject: [PATCH 08/13] review comments --- src/coreclr/src/jit/emitarm64.cpp | 8 ++++---- src/coreclr/src/jit/emitarm64.h | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index a2538277bc81b..f4cbb7d5e0e03 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -14921,11 +14921,11 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN } } - bool isFirstInstrInIG = (emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0); + bool isFirstInstrInBlock = (emitCurIGinsCnt == 0) && ((emitCurIG->igFlags & IGF_EXTEND) == 0); - if (!isFirstInstrInIG && // Don't optimize if instruction is not the first instruction in IG. - (emitLastIns != nullptr && - emitLastIns->idIns() == INS_mov) && // Don't optimize if last instruction was not 'mov'. + if (!isFirstInstrInBlock && // Don't optimize if instruction is not the first instruction in IG. + (emitLastIns != nullptr) && + (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. { // Check if we did same move in prev instruction except dst/src were switched. diff --git a/src/coreclr/src/jit/emitarm64.h b/src/coreclr/src/jit/emitarm64.h index 614df95dee975..3656a04f2ef24 100644 --- a/src/coreclr/src/jit/emitarm64.h +++ b/src/coreclr/src/jit/emitarm64.h @@ -113,8 +113,8 @@ static UINT64 NOT_helper(UINT64 value, unsigned width); // A helper method to perform a bit Replicate operation static UINT64 Replicate_helper(UINT64 value, unsigned width, emitAttr size); -// Method to do peephole optimization by checking if mov is redundant with -// respect to the last instruction. If yes, then will omit current mov instruction. +// Method to do check if mov is redundant with respect to the last instruction. +// If yes, the caller of this method can choose to omit current mov instruction. bool IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regNumber src); /************************************************************************ From 1023b61700ad8f05804a20a46f66dcf57fc958e3 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 26 Jun 2020 16:04:44 -0700 Subject: [PATCH 09/13] Add check to ensure if src/dst are both scalar or both vector --- src/coreclr/src/jit/emitarm64.cpp | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index f4cbb7d5e0e03..463512ab4b459 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -14939,12 +14939,30 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN return true; } - if (((size == EA_8BYTE) || (size == EA_16BYTE)) && // Only optimize instructions that don't clear upper bits. - (prevDst == src) && (prevSrc == dst)) // Only optimize if this is an opposite mov. + if ((prevDst == src) && (prevSrc == dst)) { - JITDUMP("\n -- suppressing mov because previous instruction already did an opposite move from dst to src " - "register.\n"); - return true; + // For mov with EA_8BYTE, ensure src/dst are both scalar or both vector. + if (size == EA_8BYTE) + { + if (isVectorRegister(src) == isVectorRegister(dst)) + { + JITDUMP("\n -- suppressing mov because previous instruction already did an opposite move from dst " + "to src register.\n"); + return true; + } + } + + // For mov with EA_16BYTE, both src/dst will be vector. + else if (size == EA_16BYTE) + { + assert(isVectorRegister(src) && isVectorRegister(dst)); + + JITDUMP("\n -- suppressing mov because previous instruction already did an opposite move from dst to " + "src register.\n"); + return true; + } + + // For mov of other sizes, don't optimize because it has side-effect of clearing the upper bits. } } From 54dc37dab2b173684f7312391a78948950ce6f1a Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Fri, 26 Jun 2020 17:25:30 -0700 Subject: [PATCH 10/13] minor comment --- src/coreclr/src/jit/emitarm64.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index 463512ab4b459..b2a7facaf0a6c 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -14879,6 +14879,7 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins // A `mov` is redundant in following 3 cases: // // 1. Move to same register +// (Except 4-byte movement like "mov w1, w1" which zeros out upper bits of x1 register) // // mov Rx, Rx // From 1e650fdea4f5a74d70491cbeb3db0f033e4a2404 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 29 Jun 2020 12:00:20 -0700 Subject: [PATCH 11/13] Add check for insFormat --- src/coreclr/src/jit/emitarm64.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index b2a7facaf0a6c..b76e3e55f115a 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -14930,9 +14930,14 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN (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(); - regNumber prevSrc = emitLastIns->idReg2(); - + regNumber prevDst = emitLastIns->idReg1(); + regNumber prevSrc = emitLastIns->idReg2(); + insFormat lastInsfmt = emitLastIns->idInsFmt(); + + // Sometimes emitLastIns can be a mov with single register e.g. "mov reg, #imm". So ensure to + // optimize formats that does vector-to-vector or scalar-to-scalar register movs. + bool isValidLastInsFormats = ((lastInsfmt == IF_DV_3C) || (lastInsfmt == IF_DR_2G) || (lastInsfmt == IF_DR_2E)); + if ((prevDst == dst) && (prevSrc == src)) { assert(emitLastIns->idOpSize() == size); @@ -14940,7 +14945,7 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN return true; } - if ((prevDst == src) && (prevSrc == dst)) + if ((prevDst == src) && (prevSrc == dst) && isValidLastInsFormats) { // For mov with EA_8BYTE, ensure src/dst are both scalar or both vector. if (size == EA_8BYTE) @@ -14957,6 +14962,7 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN else if (size == EA_16BYTE) { assert(isVectorRegister(src) && isVectorRegister(dst)); + assert(lastInsfmt == IF_DR_3C); JITDUMP("\n -- suppressing mov because previous instruction already did an opposite move from dst to " "src register.\n"); From 412c4b8ac53dbeb8f540dfcf3efe484eef41c5c1 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 29 Jun 2020 17:55:47 -0700 Subject: [PATCH 12/13] fix the typo --- src/coreclr/src/jit/emitarm64.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index b76e3e55f115a..106ae003e9907 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -14962,7 +14962,7 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN else if (size == EA_16BYTE) { assert(isVectorRegister(src) && isVectorRegister(dst)); - assert(lastInsfmt == IF_DR_3C); + assert(lastInsfmt == IF_DV_3C); JITDUMP("\n -- suppressing mov because previous instruction already did an opposite move from dst to " "src register.\n"); From b5882c536a8c284aa94f352bb4125131c140893e Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 29 Jun 2020 23:21:10 -0700 Subject: [PATCH 13/13] minor comments --- src/coreclr/src/jit/emitarm64.cpp | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/coreclr/src/jit/emitarm64.cpp b/src/coreclr/src/jit/emitarm64.cpp index 106ae003e9907..df9a1b54cd826 100644 --- a/src/coreclr/src/jit/emitarm64.cpp +++ b/src/coreclr/src/jit/emitarm64.cpp @@ -14894,9 +14894,10 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins // mov Ry, Rx # <-- current instruction can be omitted. // // Arguments: -// ins - The current instruction -// dst - The current destination -// src - The current source +// ins - The current instruction +// size - Operand size of current instruction +// dst - The current destination +// src - The current source // // Return Value: // true if previous instruction moved from current dst to src. @@ -14934,10 +14935,6 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN regNumber prevSrc = emitLastIns->idReg2(); insFormat lastInsfmt = emitLastIns->idInsFmt(); - // Sometimes emitLastIns can be a mov with single register e.g. "mov reg, #imm". So ensure to - // optimize formats that does vector-to-vector or scalar-to-scalar register movs. - bool isValidLastInsFormats = ((lastInsfmt == IF_DV_3C) || (lastInsfmt == IF_DR_2G) || (lastInsfmt == IF_DR_2E)); - if ((prevDst == dst) && (prevSrc == src)) { assert(emitLastIns->idOpSize() == size); @@ -14945,6 +14942,10 @@ bool emitter::IsRedundantMov(instruction ins, emitAttr size, regNumber dst, regN return true; } + // Sometimes emitLastIns can be a mov with single register e.g. "mov reg, #imm". So ensure to + // optimize formats that does vector-to-vector or scalar-to-scalar register movs. + bool isValidLastInsFormats = ((lastInsfmt == IF_DV_3C) || (lastInsfmt == IF_DR_2G) || (lastInsfmt == IF_DR_2E)); + if ((prevDst == src) && (prevSrc == dst) && isValidLastInsFormats) { // For mov with EA_8BYTE, ensure src/dst are both scalar or both vector.