From cea9280acfc5561541b7516da235459e6fddee4c Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Wed, 11 May 2022 22:06:14 +0100 Subject: [PATCH 01/22] resolve merge conflict --- src/coreclr/jit/codegen.h | 3 +- src/coreclr/jit/codegencommon.cpp | 132 ++++++++++++ src/coreclr/jit/codegeninterface.h | 1 + src/coreclr/jit/codegenlinear.cpp | 2 +- src/coreclr/jit/emit.cpp | 316 +++++++++++++++++++++++++++++ src/coreclr/jit/emit.h | 17 +- src/coreclr/jit/emitarm.cpp | 2 +- src/coreclr/jit/emitarm64.cpp | 2 +- src/coreclr/jit/emitpub.h | 2 +- src/coreclr/jit/emitxarch.cpp | 11 +- src/coreclr/jit/instr.cpp | 4 +- 11 files changed, 482 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index d567ef1537a37..6fae61b5ec937 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -26,6 +26,7 @@ class CodeGen final : public CodeGenInterface virtual void genGenerateCode(void** codePtr, uint32_t* nativeSizeOfCode); void genGenerateMachineCode(); + void genUpdateLiveRangesForTruncatedIGs(); void genEmitMachineCode(); void genEmitUnwindDebugGCandEH(); @@ -1473,7 +1474,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX public: void instGen(instruction ins); - void inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock); + void inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock, bool isJmpAlways = false); void inst_SET(emitJumpKind condition, regNumber reg); diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index b755578022986..6070418fce06f 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1943,6 +1943,13 @@ void CodeGen::genGenerateMachineCode() } #endif // DEBUG + // after code generation but before eh funclet generation check to see if any jumps can be removed + if (GetEmitter()->emitRemoveJumpToNextInst()) + { + // if jumps were removed the live ranges need to be updated with the new instruction group sizes + genUpdateLiveRangesForTruncatedIGs(); + } + /* We can now generate the function prolog and epilog */ genGeneratePrologsAndEpilogs(); @@ -1960,6 +1967,114 @@ void CodeGen::genGenerateMachineCode() /* The code is now complete and final; it should not change after this. */ } + +void CodeGen::genUpdateLiveRangesForTruncatedIGs() +{ +#ifdef DEBUG + if (compiler->verbose) + { + printf("*************** In genUpdateLiveRangesForTruncatedIGs()\n"); + } +#endif + bool updatedGroup = false; + + if (compiler->lvaCount > 0) + { + insGroup* ig = GetEmitter()->emitIGlist; + while (ig != nullptr) + { + if (ig->igFlags & IGF_UPD_ICOUNT) + { + + unsigned int liveVarCount = varLiveKeeper->getVarCount(); + if (liveVarCount > 0) + { +#ifdef DEBUG + if (compiler->verbose) + { + JITDUMP("IG%02u is marked with IGF_UPD_ICOUNT and has %d live variables\n", ig->igNum, + liveVarCount); + } +#endif + for (unsigned int varNum = 0; varNum < liveVarCount; varNum++) + { + if (compiler->compMap2ILvarNum(varNum) == (unsigned int)ICorDebugInfo::UNKNOWN_ILNUM) + { + continue; + } + + for (int rangeIndex = 0; rangeIndex < 2; rangeIndex++) + { + VariableLiveKeeper::LiveRangeList* liveRanges; + if (rangeIndex == 0) + { + liveRanges = varLiveKeeper->getLiveRangesForVarForProlog(varNum); + } + else + { + liveRanges = varLiveKeeper->getLiveRangesForVarForBody(varNum); + } + + for (VariableLiveKeeper::VariableLiveRange& liveRange : *liveRanges) + { + if (liveRange.m_StartEmitLocation.GetIG()->igNum == ig->igNum && + liveRange.m_StartEmitLocation.GetInsNum() > ig->igInsCnt) + { +#ifdef DEBUG + if (compiler->verbose) + { + JITDUMP("IG%02u varNum %d StartEmitLocation InsCnt changed from %d to %d\n", ig, + liveVarCount, liveRange.m_StartEmitLocation.GetInsNum(), ig->igInsCnt); + } +#endif + liveRange.m_StartEmitLocation.SetInsNum(ig->igInsCnt); + assert(liveRange.m_StartEmitLocation.GetInsNum() == ig->igInsCnt); + updatedGroup = true; + } + + if (liveRange.m_EndEmitLocation.GetIG()->igNum == ig->igNum && + liveRange.m_EndEmitLocation.GetInsNum() > ig->igInsCnt) + { + liveRange.m_EndEmitLocation.SetInsNum(ig->igInsCnt); +#ifdef DEBUG + if (compiler->verbose) + { + JITDUMP("IG%02u varNum %d EndEmitLocation InsCnt changed from %d to %d\n", ig, + liveVarCount, liveRange.m_StartEmitLocation.GetInsNum(), ig->igInsCnt); + } +#endif + assert(liveRange.m_EndEmitLocation.GetInsNum() == ig->igInsCnt); + updatedGroup = true; + } + } + } + } + } + else + { +#ifdef DEBUG + if (compiler->verbose) + { + JITDUMP("IG%02u is marked with IGF_UPD_ICOUNT but has no live variables\n", ig); + } +#endif + } + + ig->igFlags ^= IGF_UPD_ICOUNT; + } + + ig = ig->igNext; + } + } +#ifdef DEBUG + if (updatedGroup && verbose) + { + printf("\nlvaTable after genUpdateLiveRangesForTruncatedIGs\n"); + compiler->lvaTableDump(); + } +#endif +} + //---------------------------------------------------------------------- // genEmitMachineCode -- emit the actual machine instruction code // @@ -9199,6 +9314,23 @@ CodeGenInterface::VariableLiveKeeper::LiveRangeList* CodeGenInterface::VariableL return m_vlrLiveDscForProlog[varNum].getLiveRanges(); } +unsigned int CodeGenInterface::VariableLiveKeeper::getVarCount() const +{ + unsigned int count = 0; + + if (m_Compiler->opts.compDbgInfo) + { + for (unsigned int varNum = 0; varNum < m_LiveDscCount; varNum++) + { + if (m_Compiler->compMap2ILvarNum(varNum) != (unsigned int)ICorDebugInfo::UNKNOWN_ILNUM) + { + count += 1; + } + } + } + return count; +} + //------------------------------------------------------------------------ // getLiveRangesCount: Returns the count of variable locations reported for the tracked // variables, which are arguments, special arguments, and local IL variables. diff --git a/src/coreclr/jit/codegeninterface.h b/src/coreclr/jit/codegeninterface.h index 17bdcb1952f67..20fdea1ba9c03 100644 --- a/src/coreclr/jit/codegeninterface.h +++ b/src/coreclr/jit/codegeninterface.h @@ -744,6 +744,7 @@ class CodeGenInterface LiveRangeList* getLiveRangesForVarForBody(unsigned int varNum) const; LiveRangeList* getLiveRangesForVarForProlog(unsigned int varNum) const; size_t getLiveRangesCount() const; + unsigned int getVarCount() const; // For parameters locations on prolog void psiStartVariableLiveRange(CodeGenInterface::siVarLoc varLocation, unsigned int varNum); diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 6212a529fa471..7ba5a490852de 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -753,7 +753,7 @@ void CodeGen::genCodeForBBlist() break; case BBJ_ALWAYS: - inst_JMP(EJ_jmp, block->bbJumpDest); + inst_JMP(EJ_jmp, block->bbJumpDest, false); FALLTHROUGH; case BBJ_COND: diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 9516058fdfc76..46abe03494456 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -50,6 +50,11 @@ int emitLocation::GetInsNum() const return emitGetInsNumFromCodePos(codePos); } +void emitLocation::SetInsNum(int insNum) +{ + codePos = emitGetInsNumFromCodePos(insNum) + (emitGetInsOfsFromCodePos(codePos) << 16); +} + // Get the instruction offset in the current instruction group, which must be a funclet prolog group. // This is used to find an instruction offset used in unwind data. // TODO-AMD64-Bug?: We only support a single main function prolog group, but allow for multiple funclet prolog @@ -3864,6 +3869,21 @@ void emitter::emitDispGCinfo() printf("\n\n"); } +void emitter::emitDispJumpList() +{ + printf("Emitter Jump List:\n"); + unsigned int jmpCount = 0; + instrDescJmp* jmp = emitJumpList; + instrDescJmp* previousJmp = nullptr; + while (jmp) + { + emitDispIns(jmp, false, false, false, 0, nullptr, 0, jmp->idjIG); + jmpCount += 1; + jmp = jmp->idjNext; + } + printf(" total jump count: %u\n", jmpCount); +} + #endif // DEBUG /***************************************************************************** @@ -4102,6 +4122,298 @@ void emitter::emitDispCommentForHandle(size_t handle, GenTreeFlags flag) #endif // DEBUG } +/***************************************************************************** + * Checks all jumps in the jump list to see if they are unconditional jumps + * generated at the end of BB_ALWAYS basic blocks and whether they are now a jump + * to the next instruction, this can happen when the following basic block emitted + * no instructions + * + * returns true if any jumps have been removed, otherwise returns false + */ +bool emitter::emitRemoveJumpToNextInst() +{ +#ifdef TARGET_XARCH +#ifdef DEBUG + if (emitComp->verbose) + { + printf("*************** In emitRemoveJumpToNextInst()\n"); + } + if (EMIT_INSTLIST_VERBOSE) + { + printf("\nInstruction list before unconditional jump to next instruction removal:\n\n"); + emitDispIGlist(true); + } + if (EMITVERBOSE) + { + emitDispJumpList(); + } +#endif + + instrDescJmp* jmp; + UNATIVE_OFFSET adjIG; + UNATIVE_OFFSET adjLJ; + insGroup* lstIG; + int removedCount = 0; + int jmp_removal = 1; + bool jmpRemoved = false; + + /*****************************************************************************/ + /* If we iterate to look for more jumps to shorten, we start again here. */ + /*****************************************************************************/ + +AGAIN: + + lstIG = nullptr; + adjLJ = 0; + adjIG = 0; + // try to remove unconditional jumps at the end of a group to the next group + + jmp = emitJumpList; + instrDescJmp* previousJmp = nullptr; + while (jmp) + { + // if the jump is unconditional then it is a candidate + if (jmp->idInsFmt() == IF_LABEL && emitIsUncondJump(jmp) && !jmp->idjKeepLong && jmp->idGetJmpAlwaysFlag()) + { + // unset the flag so that we know it has been processed already + jmp->idSetJmpAlwaysFlag(0); + + // target group is not bound yet so use the cookie to fetch it + insGroup* targetGroup = (insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel); + + // if the target group is the next instruction group + if (targetGroup != nullptr && jmp->idjIG->igNext == targetGroup) + { + insGroup* group = jmp->idjIG; + unsigned instructionCount = group->igInsCnt; + + if (instructionCount > 0) + { + BYTE* dataPtr = group->igData; + instrDesc* instructionDescriptor = nullptr; + + do + { + instructionDescriptor = (instrDesc*)dataPtr; + dataPtr += emitSizeOfInsDsc(instructionDescriptor); + } while (--instructionCount); + + assert(instructionDescriptor != nullptr); + // instructionDescriptor is now the last instruction in the group + + if (instructionDescriptor->idIns() == jmp->idIns() && jmp == instructionDescriptor) + { + CLANG_FORMAT_COMMENT_ANCHOR + // the last instruction in the group is the jmp we're looking for + // and it jumps to the next instruction group so we don't really need it + +#ifdef DEBUG + if (EMITVERBOSE) + { + printf("Removing unconditional jump [%08X/%03u] from the last instruction in IG%02u to " + "the next IG IG%02u label %s\n", + dspPtr(jmp), jmp->idDebugOnlyInfo()->idNum, group->igNum, targetGroup->igNum, + emitLabelString(targetGroup)); + } +#endif + UNATIVE_OFFSET sizeRemoved = instructionDescriptor->idCodeSize(); + + // set state needed at the end of the loop in ADJUST_GROUP + adjIG = sizeRemoved; + lstIG = group; + jmpRemoved = true; + + // unlink the jump + if (previousJmp != nullptr) + { + previousJmp->idjNext = jmp->idjNext; + // if it's the last jump then set it to the previous + if (jmp == emitJumpLast) + { + emitJumpLast = previousJmp; + } + } + else + { + assert(jmp == emitJumpList); + emitJumpList = jmp->idjNext; + } + jmp->idjNext = nullptr; + + + +#if DEBUG + // clear the instruction data + memset((BYTE*)instructionDescriptor, 0, instructionDescriptor->idCodeSize()); +#endif + + // remove the instruction from the group + group->igInsCnt -= 1; + group->igSize -= sizeRemoved; + emitTotalCodeSize -= sizeRemoved; + + // flag the group as resized and recounted + group->igFlags |= (IGF_UPD_ISZ | IGF_UPD_ICOUNT); + + //emitRemoveLabelOnlyUsedBy(jmp); + + // cleanup + instructionDescriptor = nullptr; + jmp = nullptr; + removedCount += 1; + + // jump to adjusting the size + goto ADJUST_GROUP; + } + } + } + } + + previousJmp = jmp; + if (jmp != nullptr) + { + jmp = jmp->idjNext; + } + } + + jmp = nullptr; + lstIG = nullptr; + adjLJ = 0; + adjIG = 0; + +ADJUST_GROUP: + + /* Did we adjust any jumps? */ + if (adjIG) + { + /* Adjust offsets of any remaining blocks */ + + assert(lstIG); + + for (;;) + { + lstIG = lstIG->igNext; + if (!lstIG) + { + break; + } +#ifdef DEBUG + if (EMITVERBOSE) + { + printf("Adjusted offset of " FMT_BB " from %04X to %04X\n", lstIG->igNum, lstIG->igOffs, + lstIG->igOffs - adjIG); + } +#endif + lstIG->igOffs -= adjIG; + assert(IsCodeAligned(lstIG->igOffs)); + } + +#ifdef DEBUG + if (EMITVERBOSE) + { + printf("Total shrinkage = %3u\n", adjIG); + } +#endif + + if (jmpRemoved) + { + jmp_removal++; +#ifdef DEBUG + if (EMITVERBOSE) + { + printf("Iterating branch removal. Iteration = %d\n", jmp_removal); + } +#endif + jmpRemoved = false; + goto AGAIN; + } + } +#ifdef DEBUG + if (EMIT_INSTLIST_VERBOSE) + { + printf("\nLabels list after unconditional jump to next instruction removal:\n\n"); + emitDispIGlist(false); + } + if (EMITVERBOSE) + { + emitDispJumpList(); + } +#endif + if (emitComp->verbose) + { + printf("emitRemoveJumpToNextInst removed %d jumps\n", removedCount); + } + return removedCount > 0; +#else + return 0; +#endif // TARGET_XARCH +} + +void emitter::emitRemoveLabelOnlyUsedBy(instrDescJmp* jmpFrom) +{ + int useCount = 0; + insGroup* jmpFromTargetGroup = (insGroup*)emitCodeGetCookie(jmpFrom->idAddr()->iiaBBlabel); + + { + instrDescJmp* jmp = emitJumpList; + while (jmp) + { + insGroup* jmpTargetGroup = ((insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel)); + if (jmp != jmpFrom && jmpTargetGroup == jmpFromTargetGroup) + { + useCount += 1; + } + jmp = jmp->idjNext; + } + jmp = nullptr; + } + + if (useCount == 0) + { +#if DEBUG + if (emitComp->verbose) + { + printf("found unused label %s\n", emitLabelString(jmpFromTargetGroup)); + } +#endif + + + if (jmpFromTargetGroup->igInsCnt > 0) + { + assert(jmpFrom->idAddr()->iiaBBlabel !=nullptr); + jmpFrom->idAddr()->iiaBBlabel = nullptr; + + instrDesc* firstTargtInstruction = (instrDesc*)jmpFromTargetGroup->igData; + if ((firstTargtInstruction->idInsFmt() & IF_LABEL) != 0) + { + firstTargtInstruction->idInsFmt((insFormat)(firstTargtInstruction->idInsFmt() ^ IF_LABEL)); + +#if DEBUG + if (emitComp->verbose) + { + printf("removed IF_LABEL from "); + emitDispIns(firstTargtInstruction, false, false, false); + printf(" in "); + emitLabelString(jmpFromTargetGroup); + printf("\n"); + } +#endif + } + + assert((firstTargtInstruction->idInsFmt() & IF_LABEL) == 0); + } + else + { +#if DEBUG + if (emitComp->verbose) + { + printf("found %d uses of label %s\n",useCount, emitLabelString(jmpFromTargetGroup)); + } +#endif + } + } +} + /***************************************************************************** * Bind targets of relative jumps to choose the smallest possible encoding. * X86 and AMD64 have a small and large encoding. @@ -4126,6 +4438,10 @@ void emitter::emitJumpDistBind() printf("\nInstruction list before jump distance binding:\n\n"); emitDispIGlist(true); } + if (EMITVERBOSE) + { + emitDispJumpList(); + } #endif instrDescJmp* jmp; diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index b93fded455f32..3699cd109f928 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -160,6 +160,7 @@ class emitLocation } int GetInsNum() const; + void SetInsNum(int insNum); bool operator!=(const emitLocation& other) const { @@ -279,6 +280,7 @@ struct insGroup // IG, or, if this IG contains with an unconditional branch, some subsequent IG. #define IGF_REMOVED_ALIGN 0x0800 // IG was marked as having an alignment instruction(s), but was later unmarked // without updating the IG's size/offsets. +#define IGF_UPD_ICOUNT 0x1000 // Mask of IGF_* flags that should be propagated to new blocks when they are created. // This allows prologs and epilogs to be any number of IGs, but still be @@ -670,7 +672,14 @@ class emitter _idCnsReloc = (EA_IS_CNS_RELOC(attr) ? 1 : 0); _idDspReloc = (EA_IS_DSP_RELOC(attr) ? 1 : 0); } - + void idSetJmpAlwaysFlag(bool value) + { + _idIsJmpAlways = value ? 1 : 0; + } + bool idGetJmpAlwaysFlag() + { + return _idIsJmpAlways == 1; + } //////////////////////////////////////////////////////////////////////// // Space taken up to here: // x86: 17 bits @@ -775,8 +784,9 @@ class emitter unsigned _idCnsReloc : 1; // LargeCns is an RVA and needs reloc tag unsigned _idDspReloc : 1; // LargeDsp is an RVA and needs reloc tag + unsigned _idIsJmpAlways : 1; -#define ID_EXTRA_RELOC_BITS (2) +#define ID_EXTRA_RELOC_BITS (3) //////////////////////////////////////////////////////////////////////// // Space taken up to here: @@ -1723,6 +1733,7 @@ class emitter void emitDispIG(insGroup* ig, insGroup* igPrev = nullptr, bool verbose = false); void emitDispIGlist(bool verbose = false); void emitDispGCinfo(); + void emitDispJumpList(); void emitDispClsVar(CORINFO_FIELD_HANDLE fldHnd, ssize_t offs, bool reloc = false); void emitDispFrameRef(int varx, int disp, int offs, bool asmfm); void emitDispInsAddr(BYTE* code); @@ -1998,6 +2009,8 @@ class emitter instrDescJmp* emitJumpList; // list of local jumps in method instrDescJmp* emitJumpLast; // last of local jumps in method void emitJumpDistBind(); // Bind all the local jumps in method + bool emitRemoveJumpToNextInst(); + void emitRemoveLabelOnlyUsedBy(instrDescJmp* jmpFrom); #if FEATURE_LOOP_ALIGN instrDescAlign* emitCurIGAlignList; // list of align instructions in current IG diff --git a/src/coreclr/jit/emitarm.cpp b/src/coreclr/jit/emitarm.cpp index ccf3ddff35709..4d2c872d21300 100644 --- a/src/coreclr/jit/emitarm.cpp +++ b/src/coreclr/jit/emitarm.cpp @@ -4334,7 +4334,7 @@ void emitter::emitSetMediumJump(instrDescJmp* id) * branch. Thus, we can handle branch offsets of imm24 instead of just imm20. */ -void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0 */) +void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0 */, bool isJmpAlways /* = false */) { insFormat fmt = IF_NONE; diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index 864ba862edddd..6eaaedee5ce7d 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -8402,7 +8402,7 @@ void emitter::emitIns_J_R_I(instruction ins, emitAttr attr, BasicBlock* dst, reg appendToCurIG(id); } -void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount) +void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount, bool isJmpAlways) { insFormat fmt = IF_NONE; diff --git a/src/coreclr/jit/emitpub.h b/src/coreclr/jit/emitpub.h index 02ab3bb879d6f..cd7d756f1c3dc 100644 --- a/src/coreclr/jit/emitpub.h +++ b/src/coreclr/jit/emitpub.h @@ -76,7 +76,7 @@ const char* emitOffsetToLabel(unsigned offs); /* Output target-independent instructions */ /************************************************************************/ -void emitIns_J(instruction ins, BasicBlock* dst, int instrCount = 0); +void emitIns_J(instruction ins, BasicBlock* dst, int instrCount = 0, bool isJmpAlways = false); /************************************************************************/ /* Emit initialized data sections */ diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index a2df15491d42e..7701246c268b7 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -7464,7 +7464,7 @@ void emitter::emitSetShortJump(instrDescJmp* id) * to jump: positive is forward, negative is backward. */ -void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0 */) +void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0 */, bool isJmpAlways /* = false */) { UNATIVE_OFFSET sz; instrDescJmp* id = emitNewInstrJmp(); @@ -7493,6 +7493,8 @@ void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0 } #endif // DEBUG + id->idSetJmpAlwaysFlag(isJmpAlways); + id->idjShort = 0; if (dst != nullptr) { @@ -13671,6 +13673,13 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) case IF_RWR_LABEL: case IF_SWR_LABEL: assert(id->idGCref() == GCT_NONE); + if (!id->idIsBound()) + { + printf("instruction has IF_LABEL flag "); + emitDispIns(id,false,false,false); + printf("but should not have\n"); + assert(0); + } assert(id->idIsBound()); // TODO-XArch-Cleanup: handle IF_RWR_LABEL in emitOutputLJ() or change it to emitOutputAM()? diff --git a/src/coreclr/jit/instr.cpp b/src/coreclr/jit/instr.cpp index 67ae437f03b75..3e76cefce5118 100644 --- a/src/coreclr/jit/instr.cpp +++ b/src/coreclr/jit/instr.cpp @@ -261,7 +261,7 @@ bool CodeGenInterface::instIsFP(instruction ins) * Generate a jump instruction. */ -void CodeGen::inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock) +void CodeGen::inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock, bool isJmpAlways) { #if !FEATURE_FIXED_OUT_ARGS // On the x86 we are pushing (and changing the stack level), but on x64 and other archs we have @@ -279,7 +279,7 @@ void CodeGen::inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock) #endif #endif // !FEATURE_FIXED_OUT_ARGS - GetEmitter()->emitIns_J(emitter::emitJumpKindToIns(jmp), tgtBlock); + GetEmitter()->emitIns_J(emitter::emitJumpKindToIns(jmp), tgtBlock, 0, isJmpAlways); } /***************************************************************************** From f0941511105a6d63ff02fa676b273ae1af8dc065 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Sat, 7 May 2022 12:40:10 +0100 Subject: [PATCH 02/22] add checks for post-call jmp removal --- src/coreclr/jit/codegencommon.cpp | 18 +- src/coreclr/jit/codegeninterface.h | 4 +- src/coreclr/jit/codegenlinear.cpp | 2 +- src/coreclr/jit/emit.cpp | 282 +++++++++++++++-------------- src/coreclr/jit/emit.h | 11 +- src/coreclr/jit/emitxarch.cpp | 2 +- 6 files changed, 168 insertions(+), 151 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 6070418fce06f..43c386cea1b4a 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1946,8 +1946,8 @@ void CodeGen::genGenerateMachineCode() // after code generation but before eh funclet generation check to see if any jumps can be removed if (GetEmitter()->emitRemoveJumpToNextInst()) { - // if jumps were removed the live ranges need to be updated with the new instruction group sizes - genUpdateLiveRangesForTruncatedIGs(); + // if jumps were removed the live ranges need to be updated with the new instruction group sizes + genUpdateLiveRangesForTruncatedIGs(); } /* We can now generate the function prolog and epilog */ @@ -1967,7 +1967,10 @@ void CodeGen::genGenerateMachineCode() /* The code is now complete and final; it should not change after this. */ } - +//---------------------------------------------------------------------- +// genUpdateLiveRangesForTruncatedIGs -- find any instruction groups that have +// had their count altered and updated live ranges that refer to those groups +// void CodeGen::genUpdateLiveRangesForTruncatedIGs() { #ifdef DEBUG @@ -2050,16 +2053,15 @@ void CodeGen::genUpdateLiveRangesForTruncatedIGs() } } } +#ifdef DEBUG else { -#ifdef DEBUG if (compiler->verbose) { - JITDUMP("IG%02u is marked with IGF_UPD_ICOUNT but has no live variables\n", ig); + JITDUMP("IG%02u is marked with IGF_UPD_ICOUNT and has no live variables\n", ig); } -#endif } - +#endif ig->igFlags ^= IGF_UPD_ICOUNT; } @@ -9325,7 +9327,7 @@ unsigned int CodeGenInterface::VariableLiveKeeper::getVarCount() const if (m_Compiler->compMap2ILvarNum(varNum) != (unsigned int)ICorDebugInfo::UNKNOWN_ILNUM) { count += 1; - } + } } } return count; diff --git a/src/coreclr/jit/codegeninterface.h b/src/coreclr/jit/codegeninterface.h index 20fdea1ba9c03..d3dbd61a1bb90 100644 --- a/src/coreclr/jit/codegeninterface.h +++ b/src/coreclr/jit/codegeninterface.h @@ -743,8 +743,8 @@ class CodeGenInterface LiveRangeList* getLiveRangesForVarForBody(unsigned int varNum) const; LiveRangeList* getLiveRangesForVarForProlog(unsigned int varNum) const; - size_t getLiveRangesCount() const; - unsigned int getVarCount() const; + size_t getLiveRangesCount() const; + unsigned int getVarCount() const; // For parameters locations on prolog void psiStartVariableLiveRange(CodeGenInterface::siVarLoc varLocation, unsigned int varNum); diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 7ba5a490852de..91942613f4e6b 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -753,7 +753,7 @@ void CodeGen::genCodeForBBlist() break; case BBJ_ALWAYS: - inst_JMP(EJ_jmp, block->bbJumpDest, false); + inst_JMP(EJ_jmp, block->bbJumpDest, /* isJmpAlways */ true); FALLTHROUGH; case BBJ_COND: diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 46abe03494456..9acc9ecb3abc3 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -3872,7 +3872,7 @@ void emitter::emitDispGCinfo() void emitter::emitDispJumpList() { printf("Emitter Jump List:\n"); - unsigned int jmpCount = 0; + unsigned int jmpCount = 0; instrDescJmp* jmp = emitJumpList; instrDescJmp* previousJmp = nullptr; while (jmp) @@ -4153,23 +4153,23 @@ bool emitter::emitRemoveJumpToNextInst() UNATIVE_OFFSET adjIG; UNATIVE_OFFSET adjLJ; insGroup* lstIG; - int removedCount = 0; - int jmp_removal = 1; - bool jmpRemoved = false; + int removedCount = 0; + UNATIVE_OFFSET removedSize = 0; + int iterationCount = 1; + bool jmpRemoved = false; - /*****************************************************************************/ - /* If we iterate to look for more jumps to shorten, we start again here. */ - /*****************************************************************************/ +/*****************************************************************************/ +/* If we iterate to look for more jumps to remove, we start again here. */ +/*****************************************************************************/ AGAIN: - lstIG = nullptr; - adjLJ = 0; - adjIG = 0; - // try to remove unconditional jumps at the end of a group to the next group - + lstIG = nullptr; + adjLJ = 0; + adjIG = 0; jmp = emitJumpList; instrDescJmp* previousJmp = nullptr; + while (jmp) { // if the jump is unconditional then it is a candidate @@ -4181,7 +4181,6 @@ bool emitter::emitRemoveJumpToNextInst() // target group is not bound yet so use the cookie to fetch it insGroup* targetGroup = (insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel); - // if the target group is the next instruction group if (targetGroup != nullptr && jmp->idjIG->igNext == targetGroup) { insGroup* group = jmp->idjIG; @@ -4189,81 +4188,162 @@ bool emitter::emitRemoveJumpToNextInst() if (instructionCount > 0) { - BYTE* dataPtr = group->igData; - instrDesc* instructionDescriptor = nullptr; + BYTE* dataPtr = group->igData; + instrDesc* instructionDescriptor = nullptr; + bool previousInstructionWasCall = false; + bool checkedPreviousInstruction = false; - do + // unroll first iteration to avoid conditional check in the loop + instructionDescriptor = (instrDesc*)dataPtr; + dataPtr += emitSizeOfInsDsc(instructionDescriptor); + instructionCount -= 1; + + if (instructionCount > 0) + { + do + { + previousInstructionWasCall = instructionDescriptor->idIns() == INS_call; + instructionDescriptor = (instrDesc*)dataPtr; + dataPtr += emitSizeOfInsDsc(instructionDescriptor); + instructionCount -= 1; + } while (instructionCount > 0); + checkedPreviousInstruction = true; + } + else { - instructionDescriptor = (instrDesc*)dataPtr; - dataPtr += emitSizeOfInsDsc(instructionDescriptor); - } while (--instructionCount); + // Special case. If the jmp is both the first and the last instruction in the + // group then we might not be able to remove the jmp because the previous + // instruction group ended in a call and assumed that the following group + // would contain the instruction needed following it (see code in genCodeForBBlist()) + // + // To avoid this find the previous IG and scan through it to find the last instruction + // and identify if it a call. this will be rare so the additional cost should not be + // high overall + + insGroup* previousGroup = nullptr; + insGroup* candidateGroup = emitIGlist; + while (candidateGroup) + { + if (candidateGroup->igNext == group) + { + previousGroup = candidateGroup; + break; + } + candidateGroup = candidateGroup->igNext; + }; + + assert(previousGroup); + + if (previousGroup->igInsCnt > 0) + { + instrDesc* pgInstructionDescriptor = nullptr; + int pgInstructionCount = previousGroup->igInsCnt; + BYTE* pgDataPtr = previousGroup->igData; + do + { + pgInstructionDescriptor = (instrDesc*)pgDataPtr; + pgDataPtr += emitSizeOfInsDsc(pgInstructionDescriptor); + pgInstructionCount -= 1; + } while (pgInstructionCount > 0); + previousInstructionWasCall = (pgInstructionDescriptor->idIns() == INS_call); + checkedPreviousInstruction = true; + } +#ifdef DEBUG + else + { + if (EMITVERBOSE) + { + printf("Unconditional jump [%08X/%03u] from the last instruction in " + "IG%02u to the next IG IG%02u label %s cannot be removed because it could not " + "be determined if the previous instruction was a call\n", + dspPtr(jmp), jmp->idDebugOnlyInfo()->idNum, group->igNum, targetGroup->igNum, + emitLabelString(targetGroup)); + } + } +#endif // DEBUG + } assert(instructionDescriptor != nullptr); // instructionDescriptor is now the last instruction in the group - if (instructionDescriptor->idIns() == jmp->idIns() && jmp == instructionDescriptor) + if (checkedPreviousInstruction && instructionDescriptor->idIns() == jmp->idIns() && + jmp == instructionDescriptor) { CLANG_FORMAT_COMMENT_ANCHOR // the last instruction in the group is the jmp we're looking for // and it jumps to the next instruction group so we don't really need it -#ifdef DEBUG - if (EMITVERBOSE) + if (!previousInstructionWasCall) { - printf("Removing unconditional jump [%08X/%03u] from the last instruction in IG%02u to " - "the next IG IG%02u label %s\n", - dspPtr(jmp), jmp->idDebugOnlyInfo()->idNum, group->igNum, targetGroup->igNum, - emitLabelString(targetGroup)); - } +#ifdef DEBUG + if (EMITVERBOSE) + { + printf("Removing unconditional jump [%08X/%03u] from the last instruction in " + "IG%02u to the next IG IG%02u label %s\n", + dspPtr(jmp), jmp->idDebugOnlyInfo()->idNum, group->igNum, targetGroup->igNum, + emitLabelString(targetGroup)); + } #endif - UNATIVE_OFFSET sizeRemoved = instructionDescriptor->idCodeSize(); + UNATIVE_OFFSET sizeRemoved = instructionDescriptor->idCodeSize(); - // set state needed at the end of the loop in ADJUST_GROUP - adjIG = sizeRemoved; - lstIG = group; - jmpRemoved = true; + // set state needed at the end of the loop in ADJUST_GROUP + adjIG = sizeRemoved; + lstIG = group; + jmpRemoved = true; - // unlink the jump - if (previousJmp != nullptr) - { - previousJmp->idjNext = jmp->idjNext; - // if it's the last jump then set it to the previous - if (jmp == emitJumpLast) + // unlink the jump + if (previousJmp != nullptr) { - emitJumpLast = previousJmp; + previousJmp->idjNext = jmp->idjNext; + if (jmp == emitJumpLast) + { + emitJumpLast = previousJmp; + } } - } - else - { - assert(jmp == emitJumpList); - emitJumpList = jmp->idjNext; - } - jmp->idjNext = nullptr; - - + else + { + assert(jmp == emitJumpList); + emitJumpList = jmp->idjNext; + } + jmp->idjNext = nullptr; #if DEBUG - // clear the instruction data - memset((BYTE*)instructionDescriptor, 0, instructionDescriptor->idCodeSize()); + // clear the instruction data + memset((BYTE*)instructionDescriptor, 0, instructionDescriptor->idCodeSize()); #endif - // remove the instruction from the group - group->igInsCnt -= 1; - group->igSize -= sizeRemoved; - emitTotalCodeSize -= sizeRemoved; + // remove the instruction from the group + group->igInsCnt -= 1; + group->igSize -= (unsigned short)sizeRemoved; + emitTotalCodeSize -= sizeRemoved; - // flag the group as resized and recounted - group->igFlags |= (IGF_UPD_ISZ | IGF_UPD_ICOUNT); + // flag the group as resized and recounted + group->igFlags |= (IGF_UPD_ISZ | IGF_UPD_ICOUNT); - //emitRemoveLabelOnlyUsedBy(jmp); + // cleanup + instructionDescriptor = nullptr; + jmp = nullptr; + removedCount += 1; - // cleanup - instructionDescriptor = nullptr; - jmp = nullptr; - removedCount += 1; - - // jump to adjusting the size - goto ADJUST_GROUP; + goto ADJUST_GROUP; + } +#ifdef DEBUG + else + { + // due to x64 ABI semantics we can't remove the jmp if the IG ends + // with "call, jmp" we must have an instruction after the call to + // ensure that an exception from the call has an instruction after + // the call to be identified as being inside the EH range + if (EMITVERBOSE) + { + printf("Unconditional jump [%08X/%03u] from the last instruction in " + "IG%02u to the next IG IG%02u label %s cannot be removed because it is the " + "last instruction in the IG after a call\n", + dspPtr(jmp), jmp->idDebugOnlyInfo()->idNum, group->igNum, targetGroup->igNum, + emitLabelString(targetGroup)); + } + } +#endif // DEBUG } } } @@ -4286,6 +4366,7 @@ bool emitter::emitRemoveJumpToNextInst() /* Did we adjust any jumps? */ if (adjIG) { + removedSize += adjIG; /* Adjust offsets of any remaining blocks */ assert(lstIG); @@ -4311,17 +4392,17 @@ bool emitter::emitRemoveJumpToNextInst() #ifdef DEBUG if (EMITVERBOSE) { - printf("Total shrinkage = %3u\n", adjIG); + printf("Shrinkage = %3u\n", adjIG); } #endif if (jmpRemoved) { - jmp_removal++; + iterationCount++; #ifdef DEBUG if (EMITVERBOSE) { - printf("Iterating branch removal. Iteration = %d\n", jmp_removal); + printf("Iterating branch removal. Iteration = %d\n", iterationCount); } #endif jmpRemoved = false; @@ -4341,7 +4422,7 @@ bool emitter::emitRemoveJumpToNextInst() #endif if (emitComp->verbose) { - printf("emitRemoveJumpToNextInst removed %d jumps\n", removedCount); + printf("emitRemoveJumpToNextInst removed %d jumps and %3u bytes\n", removedCount, removedSize); } return removedCount > 0; #else @@ -4349,71 +4430,6 @@ bool emitter::emitRemoveJumpToNextInst() #endif // TARGET_XARCH } -void emitter::emitRemoveLabelOnlyUsedBy(instrDescJmp* jmpFrom) -{ - int useCount = 0; - insGroup* jmpFromTargetGroup = (insGroup*)emitCodeGetCookie(jmpFrom->idAddr()->iiaBBlabel); - - { - instrDescJmp* jmp = emitJumpList; - while (jmp) - { - insGroup* jmpTargetGroup = ((insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel)); - if (jmp != jmpFrom && jmpTargetGroup == jmpFromTargetGroup) - { - useCount += 1; - } - jmp = jmp->idjNext; - } - jmp = nullptr; - } - - if (useCount == 0) - { -#if DEBUG - if (emitComp->verbose) - { - printf("found unused label %s\n", emitLabelString(jmpFromTargetGroup)); - } -#endif - - - if (jmpFromTargetGroup->igInsCnt > 0) - { - assert(jmpFrom->idAddr()->iiaBBlabel !=nullptr); - jmpFrom->idAddr()->iiaBBlabel = nullptr; - - instrDesc* firstTargtInstruction = (instrDesc*)jmpFromTargetGroup->igData; - if ((firstTargtInstruction->idInsFmt() & IF_LABEL) != 0) - { - firstTargtInstruction->idInsFmt((insFormat)(firstTargtInstruction->idInsFmt() ^ IF_LABEL)); - -#if DEBUG - if (emitComp->verbose) - { - printf("removed IF_LABEL from "); - emitDispIns(firstTargtInstruction, false, false, false); - printf(" in "); - emitLabelString(jmpFromTargetGroup); - printf("\n"); - } -#endif - } - - assert((firstTargtInstruction->idInsFmt() & IF_LABEL) == 0); - } - else - { -#if DEBUG - if (emitComp->verbose) - { - printf("found %d uses of label %s\n",useCount, emitLabelString(jmpFromTargetGroup)); - } -#endif - } - } -} - /***************************************************************************** * Bind targets of relative jumps to choose the smallest possible encoding. * X86 and AMD64 have a small and large encoding. diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 3699cd109f928..9a8d2d763a1d6 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -159,7 +159,7 @@ class emitLocation return ig; } - int GetInsNum() const; + int GetInsNum() const; void SetInsNum(int insNum); bool operator!=(const emitLocation& other) const @@ -2006,11 +2006,10 @@ class emitter insGroup* emitPrologIG; // prolog instruction group - instrDescJmp* emitJumpList; // list of local jumps in method - instrDescJmp* emitJumpLast; // last of local jumps in method - void emitJumpDistBind(); // Bind all the local jumps in method - bool emitRemoveJumpToNextInst(); - void emitRemoveLabelOnlyUsedBy(instrDescJmp* jmpFrom); + instrDescJmp* emitJumpList; // list of local jumps in method + instrDescJmp* emitJumpLast; // last of local jumps in method + void emitJumpDistBind(); // Bind all the local jumps in method + bool emitRemoveJumpToNextInst(); // try to remove unconditional jumps to the next instruction #if FEATURE_LOOP_ALIGN instrDescAlign* emitCurIGAlignList; // list of align instructions in current IG diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 7701246c268b7..7b2bacccc8146 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -13676,7 +13676,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) if (!id->idIsBound()) { printf("instruction has IF_LABEL flag "); - emitDispIns(id,false,false,false); + emitDispIns(id, false, false, false); printf("but should not have\n"); assert(0); } From dc23cc09f666f7826d9542d4f3e59f8a8c838df6 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Mon, 9 May 2022 02:27:30 +0100 Subject: [PATCH 03/22] use emitIsLastInsCall() to preclude call,jmp pairs and simplfy emitRemoveJumpToNextInst check for aligned instruction groups and skip them to preserve alignment linearize emitRemoveJumpToNextInst, add safety asserts for ordering and comment the requirement, add debug output explaining decisions --- src/coreclr/jit/codegencommon.cpp | 110 ++++----- src/coreclr/jit/codegenlinear.cpp | 8 +- src/coreclr/jit/emit.cpp | 368 +++++++++++++----------------- src/coreclr/jit/emitxarch.cpp | 7 - 4 files changed, 208 insertions(+), 285 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 43c386cea1b4a..a06b7d6c46bae 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1983,89 +1983,68 @@ void CodeGen::genUpdateLiveRangesForTruncatedIGs() if (compiler->lvaCount > 0) { - insGroup* ig = GetEmitter()->emitIGlist; - while (ig != nullptr) + for (insGroup* ig = GetEmitter()->emitIGlist; ig; ig = ig->igNext) { - if (ig->igFlags & IGF_UPD_ICOUNT) + if ((ig->igFlags & IGF_UPD_ICOUNT) == 0) { + continue; + } + + unsigned int liveVarCount = varLiveKeeper->getVarCount(); + if (liveVarCount > 0) + { + JITDUMP("IG%02u is marked with IGF_UPD_ICOUNT and has %d live variables\n", ig->igNum, liveVarCount); - unsigned int liveVarCount = varLiveKeeper->getVarCount(); - if (liveVarCount > 0) + for (unsigned int varNum = 0; varNum < liveVarCount; varNum++) { -#ifdef DEBUG - if (compiler->verbose) + if (compiler->compMap2ILvarNum(varNum) == (unsigned int)ICorDebugInfo::UNKNOWN_ILNUM) { - JITDUMP("IG%02u is marked with IGF_UPD_ICOUNT and has %d live variables\n", ig->igNum, - liveVarCount); + continue; } -#endif - for (unsigned int varNum = 0; varNum < liveVarCount; varNum++) + + for (int rangeIndex = 0; rangeIndex < 2; rangeIndex++) { - if (compiler->compMap2ILvarNum(varNum) == (unsigned int)ICorDebugInfo::UNKNOWN_ILNUM) + VariableLiveKeeper::LiveRangeList* liveRanges; + if (rangeIndex == 0) { - continue; + liveRanges = varLiveKeeper->getLiveRangesForVarForProlog(varNum); + } + else + { + liveRanges = varLiveKeeper->getLiveRangesForVarForBody(varNum); } - for (int rangeIndex = 0; rangeIndex < 2; rangeIndex++) + for (VariableLiveKeeper::VariableLiveRange& liveRange : *liveRanges) { - VariableLiveKeeper::LiveRangeList* liveRanges; - if (rangeIndex == 0) - { - liveRanges = varLiveKeeper->getLiveRangesForVarForProlog(varNum); - } - else + if (liveRange.m_StartEmitLocation.GetIG()->igNum == ig->igNum && + liveRange.m_StartEmitLocation.GetInsNum() > ig->igInsCnt) { - liveRanges = varLiveKeeper->getLiveRangesForVarForBody(varNum); + JITDUMP("IG%02u varNum %d StartEmitLocation InsCnt changed from %d to %d\n", ig, + liveVarCount, liveRange.m_StartEmitLocation.GetInsNum(), ig->igInsCnt); + liveRange.m_StartEmitLocation.SetInsNum(ig->igInsCnt); + assert(liveRange.m_StartEmitLocation.GetInsNum() == ig->igInsCnt); + updatedGroup = true; } - for (VariableLiveKeeper::VariableLiveRange& liveRange : *liveRanges) + if (liveRange.m_EndEmitLocation.GetIG()->igNum == ig->igNum && + liveRange.m_EndEmitLocation.GetInsNum() > ig->igInsCnt) { - if (liveRange.m_StartEmitLocation.GetIG()->igNum == ig->igNum && - liveRange.m_StartEmitLocation.GetInsNum() > ig->igInsCnt) - { -#ifdef DEBUG - if (compiler->verbose) - { - JITDUMP("IG%02u varNum %d StartEmitLocation InsCnt changed from %d to %d\n", ig, - liveVarCount, liveRange.m_StartEmitLocation.GetInsNum(), ig->igInsCnt); - } -#endif - liveRange.m_StartEmitLocation.SetInsNum(ig->igInsCnt); - assert(liveRange.m_StartEmitLocation.GetInsNum() == ig->igInsCnt); - updatedGroup = true; - } - - if (liveRange.m_EndEmitLocation.GetIG()->igNum == ig->igNum && - liveRange.m_EndEmitLocation.GetInsNum() > ig->igInsCnt) - { - liveRange.m_EndEmitLocation.SetInsNum(ig->igInsCnt); -#ifdef DEBUG - if (compiler->verbose) - { - JITDUMP("IG%02u varNum %d EndEmitLocation InsCnt changed from %d to %d\n", ig, - liveVarCount, liveRange.m_StartEmitLocation.GetInsNum(), ig->igInsCnt); - } -#endif - assert(liveRange.m_EndEmitLocation.GetInsNum() == ig->igInsCnt); - updatedGroup = true; - } + JITDUMP("IG%02u varNum %d EndEmitLocation InsCnt changed from %d to %d\n", ig, + liveVarCount, liveRange.m_StartEmitLocation.GetInsNum(), ig->igInsCnt); + liveRange.m_EndEmitLocation.SetInsNum(ig->igInsCnt); + assert(liveRange.m_EndEmitLocation.GetInsNum() == ig->igInsCnt); + updatedGroup = true; } } } } -#ifdef DEBUG - else - { - if (compiler->verbose) - { - JITDUMP("IG%02u is marked with IGF_UPD_ICOUNT and has no live variables\n", ig); - } - } -#endif - ig->igFlags ^= IGF_UPD_ICOUNT; + } + else + { + JITDUMP("IG%02u is marked with IGF_UPD_ICOUNT and has no live variables\n", ig); } - ig = ig->igNext; + ig->igFlags ^= IGF_UPD_ICOUNT; } } #ifdef DEBUG @@ -9316,6 +9295,13 @@ CodeGenInterface::VariableLiveKeeper::LiveRangeList* CodeGenInterface::VariableL return m_vlrLiveDscForProlog[varNum].getLiveRanges(); } +//------------------------------------------------------------------------ +// getVarCount: Returns the count of tracked variables, which are +// arguments, special arguments, and local IL variables. +// +// Return Value: +// int - the count of variables +// unsigned int CodeGenInterface::VariableLiveKeeper::getVarCount() const { unsigned int count = 0; diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 91942613f4e6b..1c073d124220c 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -753,7 +753,13 @@ void CodeGen::genCodeForBBlist() break; case BBJ_ALWAYS: - inst_JMP(EJ_jmp, block->bbJumpDest, /* isJmpAlways */ true); + inst_JMP(EJ_jmp, block->bbJumpDest, +#ifdef TARGET_AMD64 + /* isJmpAlways */ !GetEmitter()->emitIsLastInsCall() +#else + /* isJmpAlways */ false +#endif + ); FALLTHROUGH; case BBJ_COND: diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 9acc9ecb3abc3..f47ed6f78380f 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -3869,17 +3869,17 @@ void emitter::emitDispGCinfo() printf("\n\n"); } +//------------------------------------------------------------------------ +// emitDispJumpList: displays the current emitter jump list +// void emitter::emitDispJumpList() { printf("Emitter Jump List:\n"); - unsigned int jmpCount = 0; - instrDescJmp* jmp = emitJumpList; - instrDescJmp* previousJmp = nullptr; - while (jmp) + unsigned int jmpCount = 0; + for (instrDescJmp* jmp = emitJumpList; jmp; jmp = jmp->idjNext) { emitDispIns(jmp, false, false, false, 0, nullptr, 0, jmp->idjIG); jmpCount += 1; - jmp = jmp->idjNext; } printf(" total jump count: %u\n", jmpCount); } @@ -4122,14 +4122,18 @@ void emitter::emitDispCommentForHandle(size_t handle, GenTreeFlags flag) #endif // DEBUG } -/***************************************************************************** - * Checks all jumps in the jump list to see if they are unconditional jumps - * generated at the end of BB_ALWAYS basic blocks and whether they are now a jump - * to the next instruction, this can happen when the following basic block emitted - * no instructions - * - * returns true if any jumps have been removed, otherwise returns false - */ +//**************************************************************************** +// emitRemoveJumpToNextInst: Checks all jumps in the jump list to see if they are +// unconditional jumps generated at the end of BB_ALWAYS basic blocks and whether +// they are now a jump to the next instruction, this can happen when the following +// basic block emitted no instructions +// +// Returns: +// true if any jumps have been removed, otherwise returns false +// +// Assumptions: +// the jump list must be ordered by increasing igNum+insNo +// bool emitter::emitRemoveJumpToNextInst() { #ifdef TARGET_XARCH @@ -4140,7 +4144,7 @@ bool emitter::emitRemoveJumpToNextInst() } if (EMIT_INSTLIST_VERBOSE) { - printf("\nInstruction list before unconditional jump to next instruction removal:\n\n"); + printf("\nInstruction group list before unconditional jump to next instruction removal:\n\n"); emitDispIGlist(true); } if (EMITVERBOSE) @@ -4149,281 +4153,215 @@ bool emitter::emitRemoveJumpToNextInst() } #endif - instrDescJmp* jmp; - UNATIVE_OFFSET adjIG; - UNATIVE_OFFSET adjLJ; - insGroup* lstIG; - int removedCount = 0; - UNATIVE_OFFSET removedSize = 0; - int iterationCount = 1; - bool jmpRemoved = false; - -/*****************************************************************************/ -/* If we iterate to look for more jumps to remove, we start again here. */ -/*****************************************************************************/ - -AGAIN: - - lstIG = nullptr; - adjLJ = 0; - adjIG = 0; - jmp = emitJumpList; - instrDescJmp* previousJmp = nullptr; + int removedCount = 0; + UNATIVE_OFFSET removedSize = 0; + instrDescJmp* jmp = emitJumpList; + instrDescJmp* previousJmp = nullptr; + insGroup* previousTruncatedGroup = nullptr; +#if DEBUG + UNATIVE_OFFSET previousJumpIgNo = (UNATIVE_OFFSET)-1; + unsigned int previousJumpInsNo = -1; +#endif while (jmp) { // if the jump is unconditional then it is a candidate if (jmp->idInsFmt() == IF_LABEL && emitIsUncondJump(jmp) && !jmp->idjKeepLong && jmp->idGetJmpAlwaysFlag()) { +#if DEBUG + assert(jmp->idjIG->igNum > previousJumpIgNo || previousJumpIgNo == (UNATIVE_OFFSET)-1 || + (jmp->idjIG->igNum == previousJumpIgNo && jmp->idDebugOnlyInfo()->idNum > previousJumpInsNo)); + previousJumpIgNo = jmp->idjIG->igNum; + previousJumpInsNo = jmp->idDebugOnlyInfo()->idNum; +#endif // unset the flag so that we know it has been processed already jmp->idSetJmpAlwaysFlag(0); // target group is not bound yet so use the cookie to fetch it insGroup* targetGroup = (insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel); - if (targetGroup != nullptr && jmp->idjIG->igNext == targetGroup) + if (targetGroup != nullptr && (jmp->idjIG->igFlags & IGF_HAS_ALIGN) == 0 && + jmp->idjIG->igNext == targetGroup) { insGroup* group = jmp->idjIG; unsigned instructionCount = group->igInsCnt; if (instructionCount > 0) { - BYTE* dataPtr = group->igData; - instrDesc* instructionDescriptor = nullptr; - bool previousInstructionWasCall = false; - bool checkedPreviousInstruction = false; + BYTE* dataPtr = group->igData; + instrDesc* instructionDescriptor = nullptr; - // unroll first iteration to avoid conditional check in the loop - instructionDescriptor = (instrDesc*)dataPtr; - dataPtr += emitSizeOfInsDsc(instructionDescriptor); - instructionCount -= 1; - - if (instructionCount > 0) - { - do - { - previousInstructionWasCall = instructionDescriptor->idIns() == INS_call; - instructionDescriptor = (instrDesc*)dataPtr; - dataPtr += emitSizeOfInsDsc(instructionDescriptor); - instructionCount -= 1; - } while (instructionCount > 0); - checkedPreviousInstruction = true; - } - else + while (instructionCount > 0) { - // Special case. If the jmp is both the first and the last instruction in the - // group then we might not be able to remove the jmp because the previous - // instruction group ended in a call and assumed that the following group - // would contain the instruction needed following it (see code in genCodeForBBlist()) - // - // To avoid this find the previous IG and scan through it to find the last instruction - // and identify if it a call. this will be rare so the additional cost should not be - // high overall - - insGroup* previousGroup = nullptr; - insGroup* candidateGroup = emitIGlist; - while (candidateGroup) - { - if (candidateGroup->igNext == group) - { - previousGroup = candidateGroup; - break; - } - candidateGroup = candidateGroup->igNext; - }; - - assert(previousGroup); - - if (previousGroup->igInsCnt > 0) - { - instrDesc* pgInstructionDescriptor = nullptr; - int pgInstructionCount = previousGroup->igInsCnt; - BYTE* pgDataPtr = previousGroup->igData; - do - { - pgInstructionDescriptor = (instrDesc*)pgDataPtr; - pgDataPtr += emitSizeOfInsDsc(pgInstructionDescriptor); - pgInstructionCount -= 1; - } while (pgInstructionCount > 0); - previousInstructionWasCall = (pgInstructionDescriptor->idIns() == INS_call); - checkedPreviousInstruction = true; - } -#ifdef DEBUG - else - { - if (EMITVERBOSE) - { - printf("Unconditional jump [%08X/%03u] from the last instruction in " - "IG%02u to the next IG IG%02u label %s cannot be removed because it could not " - "be determined if the previous instruction was a call\n", - dspPtr(jmp), jmp->idDebugOnlyInfo()->idNum, group->igNum, targetGroup->igNum, - emitLabelString(targetGroup)); - } - } -#endif // DEBUG - } + instructionDescriptor = (instrDesc*)dataPtr; + dataPtr += emitSizeOfInsDsc(instructionDescriptor); + instructionCount -= 1; + }; assert(instructionDescriptor != nullptr); // instructionDescriptor is now the last instruction in the group - if (checkedPreviousInstruction && instructionDescriptor->idIns() == jmp->idIns() && - jmp == instructionDescriptor) + if (instructionDescriptor->idIns() == jmp->idIns() && jmp == instructionDescriptor) { CLANG_FORMAT_COMMENT_ANCHOR - // the last instruction in the group is the jmp we're looking for - // and it jumps to the next instruction group so we don't really need it +// the last instruction in the group is the jmp we're looking for +// and it jumps to the next instruction group so we don't really need it - if (!previousInstructionWasCall) - { #ifdef DEBUG - if (EMITVERBOSE) - { - printf("Removing unconditional jump [%08X/%03u] from the last instruction in " - "IG%02u to the next IG IG%02u label %s\n", - dspPtr(jmp), jmp->idDebugOnlyInfo()->idNum, group->igNum, targetGroup->igNum, - emitLabelString(targetGroup)); - } + if (EMITVERBOSE) + { + printf("Removing unconditional jump IN%04x from the last instruction in " + "IG%02u to the next IG IG%02u label %s\n", + jmp->idDebugOnlyInfo()->idNum, group->igNum, targetGroup->igNum, + emitLabelString(targetGroup)); + } #endif - UNATIVE_OFFSET sizeRemoved = instructionDescriptor->idCodeSize(); - - // set state needed at the end of the loop in ADJUST_GROUP - adjIG = sizeRemoved; - lstIG = group; - jmpRemoved = true; - // unlink the jump - if (previousJmp != nullptr) + if (previousTruncatedGroup) + { + for (insGroup* offsetGroup = previousTruncatedGroup->igNext; offsetGroup != group->igNext; + offsetGroup = offsetGroup->igNext) { - previousJmp->idjNext = jmp->idjNext; - if (jmp == emitJumpLast) +#ifdef DEBUG + if (EMITVERBOSE) { - emitJumpLast = previousJmp; + printf("Adjusted offset of " FMT_BB " from %04X to %04X\n", offsetGroup->igNum, + offsetGroup->igOffs, offsetGroup->igOffs - removedSize); } - } - else - { - assert(jmp == emitJumpList); - emitJumpList = jmp->idjNext; - } - jmp->idjNext = nullptr; - -#if DEBUG - // clear the instruction data - memset((BYTE*)instructionDescriptor, 0, instructionDescriptor->idCodeSize()); #endif + offsetGroup->igOffs -= removedSize; + assert(IsCodeAligned(offsetGroup->igOffs)); + } + } - // remove the instruction from the group - group->igInsCnt -= 1; - group->igSize -= (unsigned short)sizeRemoved; - emitTotalCodeSize -= sizeRemoved; - - // flag the group as resized and recounted - group->igFlags |= (IGF_UPD_ISZ | IGF_UPD_ICOUNT); + previousTruncatedGroup = group; - // cleanup - instructionDescriptor = nullptr; - jmp = nullptr; - removedCount += 1; + UNATIVE_OFFSET sizeRemoved = instructionDescriptor->idCodeSize(); - goto ADJUST_GROUP; - } -#ifdef DEBUG - else + // unlink the jump + if (previousJmp != nullptr) { - // due to x64 ABI semantics we can't remove the jmp if the IG ends - // with "call, jmp" we must have an instruction after the call to - // ensure that an exception from the call has an instruction after - // the call to be identified as being inside the EH range - if (EMITVERBOSE) + previousJmp->idjNext = jmp->idjNext; + if (jmp == emitJumpLast) { - printf("Unconditional jump [%08X/%03u] from the last instruction in " - "IG%02u to the next IG IG%02u label %s cannot be removed because it is the " - "last instruction in the IG after a call\n", - dspPtr(jmp), jmp->idDebugOnlyInfo()->idNum, group->igNum, targetGroup->igNum, - emitLabelString(targetGroup)); + emitJumpLast = previousJmp; } } -#endif // DEBUG - } - } - } - } + else + { + assert(jmp == emitJumpList); + emitJumpList = jmp->idjNext; + } - previousJmp = jmp; - if (jmp != nullptr) - { - jmp = jmp->idjNext; - } - } + // setup the next iteration + { + instrDescJmp* nextJmp = jmp->idjNext; + jmp->idjNext = nullptr; + jmp = nextJmp; + } +// do not use jmp after this point +#if DEBUG + // clear the instruction data + memset((BYTE*)instructionDescriptor, 0, instructionDescriptor->idCodeSize()); +#endif - jmp = nullptr; - lstIG = nullptr; - adjLJ = 0; - adjIG = 0; + // remove the instruction from the group + group->igInsCnt -= 1; + group->igSize -= (unsigned short)sizeRemoved; + emitTotalCodeSize -= sizeRemoved; -ADJUST_GROUP: + // flag the group as resized and recounted + group->igFlags |= (IGF_UPD_ISZ | IGF_UPD_ICOUNT); - /* Did we adjust any jumps? */ - if (adjIG) - { - removedSize += adjIG; - /* Adjust offsets of any remaining blocks */ + // cleanup + instructionDescriptor = nullptr; + removedCount += 1; - assert(lstIG); + // go to the loop condition, we have setup the next jmp while editing the list + continue; + } +#if DEBUG + else if (EMITVERBOSE) + { + printf("Unconditional jump IN%04x is not the last instruction in unaligned group IG%02u, " + "skipped.\n", + jmp->idDebugOnlyInfo()->idNum, jmp->idjIG->igNum); + } +#endif + } - for (;;) - { - lstIG = lstIG->igNext; - if (!lstIG) - { - break; +#if DEBUG + else if (EMITVERBOSE) + { + printf("Unconditional jump IN%04x is the only instruction in IG%02u, skipped.\n", + jmp->idDebugOnlyInfo()->idNum, jmp->idjIG->igNum); + } +#endif } -#ifdef DEBUG - if (EMITVERBOSE) +#if DEBUG + else if (EMITVERBOSE) { - printf("Adjusted offset of " FMT_BB " from %04X to %04X\n", lstIG->igNum, lstIG->igOffs, - lstIG->igOffs - adjIG); + if (targetGroup == nullptr) + { + printf( + "Unconditional jump IN%04x from the last instruction in IG%02u target is not set!, skipped.\n", + jmp->idDebugOnlyInfo()->idNum, jmp->idjIG->igNum); + } + else if ((jmp->idjIG->igFlags & IGF_HAS_ALIGN) != 0) + { + printf("Unconditional jump IN%04x from the last instruction in IG%02u contains alignment " + "instructions, skipped.\n", + jmp->idDebugOnlyInfo()->idNum, jmp->idjIG->igNum); + } + else if (jmp->idjIG->igNext != targetGroup) + { + printf("Unconditional jump IN%04x from the last instruction in IG%02u to does not jump to the next " + "group, skipped.\n", + jmp->idDebugOnlyInfo()->idNum, jmp->idjIG->igNum); + } } #endif - lstIG->igOffs -= adjIG; - assert(IsCodeAligned(lstIG->igOffs)); } -#ifdef DEBUG - if (EMITVERBOSE) - { - printf("Shrinkage = %3u\n", adjIG); - } -#endif + previousJmp = jmp; + jmp = jmp->idjNext; + } + + jmp = nullptr; - if (jmpRemoved) + if (previousTruncatedGroup != nullptr) + { + for (insGroup* offsetGroup = previousTruncatedGroup->igNext; offsetGroup != nullptr; + offsetGroup = offsetGroup->igNext) { - iterationCount++; #ifdef DEBUG if (EMITVERBOSE) { - printf("Iterating branch removal. Iteration = %d\n", iterationCount); + printf("Adjusted offset of " FMT_BB " from %04X to %04X\n", offsetGroup->igNum, offsetGroup->igOffs, + offsetGroup->igOffs - removedSize); } #endif - jmpRemoved = false; - goto AGAIN; + offsetGroup->igOffs -= removedSize; + assert(IsCodeAligned(offsetGroup->igOffs)); } } + #ifdef DEBUG if (EMIT_INSTLIST_VERBOSE) { - printf("\nLabels list after unconditional jump to next instruction removal:\n\n"); + printf("\nInstruction group list after unconditional jump to next instruction removal:\n\n"); emitDispIGlist(false); } if (EMITVERBOSE) { emitDispJumpList(); } -#endif - if (emitComp->verbose) + + if (EMITVERBOSE) { printf("emitRemoveJumpToNextInst removed %d jumps and %3u bytes\n", removedCount, removedSize); } +#endif return removedCount > 0; #else return 0; diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 7b2bacccc8146..24f0475f2be61 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -13673,13 +13673,6 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) case IF_RWR_LABEL: case IF_SWR_LABEL: assert(id->idGCref() == GCT_NONE); - if (!id->idIsBound()) - { - printf("instruction has IF_LABEL flag "); - emitDispIns(id, false, false, false); - printf("but should not have\n"); - assert(0); - } assert(id->idIsBound()); // TODO-XArch-Cleanup: handle IF_RWR_LABEL in emitOutputLJ() or change it to emitOutputAM()? From 78a2c7f627563d4f2d68b546b585e51b90570fb5 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 17 May 2022 22:34:31 +0100 Subject: [PATCH 04/22] address feedback --- src/coreclr/jit/codegenlinear.cpp | 2 +- src/coreclr/jit/emit.cpp | 179 +++++++++++++----------------- src/coreclr/jit/emit.h | 16 +-- src/coreclr/jit/emitxarch.cpp | 5 +- 4 files changed, 84 insertions(+), 118 deletions(-) diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 1c073d124220c..0d61dcb40f0b2 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -755,7 +755,7 @@ void CodeGen::genCodeForBBlist() case BBJ_ALWAYS: inst_JMP(EJ_jmp, block->bbJumpDest, #ifdef TARGET_AMD64 - /* isJmpAlways */ !GetEmitter()->emitIsLastInsCall() + /* isJmpAlways */ !GetEmitter()->emitIsLastInsCall() && !block->hasAlign() #else /* isJmpAlways */ false #endif diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index f47ed6f78380f..9e05a2883dc07 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -4137,14 +4137,11 @@ void emitter::emitDispCommentForHandle(size_t handle, GenTreeFlags flag) bool emitter::emitRemoveJumpToNextInst() { #ifdef TARGET_XARCH + JITDUMP("*************** In emitRemoveJumpToNextInst()\n"); #ifdef DEBUG - if (emitComp->verbose) - { - printf("*************** In emitRemoveJumpToNextInst()\n"); - } if (EMIT_INSTLIST_VERBOSE) { - printf("\nInstruction group list before unconditional jump to next instruction removal:\n\n"); + JITDUMP("\nInstruction group list before unconditional jump to next instruction removal:\n\n"); emitDispIGlist(true); } if (EMITVERBOSE) @@ -4153,91 +4150,81 @@ bool emitter::emitRemoveJumpToNextInst() } #endif - int removedCount = 0; - UNATIVE_OFFSET removedSize = 0; + int totalRemovedCount = 0; + UNATIVE_OFFSET totalRemovedSize = 0; instrDescJmp* jmp = emitJumpList; instrDescJmp* previousJmp = nullptr; insGroup* previousTruncatedGroup = nullptr; #if DEBUG - UNATIVE_OFFSET previousJumpIgNo = (UNATIVE_OFFSET)-1; - unsigned int previousJumpInsNo = -1; + UNATIVE_OFFSET previousJumpIgNum = (UNATIVE_OFFSET)-1; + unsigned int previousJumpInsNum = -1; #endif while (jmp) { // if the jump is unconditional then it is a candidate - if (jmp->idInsFmt() == IF_LABEL && emitIsUncondJump(jmp) && !jmp->idjKeepLong && jmp->idGetJmpAlwaysFlag()) + if (jmp->idInsFmt() == IF_LABEL && emitIsUncondJump(jmp) && jmp->idjIsJmpAlways) { + insGroup* jmpGroup = jmp->idjIG; #if DEBUG - assert(jmp->idjIG->igNum > previousJumpIgNo || previousJumpIgNo == (UNATIVE_OFFSET)-1 || - (jmp->idjIG->igNum == previousJumpIgNo && jmp->idDebugOnlyInfo()->idNum > previousJumpInsNo)); - previousJumpIgNo = jmp->idjIG->igNum; - previousJumpInsNo = jmp->idDebugOnlyInfo()->idNum; + assert((jmpGroup->igFlags & IGF_HAS_ALIGN) == 0); + assert(jmpGroup->igNum > previousJumpIgNum || previousJumpIgNum == (UNATIVE_OFFSET)-1 || + (jmpGroup->igNum == previousJumpIgNum && jmp->idDebugOnlyInfo()->idNum > previousJumpInsNum)); + previousJumpIgNum = jmpGroup->igNum; + previousJumpInsNum = jmp->idDebugOnlyInfo()->idNum; #endif - // unset the flag so that we know it has been processed already - jmp->idSetJmpAlwaysFlag(0); + jmp->idjIsJmpAlways = 0; // do we even need to do this now? with a single loop we should never revist // target group is not bound yet so use the cookie to fetch it insGroup* targetGroup = (insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel); - if (targetGroup != nullptr && (jmp->idjIG->igFlags & IGF_HAS_ALIGN) == 0 && - jmp->idjIG->igNext == targetGroup) + if (targetGroup != nullptr && jmpGroup->igNext == targetGroup) { - insGroup* group = jmp->idjIG; - unsigned instructionCount = group->igInsCnt; + unsigned instructionCount = jmpGroup->igInsCnt; - if (instructionCount > 0) + if (instructionCount > 1) { - BYTE* dataPtr = group->igData; instrDesc* instructionDescriptor = nullptr; - while (instructionCount > 0) { - instructionDescriptor = (instrDesc*)dataPtr; - dataPtr += emitSizeOfInsDsc(instructionDescriptor); - instructionCount -= 1; - }; + BYTE* dataPtr = jmpGroup->igData; + while (instructionCount > 0) + { + instructionDescriptor = (instrDesc*)dataPtr; + dataPtr += emitSizeOfInsDsc(instructionDescriptor); + instructionCount -= 1; + }; + dataPtr = nullptr; + } assert(instructionDescriptor != nullptr); // instructionDescriptor is now the last instruction in the group if (instructionDescriptor->idIns() == jmp->idIns() && jmp == instructionDescriptor) { - CLANG_FORMAT_COMMENT_ANCHOR -// the last instruction in the group is the jmp we're looking for -// and it jumps to the next instruction group so we don't really need it + // the last instruction in the group is the jmp we're looking for + // and it jumps to the next instruction group so we don't need it -#ifdef DEBUG - if (EMITVERBOSE) - { - printf("Removing unconditional jump IN%04x from the last instruction in " - "IG%02u to the next IG IG%02u label %s\n", - jmp->idDebugOnlyInfo()->idNum, group->igNum, targetGroup->igNum, - emitLabelString(targetGroup)); - } -#endif + JITDUMP("Removing unconditional jump IN%04x from the last instruction in " + "IG%02u to the next IG IG%02u label %s\n", + jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum, targetGroup->igNum, + emitLabelString(targetGroup)); + // if a previous group was truncated iterate from the previous group to the + // current group and ajust the offsets by the total removed byte count so far if (previousTruncatedGroup) { - for (insGroup* offsetGroup = previousTruncatedGroup->igNext; offsetGroup != group->igNext; - offsetGroup = offsetGroup->igNext) + for (insGroup* offsetGroup = previousTruncatedGroup->igNext; + offsetGroup != jmpGroup->igNext; offsetGroup = offsetGroup->igNext) { -#ifdef DEBUG - if (EMITVERBOSE) - { - printf("Adjusted offset of " FMT_BB " from %04X to %04X\n", offsetGroup->igNum, - offsetGroup->igOffs, offsetGroup->igOffs - removedSize); - } -#endif - offsetGroup->igOffs -= removedSize; + JITDUMP("Adjusted offset of " FMT_BB " from %04X to %04X\n", offsetGroup->igNum, + offsetGroup->igOffs, offsetGroup->igOffs - totalRemovedSize); + + offsetGroup->igOffs -= totalRemovedSize; assert(IsCodeAligned(offsetGroup->igOffs)); } } - previousTruncatedGroup = group; - - UNATIVE_OFFSET sizeRemoved = instructionDescriptor->idCodeSize(); - // unlink the jump if (previousJmp != nullptr) { @@ -4259,65 +4246,57 @@ bool emitter::emitRemoveJumpToNextInst() jmp->idjNext = nullptr; jmp = nextJmp; } -// do not use jmp after this point + + // do not use jmp after this point because it is now the next jmp + + previousTruncatedGroup = jmpGroup; + UNATIVE_OFFSET codeSize = instructionDescriptor->idCodeSize(); + #if DEBUG - // clear the instruction data - memset((BYTE*)instructionDescriptor, 0, instructionDescriptor->idCodeSize()); + memset((BYTE*)instructionDescriptor, 0, codeSize); #endif - // remove the instruction from the group - group->igInsCnt -= 1; - group->igSize -= (unsigned short)sizeRemoved; - emitTotalCodeSize -= sizeRemoved; + jmpGroup->igInsCnt -= 1; + jmpGroup->igSize -= (unsigned short)codeSize; + jmpGroup->igFlags |= (IGF_UPD_ISZ | IGF_UPD_ICOUNT); - // flag the group as resized and recounted - group->igFlags |= (IGF_UPD_ISZ | IGF_UPD_ICOUNT); - - // cleanup - instructionDescriptor = nullptr; - removedCount += 1; + emitTotalCodeSize -= codeSize; + totalRemovedSize += codeSize; + totalRemovedCount += 1; // go to the loop condition, we have setup the next jmp while editing the list continue; } -#if DEBUG - else if (EMITVERBOSE) + else { - printf("Unconditional jump IN%04x is not the last instruction in unaligned group IG%02u, " - "skipped.\n", - jmp->idDebugOnlyInfo()->idNum, jmp->idjIG->igNum); + JITDUMP("IN%04x is not the last instruction in unaligned group IG%02u, " + "keeping.\n", + jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum); } -#endif } - -#if DEBUG - else if (EMITVERBOSE) + else { - printf("Unconditional jump IN%04x is the only instruction in IG%02u, skipped.\n", - jmp->idDebugOnlyInfo()->idNum, jmp->idjIG->igNum); + JITDUMP("IN%04x is the only instruction in IG%02u, keeping.\n", jmp->idDebugOnlyInfo()->idNum, + jmpGroup->igNum); } -#endif } #if DEBUG - else if (EMITVERBOSE) + else { if (targetGroup == nullptr) { - printf( - "Unconditional jump IN%04x from the last instruction in IG%02u target is not set!, skipped.\n", - jmp->idDebugOnlyInfo()->idNum, jmp->idjIG->igNum); + JITDUMP("IN%04x from the last instruction in IG%02u jump target is not set!, keeping.\n", + jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum); } - else if ((jmp->idjIG->igFlags & IGF_HAS_ALIGN) != 0) + else if ((jmpGroup->igFlags & IGF_HAS_ALIGN) != 0) { - printf("Unconditional jump IN%04x from the last instruction in IG%02u contains alignment " - "instructions, skipped.\n", - jmp->idDebugOnlyInfo()->idNum, jmp->idjIG->igNum); + JITDUMP("IN%04x from the last instruction in IG%02u contains alignment, keeping.\n", + jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum); } - else if (jmp->idjIG->igNext != targetGroup) + else if (jmpGroup->igNext != targetGroup) { - printf("Unconditional jump IN%04x from the last instruction in IG%02u to does not jump to the next " - "group, skipped.\n", - jmp->idDebugOnlyInfo()->idNum, jmp->idjIG->igNum); + JITDUMP("IN%04x from the last instruction in IG%02u to does not jump to the next group, keeping.\n", + jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum); } } #endif @@ -4329,19 +4308,15 @@ bool emitter::emitRemoveJumpToNextInst() jmp = nullptr; + // make sure that any instruction groups following the last truncated group have their offset adjusted if (previousTruncatedGroup != nullptr) { for (insGroup* offsetGroup = previousTruncatedGroup->igNext; offsetGroup != nullptr; offsetGroup = offsetGroup->igNext) { -#ifdef DEBUG - if (EMITVERBOSE) - { - printf("Adjusted offset of " FMT_BB " from %04X to %04X\n", offsetGroup->igNum, offsetGroup->igOffs, - offsetGroup->igOffs - removedSize); - } -#endif - offsetGroup->igOffs -= removedSize; + JITDUMP("Adjusted offset of " FMT_BB " from %04X to %04X\n", offsetGroup->igNum, offsetGroup->igOffs, + offsetGroup->igOffs - totalRemovedSize); + offsetGroup->igOffs -= totalRemovedSize; assert(IsCodeAligned(offsetGroup->igOffs)); } } @@ -4357,12 +4332,10 @@ bool emitter::emitRemoveJumpToNextInst() emitDispJumpList(); } - if (EMITVERBOSE) - { - printf("emitRemoveJumpToNextInst removed %d jumps and %3u bytes\n", removedCount, removedSize); - } #endif - return removedCount > 0; + JITDUMP("emitRemoveJumpToNextInst removed %d jumps and %3u bytes\n", totalRemovedCount, totalRemovedSize); + + return totalRemovedCount > 0; #else return 0; #endif // TARGET_XARCH diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 9a8d2d763a1d6..81d386c7aee7b 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -672,14 +672,7 @@ class emitter _idCnsReloc = (EA_IS_CNS_RELOC(attr) ? 1 : 0); _idDspReloc = (EA_IS_DSP_RELOC(attr) ? 1 : 0); } - void idSetJmpAlwaysFlag(bool value) - { - _idIsJmpAlways = value ? 1 : 0; - } - bool idGetJmpAlwaysFlag() - { - return _idIsJmpAlways == 1; - } + //////////////////////////////////////////////////////////////////////// // Space taken up to here: // x86: 17 bits @@ -784,9 +777,8 @@ class emitter unsigned _idCnsReloc : 1; // LargeCns is an RVA and needs reloc tag unsigned _idDspReloc : 1; // LargeDsp is an RVA and needs reloc tag - unsigned _idIsJmpAlways : 1; -#define ID_EXTRA_RELOC_BITS (3) +#define ID_EXTRA_RELOC_BITS (2) //////////////////////////////////////////////////////////////////////// // Space taken up to here: @@ -1544,13 +1536,15 @@ class emitter BYTE* idjAddr; // address of jump ins (for patching) } idjTemp; - unsigned idjOffs : 30; // Before jump emission, this is the byte offset within IG of the jump instruction. + unsigned idjOffs : 29; // Before jump emission, this is the byte offset within IG of the jump instruction. // After emission, for forward jumps, this is the target offset -- in bytes from the // beginning of the function -- of the target instruction of the jump, used to // determine if this jump needs to be patched. unsigned idjShort : 1; // is the jump known to be a short one? unsigned idjKeepLong : 1; // should the jump be kept long? (used for // hot to cold and cold to hot jumps) + unsigned idjIsJmpAlways : 1; // indicates that the jump was added at the end of a BB_ALWAYS basic block + // in case the next block was not the next target }; #if FEATURE_LOOP_ALIGN diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 24f0475f2be61..7786dfb588c60 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -7493,9 +7493,8 @@ void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0 } #endif // DEBUG - id->idSetJmpAlwaysFlag(isJmpAlways); - - id->idjShort = 0; + id->idjIsJmpAlways = isJmpAlways ? 1 : 0; + id->idjShort = 0; if (dst != nullptr) { /* Assume the jump will be long */ From 51ed723b18b5bc8c407d746a1a648b49a9ce5780 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 20 May 2022 00:53:17 +0100 Subject: [PATCH 05/22] change instruction omit to use code size zero --- src/coreclr/jit/codegen.h | 1 - src/coreclr/jit/codegencommon.cpp | 125 +---------------------------- src/coreclr/jit/codegeninterface.h | 3 +- src/coreclr/jit/emit.cpp | 22 +++-- src/coreclr/jit/emit.h | 4 +- src/coreclr/jit/emitxarch.cpp | 4 +- 6 files changed, 17 insertions(+), 142 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index 6fae61b5ec937..ff7c4aa501035 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -26,7 +26,6 @@ class CodeGen final : public CodeGenInterface virtual void genGenerateCode(void** codePtr, uint32_t* nativeSizeOfCode); void genGenerateMachineCode(); - void genUpdateLiveRangesForTruncatedIGs(); void genEmitMachineCode(); void genEmitUnwindDebugGCandEH(); diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index a06b7d6c46bae..ed16b9a9fa498 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -1943,19 +1943,13 @@ void CodeGen::genGenerateMachineCode() } #endif // DEBUG - // after code generation but before eh funclet generation check to see if any jumps can be removed - if (GetEmitter()->emitRemoveJumpToNextInst()) - { - // if jumps were removed the live ranges need to be updated with the new instruction group sizes - genUpdateLiveRangesForTruncatedIGs(); - } - /* We can now generate the function prolog and epilog */ - genGeneratePrologsAndEpilogs(); - /* Bind jump distances */ + // check to see if any jumps can be removed + GetEmitter()->emitRemoveJumpToNextInst(); + /* Bind jump distances */ GetEmitter()->emitJumpDistBind(); #if FEATURE_LOOP_ALIGN @@ -1967,95 +1961,6 @@ void CodeGen::genGenerateMachineCode() /* The code is now complete and final; it should not change after this. */ } -//---------------------------------------------------------------------- -// genUpdateLiveRangesForTruncatedIGs -- find any instruction groups that have -// had their count altered and updated live ranges that refer to those groups -// -void CodeGen::genUpdateLiveRangesForTruncatedIGs() -{ -#ifdef DEBUG - if (compiler->verbose) - { - printf("*************** In genUpdateLiveRangesForTruncatedIGs()\n"); - } -#endif - bool updatedGroup = false; - - if (compiler->lvaCount > 0) - { - for (insGroup* ig = GetEmitter()->emitIGlist; ig; ig = ig->igNext) - { - if ((ig->igFlags & IGF_UPD_ICOUNT) == 0) - { - continue; - } - - unsigned int liveVarCount = varLiveKeeper->getVarCount(); - if (liveVarCount > 0) - { - JITDUMP("IG%02u is marked with IGF_UPD_ICOUNT and has %d live variables\n", ig->igNum, liveVarCount); - - for (unsigned int varNum = 0; varNum < liveVarCount; varNum++) - { - if (compiler->compMap2ILvarNum(varNum) == (unsigned int)ICorDebugInfo::UNKNOWN_ILNUM) - { - continue; - } - - for (int rangeIndex = 0; rangeIndex < 2; rangeIndex++) - { - VariableLiveKeeper::LiveRangeList* liveRanges; - if (rangeIndex == 0) - { - liveRanges = varLiveKeeper->getLiveRangesForVarForProlog(varNum); - } - else - { - liveRanges = varLiveKeeper->getLiveRangesForVarForBody(varNum); - } - - for (VariableLiveKeeper::VariableLiveRange& liveRange : *liveRanges) - { - if (liveRange.m_StartEmitLocation.GetIG()->igNum == ig->igNum && - liveRange.m_StartEmitLocation.GetInsNum() > ig->igInsCnt) - { - JITDUMP("IG%02u varNum %d StartEmitLocation InsCnt changed from %d to %d\n", ig, - liveVarCount, liveRange.m_StartEmitLocation.GetInsNum(), ig->igInsCnt); - liveRange.m_StartEmitLocation.SetInsNum(ig->igInsCnt); - assert(liveRange.m_StartEmitLocation.GetInsNum() == ig->igInsCnt); - updatedGroup = true; - } - - if (liveRange.m_EndEmitLocation.GetIG()->igNum == ig->igNum && - liveRange.m_EndEmitLocation.GetInsNum() > ig->igInsCnt) - { - JITDUMP("IG%02u varNum %d EndEmitLocation InsCnt changed from %d to %d\n", ig, - liveVarCount, liveRange.m_StartEmitLocation.GetInsNum(), ig->igInsCnt); - liveRange.m_EndEmitLocation.SetInsNum(ig->igInsCnt); - assert(liveRange.m_EndEmitLocation.GetInsNum() == ig->igInsCnt); - updatedGroup = true; - } - } - } - } - } - else - { - JITDUMP("IG%02u is marked with IGF_UPD_ICOUNT and has no live variables\n", ig); - } - - ig->igFlags ^= IGF_UPD_ICOUNT; - } - } -#ifdef DEBUG - if (updatedGroup && verbose) - { - printf("\nlvaTable after genUpdateLiveRangesForTruncatedIGs\n"); - compiler->lvaTableDump(); - } -#endif -} - //---------------------------------------------------------------------- // genEmitMachineCode -- emit the actual machine instruction code // @@ -9295,30 +9200,6 @@ CodeGenInterface::VariableLiveKeeper::LiveRangeList* CodeGenInterface::VariableL return m_vlrLiveDscForProlog[varNum].getLiveRanges(); } -//------------------------------------------------------------------------ -// getVarCount: Returns the count of tracked variables, which are -// arguments, special arguments, and local IL variables. -// -// Return Value: -// int - the count of variables -// -unsigned int CodeGenInterface::VariableLiveKeeper::getVarCount() const -{ - unsigned int count = 0; - - if (m_Compiler->opts.compDbgInfo) - { - for (unsigned int varNum = 0; varNum < m_LiveDscCount; varNum++) - { - if (m_Compiler->compMap2ILvarNum(varNum) != (unsigned int)ICorDebugInfo::UNKNOWN_ILNUM) - { - count += 1; - } - } - } - return count; -} - //------------------------------------------------------------------------ // getLiveRangesCount: Returns the count of variable locations reported for the tracked // variables, which are arguments, special arguments, and local IL variables. diff --git a/src/coreclr/jit/codegeninterface.h b/src/coreclr/jit/codegeninterface.h index d3dbd61a1bb90..17bdcb1952f67 100644 --- a/src/coreclr/jit/codegeninterface.h +++ b/src/coreclr/jit/codegeninterface.h @@ -743,8 +743,7 @@ class CodeGenInterface LiveRangeList* getLiveRangesForVarForBody(unsigned int varNum) const; LiveRangeList* getLiveRangesForVarForProlog(unsigned int varNum) const; - size_t getLiveRangesCount() const; - unsigned int getVarCount() const; + size_t getLiveRangesCount() const; // For parameters locations on prolog void psiStartVariableLiveRange(CodeGenInterface::siVarLoc varLocation, unsigned int varNum); diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 9e05a2883dc07..e6cd55f39991b 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -50,11 +50,6 @@ int emitLocation::GetInsNum() const return emitGetInsNumFromCodePos(codePos); } -void emitLocation::SetInsNum(int insNum) -{ - codePos = emitGetInsNumFromCodePos(insNum) + (emitGetInsOfsFromCodePos(codePos) << 16); -} - // Get the instruction offset in the current instruction group, which must be a funclet prolog group. // This is used to find an instruction offset used in unwind data. // TODO-AMD64-Bug?: We only support a single main function prolog group, but allow for multiple funclet prolog @@ -4173,8 +4168,6 @@ bool emitter::emitRemoveJumpToNextInst() previousJumpIgNum = jmpGroup->igNum; previousJumpInsNum = jmp->idDebugOnlyInfo()->idNum; #endif - jmp->idjIsJmpAlways = 0; // do we even need to do this now? with a single loop we should never revist - // target group is not bound yet so use the cookie to fetch it insGroup* targetGroup = (insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel); @@ -4252,13 +4245,10 @@ bool emitter::emitRemoveJumpToNextInst() previousTruncatedGroup = jmpGroup; UNATIVE_OFFSET codeSize = instructionDescriptor->idCodeSize(); -#if DEBUG - memset((BYTE*)instructionDescriptor, 0, codeSize); -#endif + instructionDescriptor->idCodeSize(0); - jmpGroup->igInsCnt -= 1; jmpGroup->igSize -= (unsigned short)codeSize; - jmpGroup->igFlags |= (IGF_UPD_ISZ | IGF_UPD_ICOUNT); + jmpGroup->igFlags |= IGF_UPD_ISZ; emitTotalCodeSize -= codeSize; totalRemovedSize += codeSize; @@ -6766,7 +6756,13 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, } #endif - + if (id->idCodeSize() == 0) + { +#ifdef DEBUG + assert(cnt == 1); +#endif + continue; + } castto(id, BYTE*) += emitIssue1Instr(ig, id, &cp); #ifdef DEBUG diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 81d386c7aee7b..177c99b494d0b 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -159,8 +159,7 @@ class emitLocation return ig; } - int GetInsNum() const; - void SetInsNum(int insNum); + int GetInsNum() const; bool operator!=(const emitLocation& other) const { @@ -280,7 +279,6 @@ struct insGroup // IG, or, if this IG contains with an unconditional branch, some subsequent IG. #define IGF_REMOVED_ALIGN 0x0800 // IG was marked as having an alignment instruction(s), but was later unmarked // without updating the IG's size/offsets. -#define IGF_UPD_ICOUNT 0x1000 // Mask of IGF_* flags that should be propagated to new blocks when they are created. // This allows prologs and epilogs to be any number of IGs, but still be diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 7786dfb588c60..600715f252ba5 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -9001,7 +9001,9 @@ void emitter::emitDispIns( /* By now the size better be set to something */ - assert(id->idCodeSize() || emitInstHasNoCode(ins)); + assert( + id->idCodeSize() || emitInstHasNoCode(ins) || + (id->idCodeSize() == 0 && (((instrDescJmp*)id)->idjIsJmpAlways == 1) && emitIsUncondJump((instrDescJmp*)id))); /* Figure out the operand size */ From 9364912b5643114ad3061d7346759acb74e8f138 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 20 May 2022 15:04:36 +0100 Subject: [PATCH 06/22] address feedback --- src/coreclr/jit/emit.cpp | 153 +++++++++++++++------------------- src/coreclr/jit/emitxarch.cpp | 14 +++- 2 files changed, 75 insertions(+), 92 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index e6cd55f39991b..7726db9ad0eeb 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -4173,102 +4173,85 @@ bool emitter::emitRemoveJumpToNextInst() if (targetGroup != nullptr && jmpGroup->igNext == targetGroup) { - unsigned instructionCount = jmpGroup->igInsCnt; +// the last instruction in the group is the jmp we're looking for +// and it jumps to the next instruction group so we don't need it - if (instructionCount > 1) +#ifdef DEBUG + unsigned instructionCount = jmpGroup->igInsCnt; + assert(instructionCount > 0); + instrDesc* instructionDescriptor = nullptr; { - instrDesc* instructionDescriptor = nullptr; - + BYTE* dataPtr = jmpGroup->igData; + while (instructionCount > 0) { - BYTE* dataPtr = jmpGroup->igData; - while (instructionCount > 0) - { - instructionDescriptor = (instrDesc*)dataPtr; - dataPtr += emitSizeOfInsDsc(instructionDescriptor); - instructionCount -= 1; - }; - dataPtr = nullptr; - } + instructionDescriptor = (instrDesc*)dataPtr; + dataPtr += emitSizeOfInsDsc(instructionDescriptor); + instructionCount -= 1; + }; + dataPtr = nullptr; + } + assert(instructionDescriptor != nullptr); + assert(instructionDescriptor->idIns() == jmp->idIns() && jmp == instructionDescriptor); +#endif - assert(instructionDescriptor != nullptr); - // instructionDescriptor is now the last instruction in the group + JITDUMP("Removing unconditional jump IN%04x from the last instruction in " + "IG%02u to the next IG IG%02u label %s\n", + jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum, targetGroup->igNum, + emitLabelString(targetGroup)); - if (instructionDescriptor->idIns() == jmp->idIns() && jmp == instructionDescriptor) + // if a previous group was truncated iterate from the previous group to the + // current group and adjust the offsets by the total removed byte count so far + if (previousTruncatedGroup) + { + for (insGroup* offsetGroup = previousTruncatedGroup->igNext; offsetGroup != jmpGroup->igNext; + offsetGroup = offsetGroup->igNext) { - // the last instruction in the group is the jmp we're looking for - // and it jumps to the next instruction group so we don't need it + JITDUMP("Adjusted offset of " FMT_BB " from %04X to %04X\n", offsetGroup->igNum, + offsetGroup->igOffs, offsetGroup->igOffs - totalRemovedSize); - JITDUMP("Removing unconditional jump IN%04x from the last instruction in " - "IG%02u to the next IG IG%02u label %s\n", - jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum, targetGroup->igNum, - emitLabelString(targetGroup)); - - // if a previous group was truncated iterate from the previous group to the - // current group and ajust the offsets by the total removed byte count so far - if (previousTruncatedGroup) - { - for (insGroup* offsetGroup = previousTruncatedGroup->igNext; - offsetGroup != jmpGroup->igNext; offsetGroup = offsetGroup->igNext) - { - JITDUMP("Adjusted offset of " FMT_BB " from %04X to %04X\n", offsetGroup->igNum, - offsetGroup->igOffs, offsetGroup->igOffs - totalRemovedSize); - - offsetGroup->igOffs -= totalRemovedSize; - assert(IsCodeAligned(offsetGroup->igOffs)); - } - } + offsetGroup->igOffs -= totalRemovedSize; + assert(IsCodeAligned(offsetGroup->igOffs)); + } + } - // unlink the jump - if (previousJmp != nullptr) - { - previousJmp->idjNext = jmp->idjNext; - if (jmp == emitJumpLast) - { - emitJumpLast = previousJmp; - } - } - else - { - assert(jmp == emitJumpList); - emitJumpList = jmp->idjNext; - } + // unlink the jump + if (previousJmp != nullptr) + { + previousJmp->idjNext = jmp->idjNext; + if (jmp == emitJumpLast) + { + emitJumpLast = previousJmp; + } + } + else + { + assert(jmp == emitJumpList); + emitJumpList = jmp->idjNext; + } - // setup the next iteration - { - instrDescJmp* nextJmp = jmp->idjNext; - jmp->idjNext = nullptr; - jmp = nextJmp; - } + UNATIVE_OFFSET codeSize = jmp->idCodeSize(); + jmp->idCodeSize(0); - // do not use jmp after this point because it is now the next jmp + // setup the next iteration + { + instrDescJmp* nextJmp = jmp->idjNext; + jmp->idjNext = nullptr; + jmp = nextJmp; + } - previousTruncatedGroup = jmpGroup; - UNATIVE_OFFSET codeSize = instructionDescriptor->idCodeSize(); + // do not use jmp after this point because it is now the next jmp - instructionDescriptor->idCodeSize(0); + previousTruncatedGroup = jmpGroup; - jmpGroup->igSize -= (unsigned short)codeSize; - jmpGroup->igFlags |= IGF_UPD_ISZ; + jmpGroup->igSize -= (unsigned short)codeSize; + jmpGroup->igFlags |= IGF_UPD_ISZ; - emitTotalCodeSize -= codeSize; - totalRemovedSize += codeSize; - totalRemovedCount += 1; + emitTotalCodeSize -= codeSize; + totalRemovedSize += codeSize; + totalRemovedCount += 1; - // go to the loop condition, we have setup the next jmp while editing the list - continue; - } - else - { - JITDUMP("IN%04x is not the last instruction in unaligned group IG%02u, " - "keeping.\n", - jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum); - } - } - else - { - JITDUMP("IN%04x is the only instruction in IG%02u, keeping.\n", jmp->idDebugOnlyInfo()->idNum, - jmpGroup->igNum); - } + // go to the loop condition, we have setup the next jmp while editing the list + continue; } #if DEBUG else @@ -6756,13 +6739,7 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, } #endif - if (id->idCodeSize() == 0) - { -#ifdef DEBUG - assert(cnt == 1); -#endif - continue; - } + castto(id, BYTE*) += emitIssue1Instr(ig, id, &cp); #ifdef DEBUG diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 600715f252ba5..cc3db0091ec2d 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -13197,7 +13197,6 @@ BYTE* emitter::emitOutputIV(BYTE* dst, instrDesc* id) * This function also handles non-jumps that have jump-like characteristics, like RIP-relative LEA of a label that * needs to get bound to an actual address and processed by branch shortening. */ - BYTE* emitter::emitOutputLJ(insGroup* ig, BYTE* dst, instrDesc* i) { unsigned srcOffs; @@ -13252,6 +13251,11 @@ BYTE* emitter::emitOutputLJ(insGroup* ig, BYTE* dst, instrDesc* i) break; } + if (jmp && id->idjIsJmpAlways == 1 && id->idCodeSize() == 0) + { + return dst; + } + // Figure out the distance to the target srcOffs = emitCurCodeOffs(dst); srcAddr = emitOffsetToPtr(srcOffs); @@ -13674,7 +13678,8 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) case IF_RWR_LABEL: case IF_SWR_LABEL: assert(id->idGCref() == GCT_NONE); - assert(id->idIsBound()); + assert(id->idIsBound() || + (emitIsUncondJump(id) && id->idCodeSize() == 0 && ((instrDescJmp*)id)->idjIsJmpAlways == 1)); // TODO-XArch-Cleanup: handle IF_RWR_LABEL in emitOutputLJ() or change it to emitOutputAM()? dst = emitOutputLJ(ig, dst, id); @@ -14694,10 +14699,11 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) assert((int)emitCurStackLvl >= 0); - // Only epilog "instructions" and some pseudo-instrs + // Only epilog "instructions", some pseudo-instrs and block ending jumps // are allowed not to generate any code - assert(*dp != dst || emitInstHasNoCode(ins)); + assert(*dp != dst || (emitInstHasNoCode(ins) || + (emitIsUncondJump(id) && id->idCodeSize() == 0 && ((instrDescJmp*)id)->idjIsJmpAlways == 1))); #ifdef DEBUG if (emitComp->opts.disAsm || emitComp->verbose) From 3650db612af2b2ed2ef1dee61e961ea686edf582 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 20 May 2022 15:48:24 +0100 Subject: [PATCH 07/22] add assertion info dump --- src/coreclr/jit/emit.cpp | 18 ++++++++++++++++-- src/coreclr/jit/emitxarch.cpp | 5 +++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 7726db9ad0eeb..a20b9a6dc31f5 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -4173,8 +4173,9 @@ bool emitter::emitRemoveJumpToNextInst() if (targetGroup != nullptr && jmpGroup->igNext == targetGroup) { -// the last instruction in the group is the jmp we're looking for -// and it jumps to the next instruction group so we don't need it + // the last instruction in the group is the jmp we're looking for + // and it jumps to the next instruction group so we don't need it + CLANG_FORMAT_COMMENT_ANCHOR #ifdef DEBUG unsigned instructionCount = jmpGroup->igInsCnt; @@ -4191,6 +4192,19 @@ bool emitter::emitRemoveJumpToNextInst() dataPtr = nullptr; } assert(instructionDescriptor != nullptr); + if (!(instructionDescriptor->idIns() == jmp->idIns() && jmp == instructionDescriptor)) + { + printf("about to assert, dumping context information\n"); + printf("method: %s\n", emitComp->impInlineRoot()->info.compMethodName); + printf(" jmp: %x3: ", jmp->idDebugOnlyInfo()->idNum); + emitDispIns(jmp, false, true, false, 0, nullptr, 0, jmpGroup); + printf(" instructionDescriptor: %x3: ", instructionDescriptor->idDebugOnlyInfo()->idNum); + emitDispIns(instructionDescriptor, false, true, false, 0, nullptr, 0, jmpGroup); + printf("jump group:\n"); + emitDispIG(jmpGroup, nullptr, true); + printf("target group:\n"); + emitDispIG(targetGroup, nullptr, false); + } assert(instructionDescriptor->idIns() == jmp->idIns() && jmp == instructionDescriptor); #endif diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index cc3db0091ec2d..6d40db1a12b03 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -9921,6 +9921,11 @@ void emitter::emitDispIns( { printf("L_M%03u_" FMT_BB, emitComp->compMethodID, id->idAddr()->iiaBBlabel->bbNum); } + + if (((instrDescJmp*)id)->idjIsJmpAlways) + { + printf(" ; BB_ALWAYS"); + } break; case IF_METHOD: From 49b6797e553fa474142496d59e1ad7628140bbe8 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Tue, 24 May 2022 22:37:47 +0100 Subject: [PATCH 08/22] address feedback --- src/coreclr/jit/emit.cpp | 10 +++------- src/coreclr/jit/emit.h | 6 +++++- src/coreclr/jit/emitxarch.cpp | 21 +++++++++++---------- 3 files changed, 19 insertions(+), 18 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index a20b9a6dc31f5..8274a375299f8 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -4129,7 +4129,7 @@ void emitter::emitDispCommentForHandle(size_t handle, GenTreeFlags flag) // Assumptions: // the jump list must be ordered by increasing igNum+insNo // -bool emitter::emitRemoveJumpToNextInst() +void emitter::emitRemoveJumpToNextInst() { #ifdef TARGET_XARCH JITDUMP("*************** In emitRemoveJumpToNextInst()\n"); @@ -4192,7 +4192,7 @@ bool emitter::emitRemoveJumpToNextInst() dataPtr = nullptr; } assert(instructionDescriptor != nullptr); - if (!(instructionDescriptor->idIns() == jmp->idIns() && jmp == instructionDescriptor)) + if (jmp != instructionDescriptor) { printf("about to assert, dumping context information\n"); printf("method: %s\n", emitComp->impInlineRoot()->info.compMethodName); @@ -4205,7 +4205,7 @@ bool emitter::emitRemoveJumpToNextInst() printf("target group:\n"); emitDispIG(targetGroup, nullptr, false); } - assert(instructionDescriptor->idIns() == jmp->idIns() && jmp == instructionDescriptor); + assert(jmp == instructionDescriptor); #endif JITDUMP("Removing unconditional jump IN%04x from the last instruction in " @@ -4321,10 +4321,6 @@ bool emitter::emitRemoveJumpToNextInst() #endif JITDUMP("emitRemoveJumpToNextInst removed %d jumps and %3u bytes\n", totalRemovedCount, totalRemovedSize); - - return totalRemovedCount > 0; -#else - return 0; #endif // TARGET_XARCH } diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 177c99b494d0b..8ef19c5bf97dd 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -2001,7 +2001,7 @@ class emitter instrDescJmp* emitJumpList; // list of local jumps in method instrDescJmp* emitJumpLast; // last of local jumps in method void emitJumpDistBind(); // Bind all the local jumps in method - bool emitRemoveJumpToNextInst(); // try to remove unconditional jumps to the next instruction + void emitRemoveJumpToNextInst(); // try to remove unconditional jumps to the next instruction #if FEATURE_LOOP_ALIGN instrDescAlign* emitCurIGAlignList; // list of align instructions in current IG @@ -2133,6 +2133,10 @@ class emitter void emitEnableGC(); #endif // !defined(JIT32_GCENCODER) +#if defined(TARGET_XARCH) + bool emitInstHasNoCode(instrDesc* id); +#endif + void emitGenIG(insGroup* ig); insGroup* emitSavIG(bool emitAdd = false); void emitNxtIG(bool extend = false); diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 6d40db1a12b03..476f86985a515 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -2064,9 +2064,14 @@ const emitJumpKind emitReverseJumpKinds[] = { * but the target register need not be byte-addressable */ -inline bool emitInstHasNoCode(instruction ins) +inline bool emitter::emitInstHasNoCode(instrDesc* id) { - if (ins == INS_align) + if (id->idIns() == INS_align) + { + return true; + } + + if (id->idCodeSize() == 0 && emitIsUncondJump((instrDescJmp*)id) && (((instrDescJmp*)id)->idjIsJmpAlways)) { return true; } @@ -9001,9 +9006,7 @@ void emitter::emitDispIns( /* By now the size better be set to something */ - assert( - id->idCodeSize() || emitInstHasNoCode(ins) || - (id->idCodeSize() == 0 && (((instrDescJmp*)id)->idjIsJmpAlways == 1) && emitIsUncondJump((instrDescJmp*)id))); + assert(id->idCodeSize() || emitInstHasNoCode(id)); /* Figure out the operand size */ @@ -13256,7 +13259,7 @@ BYTE* emitter::emitOutputLJ(insGroup* ig, BYTE* dst, instrDesc* i) break; } - if (jmp && id->idjIsJmpAlways == 1 && id->idCodeSize() == 0) + if (jmp && id->idjIsJmpAlways && id->idCodeSize() == 0) { return dst; } @@ -13683,8 +13686,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) case IF_RWR_LABEL: case IF_SWR_LABEL: assert(id->idGCref() == GCT_NONE); - assert(id->idIsBound() || - (emitIsUncondJump(id) && id->idCodeSize() == 0 && ((instrDescJmp*)id)->idjIsJmpAlways == 1)); + assert(id->idIsBound() || emitInstHasNoCode(id)); // TODO-XArch-Cleanup: handle IF_RWR_LABEL in emitOutputLJ() or change it to emitOutputAM()? dst = emitOutputLJ(ig, dst, id); @@ -14707,8 +14709,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) // Only epilog "instructions", some pseudo-instrs and block ending jumps // are allowed not to generate any code - assert(*dp != dst || (emitInstHasNoCode(ins) || - (emitIsUncondJump(id) && id->idCodeSize() == 0 && ((instrDescJmp*)id)->idjIsJmpAlways == 1))); + assert(*dp != dst || emitInstHasNoCode(id)); #ifdef DEBUG if (emitComp->opts.disAsm || emitComp->verbose) From 21eb96fdb5c9f72630d2d174c9413b53c73a0c89 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Thu, 26 May 2022 21:26:33 +0100 Subject: [PATCH 09/22] address feedback --- src/coreclr/jit/codegen.h | 5 +- src/coreclr/jit/codegenarmarch.cpp | 24 +++++++++ src/coreclr/jit/codegenlinear.cpp | 13 +++-- src/coreclr/jit/codegenxarch.cpp | 24 +++++++++ src/coreclr/jit/emit.cpp | 81 +++++++++++++++++------------- src/coreclr/jit/emit.h | 23 +++++---- src/coreclr/jit/emitarm.cpp | 2 +- src/coreclr/jit/emitarm.h | 6 +++ src/coreclr/jit/emitarm64.cpp | 2 +- src/coreclr/jit/emitarm64.h | 6 +++ src/coreclr/jit/emitpub.h | 6 --- src/coreclr/jit/emitxarch.cpp | 22 ++++---- src/coreclr/jit/emitxarch.h | 6 +++ src/coreclr/jit/instr.cpp | 26 ---------- 14 files changed, 152 insertions(+), 94 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index ff7c4aa501035..cec1f24c68ed8 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -1472,8 +1472,11 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX public: void instGen(instruction ins); - +#if defined(TARGET_XARCH) void inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock, bool isJmpAlways = false); +#else + void inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock); +#endif void inst_SET(emitJumpKind condition, regNumber reg); diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index f1426fc23487b..0d3bf4b2db4c7 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -4301,6 +4301,30 @@ void CodeGen::inst_SETCC(GenCondition condition, var_types type, regNumber dstRe #endif } +//------------------------------------------------------------------------ +// inst_JMP: Generate a jump instruction. +// +void CodeGen::inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock) +{ +#if !FEATURE_FIXED_OUT_ARGS + // On the x86 we are pushing (and changing the stack level), but on x64 and other archs we have + // a fixed outgoing args area that we store into and we never change the stack level when calling methods. + // + // Thus only on x86 do we need to assert that the stack level at the target block matches the current stack level. + // + CLANG_FORMAT_COMMENT_ANCHOR; + +#ifdef UNIX_X86_ABI + // bbTgtStkDepth is a (pure) argument count (stack alignment padding should be excluded). + assert((tgtBlock->bbTgtStkDepth * sizeof(int) == (genStackLevel - curNestedAlignment)) || isFramePointerUsed()); +#else + assert((tgtBlock->bbTgtStkDepth * sizeof(int) == genStackLevel) || isFramePointerUsed()); +#endif +#endif // !FEATURE_FIXED_OUT_ARGS + + GetEmitter()->emitIns_J(emitter::emitJumpKindToIns(jmp), tgtBlock); +} + //------------------------------------------------------------------------ // genCodeForStoreBlk: Produce code for a GT_STORE_OBJ/GT_STORE_DYN_BLK/GT_STORE_BLK node. // diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 0d61dcb40f0b2..64c801e0f2441 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -753,11 +753,18 @@ void CodeGen::genCodeForBBlist() break; case BBJ_ALWAYS: - inst_JMP(EJ_jmp, block->bbJumpDest, + inst_JMP(EJ_jmp, block->bbJumpDest #ifdef TARGET_AMD64 + // AMD64 and ARM64 require an instruction after a call instruction for unwinding + // so if the last instruction generated was a call instruction do not allow + // this jump to be marked for possible later removal. + // + // If a block has alignment the jump before the alignment can prevent the need + // to decode the instructions used for alignment so we want to keep the jump + // and it should not be marked for posible later removal. + + , /* isJmpAlways */ !GetEmitter()->emitIsLastInsCall() && !block->hasAlign() -#else - /* isJmpAlways */ false #endif ); FALLTHROUGH; diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index bab2c05407b74..93e6bf1feb752 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -1368,6 +1368,30 @@ void CodeGen::inst_SETCC(GenCondition condition, var_types type, regNumber dstRe } } +//------------------------------------------------------------------------ +// inst_JMP: Generate a jump instruction. +// +void CodeGen::inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock, bool isJmpAlways) +{ +#if !FEATURE_FIXED_OUT_ARGS + // On the x86 we are pushing (and changing the stack level), but on x64 and other archs we have + // a fixed outgoing args area that we store into and we never change the stack level when calling methods. + // + // Thus only on x86 do we need to assert that the stack level at the target block matches the current stack level. + // + CLANG_FORMAT_COMMENT_ANCHOR; + +#ifdef UNIX_X86_ABI + // bbTgtStkDepth is a (pure) argument count (stack alignment padding should be excluded). + assert((tgtBlock->bbTgtStkDepth * sizeof(int) == (genStackLevel - curNestedAlignment)) || isFramePointerUsed()); +#else + assert((tgtBlock->bbTgtStkDepth * sizeof(int) == genStackLevel) || isFramePointerUsed()); +#endif +#endif // !FEATURE_FIXED_OUT_ARGS + + GetEmitter()->emitIns_J(emitter::emitJumpKindToIns(jmp), tgtBlock, 0, isJmpAlways); +} + //------------------------------------------------------------------------ // genCodeForReturnTrap: Produce code for a GT_RETURNTRAP node. // diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 8274a375299f8..fa2f884df13fe 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -3871,9 +3871,18 @@ void emitter::emitDispJumpList() { printf("Emitter Jump List:\n"); unsigned int jmpCount = 0; - for (instrDescJmp* jmp = emitJumpList; jmp; jmp = jmp->idjNext) + for (instrDescJmp* jmp = emitJumpList; jmp != nullptr; jmp = jmp->idjNext) { - emitDispIns(jmp, false, false, false, 0, nullptr, 0, jmp->idjIG); + printf("IG%02u IN%04x %3s[size:%u] -> IG%02u L_M%03u_" FMT_BB "%s\n", jmp->idjIG->igNum, + jmp->idDebugOnlyInfo()->idNum, codeGen->genInsDisplayName(jmp), jmp->idCodeSize(), + ((insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel))->igNum, emitComp->compMethodID, + jmp->idAddr()->iiaBBlabel->bbNum, +#if defined(TARGET_XARCH) + jmp->idjIsJmpAlways ? " ; BBJ_ALWAYS" : "" +#else + "" +#endif + ); jmpCount += 1; } printf(" total jump count: %u\n", jmpCount); @@ -4119,12 +4128,9 @@ void emitter::emitDispCommentForHandle(size_t handle, GenTreeFlags flag) //**************************************************************************** // emitRemoveJumpToNextInst: Checks all jumps in the jump list to see if they are -// unconditional jumps generated at the end of BB_ALWAYS basic blocks and whether -// they are now a jump to the next instruction, this can happen when the following -// basic block emitted no instructions -// -// Returns: -// true if any jumps have been removed, otherwise returns false +// unconditional jumps generated at the end of BBJ_ALWAYS basic blocks and whether +// they are now a jump to the next instruction. This can happen when the following +// basic block emitted no instructions. // // Assumptions: // the jump list must be ordered by increasing igNum+insNo @@ -4143,7 +4149,7 @@ void emitter::emitRemoveJumpToNextInst() { emitDispJumpList(); } -#endif +#endif // DEBUG int totalRemovedCount = 0; UNATIVE_OFFSET totalRemovedSize = 0; @@ -4153,7 +4159,7 @@ void emitter::emitRemoveJumpToNextInst() #if DEBUG UNATIVE_OFFSET previousJumpIgNum = (UNATIVE_OFFSET)-1; unsigned int previousJumpInsNum = -1; -#endif +#endif // DEBUG while (jmp) { @@ -4163,15 +4169,15 @@ void emitter::emitRemoveJumpToNextInst() insGroup* jmpGroup = jmp->idjIG; #if DEBUG assert((jmpGroup->igFlags & IGF_HAS_ALIGN) == 0); - assert(jmpGroup->igNum > previousJumpIgNum || previousJumpIgNum == (UNATIVE_OFFSET)-1 || - (jmpGroup->igNum == previousJumpIgNum && jmp->idDebugOnlyInfo()->idNum > previousJumpInsNum)); + assert((jmpGroup->igNum > previousJumpIgNum) || (previousJumpIgNum == (UNATIVE_OFFSET)-1) || + ((jmpGroup->igNum == previousJumpIgNum) && (jmp->idDebugOnlyInfo()->idNum > previousJumpInsNum))); previousJumpIgNum = jmpGroup->igNum; previousJumpInsNum = jmp->idDebugOnlyInfo()->idNum; -#endif +#endif // DEBUG // target group is not bound yet so use the cookie to fetch it insGroup* targetGroup = (insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel); - if (targetGroup != nullptr && jmpGroup->igNext == targetGroup) + if ((targetGroup != nullptr) && (jmpGroup->igNext == targetGroup)) { // the last instruction in the group is the jmp we're looking for // and it jumps to the next instruction group so we don't need it @@ -4180,33 +4186,32 @@ void emitter::emitRemoveJumpToNextInst() #ifdef DEBUG unsigned instructionCount = jmpGroup->igInsCnt; assert(instructionCount > 0); - instrDesc* instructionDescriptor = nullptr; + instrDesc* id = nullptr; { BYTE* dataPtr = jmpGroup->igData; while (instructionCount > 0) { - instructionDescriptor = (instrDesc*)dataPtr; - dataPtr += emitSizeOfInsDsc(instructionDescriptor); + id = (instrDesc*)dataPtr; + dataPtr += emitSizeOfInsDsc(id); instructionCount -= 1; - }; - dataPtr = nullptr; + } } - assert(instructionDescriptor != nullptr); - if (jmp != instructionDescriptor) + assert(id != nullptr); + if (jmp != id) { printf("about to assert, dumping context information\n"); printf("method: %s\n", emitComp->impInlineRoot()->info.compMethodName); printf(" jmp: %x3: ", jmp->idDebugOnlyInfo()->idNum); emitDispIns(jmp, false, true, false, 0, nullptr, 0, jmpGroup); - printf(" instructionDescriptor: %x3: ", instructionDescriptor->idDebugOnlyInfo()->idNum); - emitDispIns(instructionDescriptor, false, true, false, 0, nullptr, 0, jmpGroup); + printf(" instructionDescriptor: %x3: ", id->idDebugOnlyInfo()->idNum); + emitDispIns(id, false, true, false, 0, nullptr, 0, jmpGroup); printf("jump group:\n"); emitDispIG(jmpGroup, nullptr, true); printf("target group:\n"); emitDispIG(targetGroup, nullptr, false); } - assert(jmp == instructionDescriptor); -#endif + assert(jmp == id); +#endif // DEBUG JITDUMP("Removing unconditional jump IN%04x from the last instruction in " "IG%02u to the next IG IG%02u label %s\n", @@ -4286,7 +4291,7 @@ void emitter::emitRemoveJumpToNextInst() jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum); } } -#endif +#endif // DEBUG } previousJmp = jmp; @@ -4308,19 +4313,25 @@ void emitter::emitRemoveJumpToNextInst() } } -#ifdef DEBUG - if (EMIT_INSTLIST_VERBOSE) + if (totalRemovedCount > 0) { - printf("\nInstruction group list after unconditional jump to next instruction removal:\n\n"); - emitDispIGlist(false); +#ifdef DEBUG + if (EMIT_INSTLIST_VERBOSE) + { + printf("\nInstruction group list after unconditional jump to next instruction removal:\n\n"); + emitDispIGlist(false); + } + if (EMITVERBOSE) + { + emitDispJumpList(); + } +#endif // DEBUG + JITDUMP("emitRemoveJumpToNextInst removed %d jumps and %3u bytes\n", totalRemovedCount, totalRemovedSize); } - if (EMITVERBOSE) + else { - emitDispJumpList(); + JITDUMP("emitRemoveJumpToNextInst no unconditional jumps removed\n"); } - -#endif - JITDUMP("emitRemoveJumpToNextInst removed %d jumps and %3u bytes\n", totalRemovedCount, totalRemovedSize); #endif // TARGET_XARCH } diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 8ef19c5bf97dd..8c13cf7922e1d 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -1534,15 +1534,20 @@ class emitter BYTE* idjAddr; // address of jump ins (for patching) } idjTemp; - unsigned idjOffs : 29; // Before jump emission, this is the byte offset within IG of the jump instruction. - // After emission, for forward jumps, this is the target offset -- in bytes from the - // beginning of the function -- of the target instruction of the jump, used to - // determine if this jump needs to be patched. - unsigned idjShort : 1; // is the jump known to be a short one? - unsigned idjKeepLong : 1; // should the jump be kept long? (used for - // hot to cold and cold to hot jumps) - unsigned idjIsJmpAlways : 1; // indicates that the jump was added at the end of a BB_ALWAYS basic block - // in case the next block was not the next target + // Before jump emission, this is the byte offset within IG of the jump instruction. + // After emission, for forward jumps, this is the target offset -- in bytes from the + // beginning of the function -- of the target instruction of the jump, used to + // determine if this jump needs to be patched. + unsigned idjOffs : +#if defined(TARGET_XARCH) + 29; + + unsigned idjIsJmpAlways : 1; // indicates that the jump was added at the end of a BBJ_ALWAYS basic block +#else + 30; +#endif + unsigned idjShort : 1; // is the jump known to be a short one? + unsigned idjKeepLong : 1; // should the jump be kept long? (used for hot to cold and cold to hot jumps) }; #if FEATURE_LOOP_ALIGN diff --git a/src/coreclr/jit/emitarm.cpp b/src/coreclr/jit/emitarm.cpp index 4d2c872d21300..ccf3ddff35709 100644 --- a/src/coreclr/jit/emitarm.cpp +++ b/src/coreclr/jit/emitarm.cpp @@ -4334,7 +4334,7 @@ void emitter::emitSetMediumJump(instrDescJmp* id) * branch. Thus, we can handle branch offsets of imm24 instead of just imm20. */ -void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0 */, bool isJmpAlways /* = false */) +void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0 */) { insFormat fmt = IF_NONE; diff --git a/src/coreclr/jit/emitarm.h b/src/coreclr/jit/emitarm.h index 9dd2c175190dc..09133a1d88f02 100644 --- a/src/coreclr/jit/emitarm.h +++ b/src/coreclr/jit/emitarm.h @@ -192,6 +192,12 @@ inline static unsigned getBitWidth(emitAttr size) return (unsigned)size * BITS_PER_BYTE; } +/************************************************************************/ +/* Output target-independent instructions */ +/************************************************************************/ + +void emitIns_J(instruction ins, BasicBlock* dst, int instrCount = 0); + /************************************************************************/ /* The public entry points to output instructions */ /************************************************************************/ diff --git a/src/coreclr/jit/emitarm64.cpp b/src/coreclr/jit/emitarm64.cpp index 6eaaedee5ce7d..864ba862edddd 100644 --- a/src/coreclr/jit/emitarm64.cpp +++ b/src/coreclr/jit/emitarm64.cpp @@ -8402,7 +8402,7 @@ void emitter::emitIns_J_R_I(instruction ins, emitAttr attr, BasicBlock* dst, reg appendToCurIG(id); } -void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount, bool isJmpAlways) +void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount) { insFormat fmt = IF_NONE; diff --git a/src/coreclr/jit/emitarm64.h b/src/coreclr/jit/emitarm64.h index 0279b3360e75d..8970c15b3c090 100644 --- a/src/coreclr/jit/emitarm64.h +++ b/src/coreclr/jit/emitarm64.h @@ -705,6 +705,12 @@ inline static ssize_t computeRelPageAddr(size_t dstAddr, size_t srcAddr) return (dstAddr >> 12) - (srcAddr >> 12); } +/************************************************************************/ +/* Output target-independent instructions */ +/************************************************************************/ + +void emitIns_J(instruction ins, BasicBlock* dst, int instrCount = 0); + /************************************************************************/ /* The public entry points to output instructions */ /************************************************************************/ diff --git a/src/coreclr/jit/emitpub.h b/src/coreclr/jit/emitpub.h index cd7d756f1c3dc..6fc2a78707417 100644 --- a/src/coreclr/jit/emitpub.h +++ b/src/coreclr/jit/emitpub.h @@ -72,12 +72,6 @@ UNATIVE_OFFSET emitCodeOffset(void* blockPtr, unsigned codeOffs); const char* emitOffsetToLabel(unsigned offs); #endif // DEBUG -/************************************************************************/ -/* Output target-independent instructions */ -/************************************************************************/ - -void emitIns_J(instruction ins, BasicBlock* dst, int instrCount = 0, bool isJmpAlways = false); - /************************************************************************/ /* Emit initialized data sections */ /************************************************************************/ diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 476f86985a515..1a8370c33b853 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -2071,7 +2071,7 @@ inline bool emitter::emitInstHasNoCode(instrDesc* id) return true; } - if (id->idCodeSize() == 0 && emitIsUncondJump((instrDescJmp*)id) && (((instrDescJmp*)id)->idjIsJmpAlways)) + if ((id->idCodeSize() == 0) && emitIsUncondJump(id) && (((instrDescJmp*)id)->idjIsJmpAlways)) { return true; } @@ -9925,10 +9925,6 @@ void emitter::emitDispIns( printf("L_M%03u_" FMT_BB, emitComp->compMethodID, id->idAddr()->iiaBBlabel->bbNum); } - if (((instrDescJmp*)id)->idjIsJmpAlways) - { - printf(" ; BB_ALWAYS"); - } break; case IF_METHOD: @@ -13259,11 +13255,6 @@ BYTE* emitter::emitOutputLJ(insGroup* ig, BYTE* dst, instrDesc* i) break; } - if (jmp && id->idjIsJmpAlways && id->idCodeSize() == 0) - { - return dst; - } - // Figure out the distance to the target srcOffs = emitCurCodeOffs(dst); srcAddr = emitOffsetToPtr(srcOffs); @@ -13689,8 +13680,15 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) assert(id->idIsBound() || emitInstHasNoCode(id)); // TODO-XArch-Cleanup: handle IF_RWR_LABEL in emitOutputLJ() or change it to emitOutputAM()? - dst = emitOutputLJ(ig, dst, id); - sz = (id->idInsFmt() == IF_SWR_LABEL ? sizeof(instrDescLbl) : sizeof(instrDescJmp)); + if (id->idCodeSize() != 0) + { + dst = emitOutputLJ(ig, dst, id); + } + else + { + assert(((instrDescJmp*)id)->idjIsJmpAlways); + } + sz = (id->idInsFmt() == IF_SWR_LABEL ? sizeof(instrDescLbl) : sizeof(instrDescJmp)); break; case IF_METHOD: diff --git a/src/coreclr/jit/emitxarch.h b/src/coreclr/jit/emitxarch.h index 3f1efdaac5e52..68b45b0be6f7f 100644 --- a/src/coreclr/jit/emitxarch.h +++ b/src/coreclr/jit/emitxarch.h @@ -300,6 +300,12 @@ inline emitAttr emitDecodeScale(unsigned ensz) return emitter::emitSizeDecode[ensz]; } +/************************************************************************/ +/* Output target-independent instructions */ +/************************************************************************/ + +void emitIns_J(instruction ins, BasicBlock* dst, int instrCount = 0, bool isJmpAlways = false); + /************************************************************************/ /* The public entry points to output instructions */ /************************************************************************/ diff --git a/src/coreclr/jit/instr.cpp b/src/coreclr/jit/instr.cpp index 3e76cefce5118..671445a2457d8 100644 --- a/src/coreclr/jit/instr.cpp +++ b/src/coreclr/jit/instr.cpp @@ -256,32 +256,6 @@ bool CodeGenInterface::instIsFP(instruction ins) #endif } -/***************************************************************************** - * - * Generate a jump instruction. - */ - -void CodeGen::inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock, bool isJmpAlways) -{ -#if !FEATURE_FIXED_OUT_ARGS - // On the x86 we are pushing (and changing the stack level), but on x64 and other archs we have - // a fixed outgoing args area that we store into and we never change the stack level when calling methods. - // - // Thus only on x86 do we need to assert that the stack level at the target block matches the current stack level. - // - CLANG_FORMAT_COMMENT_ANCHOR; - -#ifdef UNIX_X86_ABI - // bbTgtStkDepth is a (pure) argument count (stack alignment padding should be excluded). - assert((tgtBlock->bbTgtStkDepth * sizeof(int) == (genStackLevel - curNestedAlignment)) || isFramePointerUsed()); -#else - assert((tgtBlock->bbTgtStkDepth * sizeof(int) == genStackLevel) || isFramePointerUsed()); -#endif -#endif // !FEATURE_FIXED_OUT_ARGS - - GetEmitter()->emitIns_J(emitter::emitJumpKindToIns(jmp), tgtBlock, 0, isJmpAlways); -} - /***************************************************************************** * * Generate a set instruction. From 28ada9fd602284308e3343ef464594e00fa8a1d5 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 27 May 2022 02:17:20 +0100 Subject: [PATCH 10/22] remove end loop cleanup. cleanup jitdump messages and add early out flag. --- src/coreclr/jit/emit.cpp | 108 +++++++++++++++------------------- src/coreclr/jit/emit.h | 1 + src/coreclr/jit/emitxarch.cpp | 1 + 3 files changed, 48 insertions(+), 62 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index fa2f884df13fe..18d1f154eee7a 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -1077,6 +1077,7 @@ void emitter::emitBegFN(bool hasFramePtr emitNoGCRequestCount = 0; emitNoGCIG = false; emitForceNewIG = false; + emitContainsCandidateJumpsToNextInst = false; #if FEATURE_LOOP_ALIGN /* We don't have any align instructions */ @@ -3873,7 +3874,7 @@ void emitter::emitDispJumpList() unsigned int jmpCount = 0; for (instrDescJmp* jmp = emitJumpList; jmp != nullptr; jmp = jmp->idjNext) { - printf("IG%02u IN%04x %3s[size:%u] -> IG%02u L_M%03u_" FMT_BB "%s\n", jmp->idjIG->igNum, + printf("IG%02u IN%04x %3s[%u] -> IG%02u L_M%03u_" FMT_BB "%s\n", jmp->idjIG->igNum, jmp->idDebugOnlyInfo()->idNum, codeGen->genInsDisplayName(jmp), jmp->idCodeSize(), ((insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel))->igNum, emitComp->compMethodID, jmp->idAddr()->iiaBBlabel->bbNum, @@ -4138,6 +4139,11 @@ void emitter::emitDispCommentForHandle(size_t handle, GenTreeFlags flag) void emitter::emitRemoveJumpToNextInst() { #ifdef TARGET_XARCH + if (!emitContainsCandidateJumpsToNextInst) + { + return; + } + JITDUMP("*************** In emitRemoveJumpToNextInst()\n"); #ifdef DEBUG if (EMIT_INSTLIST_VERBOSE) @@ -4155,7 +4161,6 @@ void emitter::emitRemoveJumpToNextInst() UNATIVE_OFFSET totalRemovedSize = 0; instrDescJmp* jmp = emitJumpList; instrDescJmp* previousJmp = nullptr; - insGroup* previousTruncatedGroup = nullptr; #if DEBUG UNATIVE_OFFSET previousJumpIgNum = (UNATIVE_OFFSET)-1; unsigned int previousJumpInsNum = -1; @@ -4163,10 +4168,12 @@ void emitter::emitRemoveJumpToNextInst() while (jmp) { + insGroup* jmpGroup = jmp->idjIG; + instrDescJmp* nextJmp = jmp->idjNext; + // if the jump is unconditional then it is a candidate if (jmp->idInsFmt() == IF_LABEL && emitIsUncondJump(jmp) && jmp->idjIsJmpAlways) { - insGroup* jmpGroup = jmp->idjIG; #if DEBUG assert((jmpGroup->igFlags & IGF_HAS_ALIGN) == 0); assert((jmpGroup->igNum > previousJumpIgNum) || (previousJumpIgNum == (UNATIVE_OFFSET)-1) || @@ -4174,6 +4181,7 @@ void emitter::emitRemoveJumpToNextInst() previousJumpIgNum = jmpGroup->igNum; previousJumpInsNum = jmp->idDebugOnlyInfo()->idNum; #endif // DEBUG + // target group is not bound yet so use the cookie to fetch it insGroup* targetGroup = (insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel); @@ -4199,11 +4207,11 @@ void emitter::emitRemoveJumpToNextInst() assert(id != nullptr); if (jmp != id) { - printf("about to assert, dumping context information\n"); + printf("jmp != id, dumping context information\n"); printf("method: %s\n", emitComp->impInlineRoot()->info.compMethodName); printf(" jmp: %x3: ", jmp->idDebugOnlyInfo()->idNum); emitDispIns(jmp, false, true, false, 0, nullptr, 0, jmpGroup); - printf(" instructionDescriptor: %x3: ", id->idDebugOnlyInfo()->idNum); + printf(" id: %x3: ", id->idDebugOnlyInfo()->idNum); emitDispIns(id, false, true, false, 0, nullptr, 0, jmpGroup); printf("jump group:\n"); emitDispIG(jmpGroup, nullptr, true); @@ -4213,27 +4221,11 @@ void emitter::emitRemoveJumpToNextInst() assert(jmp == id); #endif // DEBUG - JITDUMP("Removing unconditional jump IN%04x from the last instruction in " - "IG%02u to the next IG IG%02u label %s\n", - jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum, targetGroup->igNum, + JITDUMP("IG%02u IN%04x is the last instruction in the group and jumps to the next instruction group IG%02u %s, removing.\n", + jmpGroup->igNum, jmp->idDebugOnlyInfo()->idNum, targetGroup->igNum, emitLabelString(targetGroup)); - // if a previous group was truncated iterate from the previous group to the - // current group and adjust the offsets by the total removed byte count so far - if (previousTruncatedGroup) - { - for (insGroup* offsetGroup = previousTruncatedGroup->igNext; offsetGroup != jmpGroup->igNext; - offsetGroup = offsetGroup->igNext) - { - JITDUMP("Adjusted offset of " FMT_BB " from %04X to %04X\n", offsetGroup->igNum, - offsetGroup->igOffs, offsetGroup->igOffs - totalRemovedSize); - - offsetGroup->igOffs -= totalRemovedSize; - assert(IsCodeAligned(offsetGroup->igOffs)); - } - } - - // unlink the jump + // Unlink the jump from emitJumpList while keeping the previousJmp the same. if (previousJmp != nullptr) { previousJmp->idjNext = jmp->idjNext; @@ -4251,71 +4243,63 @@ void emitter::emitRemoveJumpToNextInst() UNATIVE_OFFSET codeSize = jmp->idCodeSize(); jmp->idCodeSize(0); - // setup the next iteration - { - instrDescJmp* nextJmp = jmp->idjNext; - jmp->idjNext = nullptr; - jmp = nextJmp; - } - - // do not use jmp after this point because it is now the next jmp - - previousTruncatedGroup = jmpGroup; - jmpGroup->igSize -= (unsigned short)codeSize; jmpGroup->igFlags |= IGF_UPD_ISZ; emitTotalCodeSize -= codeSize; totalRemovedSize += codeSize; totalRemovedCount += 1; - - // go to the loop condition, we have setup the next jmp while editing the list - continue; } -#if DEBUG else { + // Update the previousJmp + previousJmp = jmp; +#if DEBUG if (targetGroup == nullptr) { - JITDUMP("IN%04x from the last instruction in IG%02u jump target is not set!, keeping.\n", - jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum); + JITDUMP("IG%02u IN%04x jump target is not set!, keeping.\n", + jmpGroup->igNum,jmp->idDebugOnlyInfo()->idNum ); } else if ((jmpGroup->igFlags & IGF_HAS_ALIGN) != 0) { - JITDUMP("IN%04x from the last instruction in IG%02u contains alignment, keeping.\n", - jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum); + JITDUMP("IG%02u IN%04x containing instruction group has alignment, keeping.\n", + jmpGroup->igNum,jmp->idDebugOnlyInfo()->idNum); } else if (jmpGroup->igNext != targetGroup) { - JITDUMP("IN%04x from the last instruction in IG%02u to does not jump to the next group, keeping.\n", - jmp->idDebugOnlyInfo()->idNum, jmpGroup->igNum); + JITDUMP("IG%02u IN%04x does not jump to the next instruction group, keeping.\n", + jmpGroup->igNum,jmp->idDebugOnlyInfo()->idNum); } - } #endif // DEBUG + } + } + else + { + // Update the previousJmp + previousJmp = jmp; } - previousJmp = jmp; - jmp = jmp->idjNext; - } - - jmp = nullptr; - - // make sure that any instruction groups following the last truncated group have their offset adjusted - if (previousTruncatedGroup != nullptr) - { - for (insGroup* offsetGroup = previousTruncatedGroup->igNext; offsetGroup != nullptr; - offsetGroup = offsetGroup->igNext) + if (totalRemovedCount>0) { - JITDUMP("Adjusted offset of " FMT_BB " from %04X to %04X\n", offsetGroup->igNum, offsetGroup->igOffs, - offsetGroup->igOffs - totalRemovedSize); - offsetGroup->igOffs -= totalRemovedSize; - assert(IsCodeAligned(offsetGroup->igOffs)); + insGroup* adjOffIG = jmpGroup->igNext; + insGroup* adjOffUptoIG = nextJmp != nullptr ? nextJmp->idjIG : emitIGlast; + while ((adjOffIG != nullptr) && (adjOffIG->igNum <= adjOffUptoIG->igNum)) + { + JITDUMP("Adjusted offset of IG%02u from %04X to %04X\n", adjOffIG->igNum, adjOffIG->igOffs, + (adjOffIG->igOffs - totalRemovedSize)); + adjOffIG->igOffs -= totalRemovedSize; + adjOffIG = adjOffIG->igNext; + } } + + jmp = nextJmp; } if (totalRemovedCount > 0) { #ifdef DEBUG + emitCheckIGoffsets(); + if (EMIT_INSTLIST_VERBOSE) { printf("\nInstruction group list after unconditional jump to next instruction removal:\n\n"); @@ -4326,7 +4310,7 @@ void emitter::emitRemoveJumpToNextInst() emitDispJumpList(); } #endif // DEBUG - JITDUMP("emitRemoveJumpToNextInst removed %d jumps and %3u bytes\n", totalRemovedCount, totalRemovedSize); + JITDUMP("emitRemoveJumpToNextInst removed %d jumps and %u bytes\n", totalRemovedCount, totalRemovedSize); } else { diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 8c13cf7922e1d..1e75d0bc41d68 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -2006,6 +2006,7 @@ class emitter instrDescJmp* emitJumpList; // list of local jumps in method instrDescJmp* emitJumpLast; // last of local jumps in method void emitJumpDistBind(); // Bind all the local jumps in method + bool emitContainsCandidateJumpsToNextInst; void emitRemoveJumpToNextInst(); // try to remove unconditional jumps to the next instruction #if FEATURE_LOOP_ALIGN diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 1a8370c33b853..c25ff364df673 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -7498,6 +7498,7 @@ void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0 } #endif // DEBUG + emitContainsCandidateJumpsToNextInst |= isJmpAlways; id->idjIsJmpAlways = isJmpAlways ? 1 : 0; id->idjShort = 0; if (dst != nullptr) From 1700e7077c78a32f79269d39a0fb145643946951 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 27 May 2022 19:27:32 +0100 Subject: [PATCH 11/22] address feedback, formatting --- src/coreclr/jit/emit.cpp | 37 ++++++++++++++++++++----------------- src/coreclr/jit/emit.h | 6 +++--- 2 files changed, 23 insertions(+), 20 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 18d1f154eee7a..87bb11b425375 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -1073,10 +1073,10 @@ void emitter::emitBegFN(bool hasFramePtr emitJumpList = emitJumpLast = nullptr; emitCurIGjmpList = nullptr; - emitFwdJumps = false; - emitNoGCRequestCount = 0; - emitNoGCIG = false; - emitForceNewIG = false; + emitFwdJumps = false; + emitNoGCRequestCount = 0; + emitNoGCIG = false; + emitForceNewIG = false; emitContainsCandidateJumpsToNextInst = false; #if FEATURE_LOOP_ALIGN @@ -4157,11 +4157,11 @@ void emitter::emitRemoveJumpToNextInst() } #endif // DEBUG - int totalRemovedCount = 0; - UNATIVE_OFFSET totalRemovedSize = 0; - instrDescJmp* jmp = emitJumpList; - instrDescJmp* previousJmp = nullptr; + UNATIVE_OFFSET totalRemovedSize = 0; + instrDescJmp* jmp = emitJumpList; + instrDescJmp* previousJmp = nullptr; #if DEBUG + int totalRemovedCount = 0; UNATIVE_OFFSET previousJumpIgNum = (UNATIVE_OFFSET)-1; unsigned int previousJumpInsNum = -1; #endif // DEBUG @@ -4221,7 +4221,8 @@ void emitter::emitRemoveJumpToNextInst() assert(jmp == id); #endif // DEBUG - JITDUMP("IG%02u IN%04x is the last instruction in the group and jumps to the next instruction group IG%02u %s, removing.\n", + JITDUMP("IG%02u IN%04x is the last instruction in the group and jumps to the next instruction group " + "IG%02u %s, removing.\n", jmpGroup->igNum, jmp->idDebugOnlyInfo()->idNum, targetGroup->igNum, emitLabelString(targetGroup)); @@ -4248,7 +4249,9 @@ void emitter::emitRemoveJumpToNextInst() emitTotalCodeSize -= codeSize; totalRemovedSize += codeSize; +#if DEBUG totalRemovedCount += 1; +#endif // DEBUG } else { @@ -4257,18 +4260,18 @@ void emitter::emitRemoveJumpToNextInst() #if DEBUG if (targetGroup == nullptr) { - JITDUMP("IG%02u IN%04x jump target is not set!, keeping.\n", - jmpGroup->igNum,jmp->idDebugOnlyInfo()->idNum ); + JITDUMP("IG%02u IN%04x jump target is not set!, keeping.\n", jmpGroup->igNum, + jmp->idDebugOnlyInfo()->idNum); } else if ((jmpGroup->igFlags & IGF_HAS_ALIGN) != 0) { - JITDUMP("IG%02u IN%04x containing instruction group has alignment, keeping.\n", - jmpGroup->igNum,jmp->idDebugOnlyInfo()->idNum); + JITDUMP("IG%02u IN%04x containing instruction group has alignment, keeping.\n", jmpGroup->igNum, + jmp->idDebugOnlyInfo()->idNum); } else if (jmpGroup->igNext != targetGroup) { - JITDUMP("IG%02u IN%04x does not jump to the next instruction group, keeping.\n", - jmpGroup->igNum,jmp->idDebugOnlyInfo()->idNum); + JITDUMP("IG%02u IN%04x does not jump to the next instruction group, keeping.\n", jmpGroup->igNum, + jmp->idDebugOnlyInfo()->idNum); } #endif // DEBUG } @@ -4279,7 +4282,7 @@ void emitter::emitRemoveJumpToNextInst() previousJmp = jmp; } - if (totalRemovedCount>0) + if (totalRemovedSize > 0) { insGroup* adjOffIG = jmpGroup->igNext; insGroup* adjOffUptoIG = nextJmp != nullptr ? nextJmp->idjIG : emitIGlast; @@ -4295,7 +4298,7 @@ void emitter::emitRemoveJumpToNextInst() jmp = nextJmp; } - if (totalRemovedCount > 0) + if (totalRemovedSize > 0) { #ifdef DEBUG emitCheckIGoffsets(); diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 1e75d0bc41d68..78dc1bc3e01b4 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -2003,9 +2003,9 @@ class emitter insGroup* emitPrologIG; // prolog instruction group - instrDescJmp* emitJumpList; // list of local jumps in method - instrDescJmp* emitJumpLast; // last of local jumps in method - void emitJumpDistBind(); // Bind all the local jumps in method + instrDescJmp* emitJumpList; // list of local jumps in method + instrDescJmp* emitJumpLast; // last of local jumps in method + void emitJumpDistBind(); // Bind all the local jumps in method bool emitContainsCandidateJumpsToNextInst; void emitRemoveJumpToNextInst(); // try to remove unconditional jumps to the next instruction From e05531cfcec126026afafd99ef70e75c009c8e9f Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Wed, 1 Jun 2022 01:47:52 +0100 Subject: [PATCH 12/22] update variable naming and clarify comments --- src/coreclr/jit/codegen.h | 2 +- src/coreclr/jit/codegenlinear.cpp | 6 +++--- src/coreclr/jit/codegenxarch.cpp | 4 ++-- src/coreclr/jit/emit.cpp | 32 +++++++++++++++---------------- src/coreclr/jit/emit.h | 7 ++++--- src/coreclr/jit/emitxarch.cpp | 15 +++++++++------ src/coreclr/jit/emitxarch.h | 2 +- 7 files changed, 35 insertions(+), 33 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index cec1f24c68ed8..8646d9191edcf 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -1473,7 +1473,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX public: void instGen(instruction ins); #if defined(TARGET_XARCH) - void inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock, bool isJmpAlways = false); + void inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock, bool isRemovableJmpCandidate = false); #else void inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock); #endif diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 64c801e0f2441..9cb02a873ea63 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -756,15 +756,15 @@ void CodeGen::genCodeForBBlist() inst_JMP(EJ_jmp, block->bbJumpDest #ifdef TARGET_AMD64 // AMD64 and ARM64 require an instruction after a call instruction for unwinding - // so if the last instruction generated was a call instruction do not allow - // this jump to be marked for possible later removal. + // inside an EH region so if the last instruction generated was a call instruction + // do not allow this jump to be marked for possible later removal. // // If a block has alignment the jump before the alignment can prevent the need // to decode the instructions used for alignment so we want to keep the jump // and it should not be marked for posible later removal. , - /* isJmpAlways */ !GetEmitter()->emitIsLastInsCall() && !block->hasAlign() + /* isRemovableJmpCandidate */ !GetEmitter()->emitIsLastInsCall() && !block->hasAlign() #endif ); FALLTHROUGH; diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 93e6bf1feb752..a75cfeaf33ee7 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -1371,7 +1371,7 @@ void CodeGen::inst_SETCC(GenCondition condition, var_types type, regNumber dstRe //------------------------------------------------------------------------ // inst_JMP: Generate a jump instruction. // -void CodeGen::inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock, bool isJmpAlways) +void CodeGen::inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock, bool isRemovableJmpCandidate) { #if !FEATURE_FIXED_OUT_ARGS // On the x86 we are pushing (and changing the stack level), but on x64 and other archs we have @@ -1389,7 +1389,7 @@ void CodeGen::inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock, bool isJmpAlways) #endif #endif // !FEATURE_FIXED_OUT_ARGS - GetEmitter()->emitIns_J(emitter::emitJumpKindToIns(jmp), tgtBlock, 0, isJmpAlways); + GetEmitter()->emitIns_J(emitter::emitJumpKindToIns(jmp), tgtBlock, 0, isRemovableJmpCandidate); } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 87bb11b425375..23edc26edeec7 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -1073,11 +1073,11 @@ void emitter::emitBegFN(bool hasFramePtr emitJumpList = emitJumpLast = nullptr; emitCurIGjmpList = nullptr; - emitFwdJumps = false; - emitNoGCRequestCount = 0; - emitNoGCIG = false; - emitForceNewIG = false; - emitContainsCandidateJumpsToNextInst = false; + emitFwdJumps = false; + emitNoGCRequestCount = 0; + emitNoGCIG = false; + emitForceNewIG = false; + emitContainsRemovableJmpCandidates = false; #if FEATURE_LOOP_ALIGN /* We don't have any align instructions */ @@ -3874,12 +3874,11 @@ void emitter::emitDispJumpList() unsigned int jmpCount = 0; for (instrDescJmp* jmp = emitJumpList; jmp != nullptr; jmp = jmp->idjNext) { - printf("IG%02u IN%04x %3s[%u] -> IG%02u L_M%03u_" FMT_BB "%s\n", jmp->idjIG->igNum, - jmp->idDebugOnlyInfo()->idNum, codeGen->genInsDisplayName(jmp), jmp->idCodeSize(), - ((insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel))->igNum, emitComp->compMethodID, - jmp->idAddr()->iiaBBlabel->bbNum, + printf("IG%02u IN%04x %3s[%u] -> IG%02u %s\n", jmp->idjIG->igNum, jmp->idDebugOnlyInfo()->idNum, + codeGen->genInsDisplayName(jmp), jmp->idCodeSize(), + ((insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel))->igNum, #if defined(TARGET_XARCH) - jmp->idjIsJmpAlways ? " ; BBJ_ALWAYS" : "" + jmp->idjIsRemovableJmpCandidate ? " ; removal candidate" : "" #else "" #endif @@ -4129,9 +4128,9 @@ void emitter::emitDispCommentForHandle(size_t handle, GenTreeFlags flag) //**************************************************************************** // emitRemoveJumpToNextInst: Checks all jumps in the jump list to see if they are -// unconditional jumps generated at the end of BBJ_ALWAYS basic blocks and whether -// they are now a jump to the next instruction. This can happen when the following -// basic block emitted no instructions. +// unconditional jumps generated marked with idjIsRemovableJmpCandidate which are +// generated at of BBJ_ALWAYS basic blocks. If any candidate is not a jump to the +// next instruction it will be removed from the jump list. // // Assumptions: // the jump list must be ordered by increasing igNum+insNo @@ -4139,7 +4138,7 @@ void emitter::emitDispCommentForHandle(size_t handle, GenTreeFlags flag) void emitter::emitRemoveJumpToNextInst() { #ifdef TARGET_XARCH - if (!emitContainsCandidateJumpsToNextInst) + if (!emitContainsRemovableJmpCandidates) { return; } @@ -4171,8 +4170,7 @@ void emitter::emitRemoveJumpToNextInst() insGroup* jmpGroup = jmp->idjIG; instrDescJmp* nextJmp = jmp->idjNext; - // if the jump is unconditional then it is a candidate - if (jmp->idInsFmt() == IF_LABEL && emitIsUncondJump(jmp) && jmp->idjIsJmpAlways) + if (jmp->idInsFmt() == IF_LABEL && emitIsUncondJump(jmp) && jmp->idjIsRemovableJmpCandidate) { #if DEBUG assert((jmpGroup->igFlags & IGF_HAS_ALIGN) == 0); @@ -4317,7 +4315,7 @@ void emitter::emitRemoveJumpToNextInst() } else { - JITDUMP("emitRemoveJumpToNextInst no unconditional jumps removed\n"); + JITDUMP("emitRemoveJumpToNextInst removed no unconditional jumps\n"); } #endif // TARGET_XARCH } diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 78dc1bc3e01b4..cae8bc7b98c77 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -1541,8 +1541,9 @@ class emitter unsigned idjOffs : #if defined(TARGET_XARCH) 29; - - unsigned idjIsJmpAlways : 1; // indicates that the jump was added at the end of a BBJ_ALWAYS basic block + // indicates that the jump was added at the end of a BBJ_ALWAYS basic block and is + // a candidate for being removed if it jumps to the next instruction + unsigned idjIsRemovableJmpCandidate : 1; #else 30; #endif @@ -2006,7 +2007,7 @@ class emitter instrDescJmp* emitJumpList; // list of local jumps in method instrDescJmp* emitJumpLast; // last of local jumps in method void emitJumpDistBind(); // Bind all the local jumps in method - bool emitContainsCandidateJumpsToNextInst; + bool emitContainsRemovableJmpCandidates; void emitRemoveJumpToNextInst(); // try to remove unconditional jumps to the next instruction #if FEATURE_LOOP_ALIGN diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index c25ff364df673..cac52d15da31e 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -2071,7 +2071,7 @@ inline bool emitter::emitInstHasNoCode(instrDesc* id) return true; } - if ((id->idCodeSize() == 0) && emitIsUncondJump(id) && (((instrDescJmp*)id)->idjIsJmpAlways)) + if ((id->idCodeSize() == 0) && emitIsUncondJump(id) && (((instrDescJmp*)id)->idjIsRemovableJmpCandidate)) { return true; } @@ -7469,7 +7469,10 @@ void emitter::emitSetShortJump(instrDescJmp* id) * to jump: positive is forward, negative is backward. */ -void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0 */, bool isJmpAlways /* = false */) +void emitter::emitIns_J(instruction ins, + BasicBlock* dst, + int instrCount /* = 0 */, + bool isRemovableJmpCandidate /* = false */) { UNATIVE_OFFSET sz; instrDescJmp* id = emitNewInstrJmp(); @@ -7498,9 +7501,9 @@ void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0 } #endif // DEBUG - emitContainsCandidateJumpsToNextInst |= isJmpAlways; - id->idjIsJmpAlways = isJmpAlways ? 1 : 0; - id->idjShort = 0; + emitContainsRemovableJmpCandidates |= isRemovableJmpCandidate; + id->idjIsRemovableJmpCandidate = isRemovableJmpCandidate ? 1 : 0; + id->idjShort = 0; if (dst != nullptr) { /* Assume the jump will be long */ @@ -13687,7 +13690,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) } else { - assert(((instrDescJmp*)id)->idjIsJmpAlways); + assert(((instrDescJmp*)id)->idjIsRemovableJmpCandidate); } sz = (id->idInsFmt() == IF_SWR_LABEL ? sizeof(instrDescLbl) : sizeof(instrDescJmp)); break; diff --git a/src/coreclr/jit/emitxarch.h b/src/coreclr/jit/emitxarch.h index 68b45b0be6f7f..32ecd9a642117 100644 --- a/src/coreclr/jit/emitxarch.h +++ b/src/coreclr/jit/emitxarch.h @@ -304,7 +304,7 @@ inline emitAttr emitDecodeScale(unsigned ensz) /* Output target-independent instructions */ /************************************************************************/ -void emitIns_J(instruction ins, BasicBlock* dst, int instrCount = 0, bool isJmpAlways = false); +void emitIns_J(instruction ins, BasicBlock* dst, int instrCount = 0, bool isRemovableJmpCandidate = false); /************************************************************************/ /* The public entry points to output instructions */ From 9a83e0c9eba9b74454a9aabf6265404eb6e83b70 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Wed, 1 Jun 2022 12:15:07 +0100 Subject: [PATCH 13/22] more feedback --- src/coreclr/jit/codegenlinear.cpp | 2 +- src/coreclr/jit/emit.cpp | 14 +++++--------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 9cb02a873ea63..5d0c0a0e693c5 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -755,7 +755,7 @@ void CodeGen::genCodeForBBlist() case BBJ_ALWAYS: inst_JMP(EJ_jmp, block->bbJumpDest #ifdef TARGET_AMD64 - // AMD64 and ARM64 require an instruction after a call instruction for unwinding + // AMD64 and requirea an instruction after a call instruction for unwinding // inside an EH region so if the last instruction generated was a call instruction // do not allow this jump to be marked for possible later removal. // diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 23edc26edeec7..2b1e5e34ff320 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -4128,9 +4128,9 @@ void emitter::emitDispCommentForHandle(size_t handle, GenTreeFlags flag) //**************************************************************************** // emitRemoveJumpToNextInst: Checks all jumps in the jump list to see if they are -// unconditional jumps generated marked with idjIsRemovableJmpCandidate which are -// generated at of BBJ_ALWAYS basic blocks. If any candidate is not a jump to the -// next instruction it will be removed from the jump list. +// unconditional jumps marked with idjIsRemovableJmpCandidate,generated from +// BBJ_ALWAYS blocks. Any such candidate that jumps to the next instruction +// will be removed from the jump list. // // Assumptions: // the jump list must be ordered by increasing igNum+insNo @@ -4160,7 +4160,6 @@ void emitter::emitRemoveJumpToNextInst() instrDescJmp* jmp = emitJumpList; instrDescJmp* previousJmp = nullptr; #if DEBUG - int totalRemovedCount = 0; UNATIVE_OFFSET previousJumpIgNum = (UNATIVE_OFFSET)-1; unsigned int previousJumpInsNum = -1; #endif // DEBUG @@ -4215,8 +4214,8 @@ void emitter::emitRemoveJumpToNextInst() emitDispIG(jmpGroup, nullptr, true); printf("target group:\n"); emitDispIG(targetGroup, nullptr, false); + assert(jmp == id); } - assert(jmp == id); #endif // DEBUG JITDUMP("IG%02u IN%04x is the last instruction in the group and jumps to the next instruction group " @@ -4247,9 +4246,6 @@ void emitter::emitRemoveJumpToNextInst() emitTotalCodeSize -= codeSize; totalRemovedSize += codeSize; -#if DEBUG - totalRemovedCount += 1; -#endif // DEBUG } else { @@ -4311,7 +4307,7 @@ void emitter::emitRemoveJumpToNextInst() emitDispJumpList(); } #endif // DEBUG - JITDUMP("emitRemoveJumpToNextInst removed %d jumps and %u bytes\n", totalRemovedCount, totalRemovedSize); + JITDUMP("emitRemoveJumpToNextInst removed %u bytes of unconditional jumps\n", totalRemovedSize); } else { From 7167660f3ac2116c1bd152e98ef924e1fdd7d8e4 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Thu, 2 Jun 2022 02:12:28 +0100 Subject: [PATCH 14/22] add last instruction check and group flag set in emitSavIG --- src/coreclr/jit/codegenlinear.cpp | 2 +- src/coreclr/jit/emit.cpp | 22 +++++++++++++++++++--- src/coreclr/jit/emit.h | 1 + 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 5d0c0a0e693c5..2d7f49c0cb725 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -755,7 +755,7 @@ void CodeGen::genCodeForBBlist() case BBJ_ALWAYS: inst_JMP(EJ_jmp, block->bbJumpDest #ifdef TARGET_AMD64 - // AMD64 and requirea an instruction after a call instruction for unwinding + // AMD64 and requires an instruction after a call instruction for unwinding // inside an EH region so if the last instruction generated was a call instruction // do not allow this jump to be marked for possible later removal. // diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 2b1e5e34ff320..c475ef3812e53 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -1006,6 +1006,15 @@ insGroup* emitter::emitSavIG(bool emitAdd) assert(emitLastIns != nullptr); assert(emitCurIGfreeBase <= (BYTE*)emitLastIns); assert((BYTE*)emitLastIns < emitCurIGfreeBase + sz); + +#if defined(TARGET_XARCH) + assert(emitLastIns != nullptr); + if (emitLastIns->idIns() == INS_jmp && (((instrDescJmp*)emitLastIns)->idjIsRemovableJmpCandidate)) + { + ig->igFlags |= IGF_HAS_REMOVABLE_JMP; + } +#endif + emitLastIns = (instrDesc*)((BYTE*)id + ((BYTE*)emitLastIns - (BYTE*)emitCurIGfreeBase)); } @@ -4182,7 +4191,8 @@ void emitter::emitRemoveJumpToNextInst() // target group is not bound yet so use the cookie to fetch it insGroup* targetGroup = (insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel); - if ((targetGroup != nullptr) && (jmpGroup->igNext == targetGroup)) + if ((targetGroup != nullptr) && (jmpGroup->igNext == targetGroup) && + ((jmpGroup->igFlags & IGF_HAS_REMOVABLE_JMP) != 0)) { // the last instruction in the group is the jmp we're looking for // and it jumps to the next instruction group so we don't need it @@ -4206,9 +4216,9 @@ void emitter::emitRemoveJumpToNextInst() { printf("jmp != id, dumping context information\n"); printf("method: %s\n", emitComp->impInlineRoot()->info.compMethodName); - printf(" jmp: %x3: ", jmp->idDebugOnlyInfo()->idNum); + printf(" jmp: %u: ", jmp->idDebugOnlyInfo()->idNum); emitDispIns(jmp, false, true, false, 0, nullptr, 0, jmpGroup); - printf(" id: %x3: ", id->idDebugOnlyInfo()->idNum); + printf(" id: %u: ", id->idDebugOnlyInfo()->idNum); emitDispIns(id, false, true, false, 0, nullptr, 0, jmpGroup); printf("jump group:\n"); emitDispIG(jmpGroup, nullptr, true); @@ -4267,6 +4277,12 @@ void emitter::emitRemoveJumpToNextInst() JITDUMP("IG%02u IN%04x does not jump to the next instruction group, keeping.\n", jmpGroup->igNum, jmp->idDebugOnlyInfo()->idNum); } + else if ((jmpGroup->igFlags & IGF_HAS_REMOVABLE_JMP) == 0) + { + JITDUMP("IG%02u IN%04x containing instruction group is not marked with IGF_HAS_REMOVABLE_JMP, " + "keeping.\n", + jmpGroup->igNum, jmp->idDebugOnlyInfo()->idNum); + } #endif // DEBUG } } diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index cae8bc7b98c77..5f48c0063fcbd 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -279,6 +279,7 @@ struct insGroup // IG, or, if this IG contains with an unconditional branch, some subsequent IG. #define IGF_REMOVED_ALIGN 0x0800 // IG was marked as having an alignment instruction(s), but was later unmarked // without updating the IG's size/offsets. +#define IGF_HAS_REMOVABLE_JMP 0x1000 // this group ends with an unconditional jump which is a candidate for removal // Mask of IGF_* flags that should be propagated to new blocks when they are created. // This allows prologs and epilogs to be any number of IGs, but still be From ad2d38a891d81c4c338c4f47d61157880d68cf25 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 3 Jun 2022 01:57:12 +0100 Subject: [PATCH 15/22] more feedback --- src/coreclr/jit/emit.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index c475ef3812e53..f9272fdcd43d4 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -1009,7 +1009,7 @@ insGroup* emitter::emitSavIG(bool emitAdd) #if defined(TARGET_XARCH) assert(emitLastIns != nullptr); - if (emitLastIns->idIns() == INS_jmp && (((instrDescJmp*)emitLastIns)->idjIsRemovableJmpCandidate)) + if (emitLastIns->idIns() == INS_jmp) { ig->igFlags |= IGF_HAS_REMOVABLE_JMP; } @@ -4191,8 +4191,9 @@ void emitter::emitRemoveJumpToNextInst() // target group is not bound yet so use the cookie to fetch it insGroup* targetGroup = (insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel); - if ((targetGroup != nullptr) && (jmpGroup->igNext == targetGroup) && - ((jmpGroup->igFlags & IGF_HAS_REMOVABLE_JMP) != 0)) + assert(targetGroup != nullptr); + + if ((jmpGroup->igNext == targetGroup) && ((jmpGroup->igFlags & IGF_HAS_REMOVABLE_JMP) != 0)) { // the last instruction in the group is the jmp we're looking for // and it jumps to the next instruction group so we don't need it From 84d5dc34e24af5b3a584bcd848f3af85762fdce8 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 3 Jun 2022 14:24:05 +0100 Subject: [PATCH 16/22] fix jitdump output and make removal candidate printing optional --- src/coreclr/jit/emit.cpp | 19 +++++++++++++++---- src/coreclr/jit/emit.h | 2 +- src/coreclr/jit/emitxarch.cpp | 16 +++++++++++++++- 3 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index f9272fdcd43d4..8dd1f8b4d6aaf 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -3821,10 +3821,21 @@ void emitter::emitDispIG(insGroup* ig, insGroup* igPrev, bool verbose) { instrDesc* id = (instrDesc*)ins; +#if defined(TARGET_XARCH) + if (emitInstHasNoCode(id)) + { + // an instruction with no code prevents us being able to iterate to the + // next instructions so we must be certain that when we find one it is + // the last instruction in a group + assert(cnt == 1); + break; + } +#endif emitDispIns(id, false, true, false, ofs, nullptr, 0, ig); ins += emitSizeOfInsDsc(id); ofs += id->idCodeSize(); + } while (--cnt); printf("\n"); @@ -3877,7 +3888,7 @@ void emitter::emitDispGCinfo() //------------------------------------------------------------------------ // emitDispJumpList: displays the current emitter jump list // -void emitter::emitDispJumpList() +void emitter::emitDispJumpList(bool showJumpRemovalCndidates) { printf("Emitter Jump List:\n"); unsigned int jmpCount = 0; @@ -3887,7 +3898,7 @@ void emitter::emitDispJumpList() codeGen->genInsDisplayName(jmp), jmp->idCodeSize(), ((insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel))->igNum, #if defined(TARGET_XARCH) - jmp->idjIsRemovableJmpCandidate ? " ; removal candidate" : "" + (showJumpRemovalCndidates && jmp->idjIsRemovableJmpCandidate) ? " ; removal candidate" : "" #else "" #endif @@ -4161,7 +4172,7 @@ void emitter::emitRemoveJumpToNextInst() } if (EMITVERBOSE) { - emitDispJumpList(); + emitDispJumpList(/*showJumpRemovalCndidates = */ true); } #endif // DEBUG @@ -4321,7 +4332,7 @@ void emitter::emitRemoveJumpToNextInst() } if (EMITVERBOSE) { - emitDispJumpList(); + emitDispJumpList(/*showJumpRemovalCndidates = */ true); } #endif // DEBUG JITDUMP("emitRemoveJumpToNextInst removed %u bytes of unconditional jumps\n", totalRemovedSize); diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 5f48c0063fcbd..086d83bb583ed 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -1732,7 +1732,7 @@ class emitter void emitDispIG(insGroup* ig, insGroup* igPrev = nullptr, bool verbose = false); void emitDispIGlist(bool verbose = false); void emitDispGCinfo(); - void emitDispJumpList(); + void emitDispJumpList(bool showJumpRemovalCndidates = false); void emitDispClsVar(CORINFO_FIELD_HANDLE fldHnd, ssize_t offs, bool reloc = false); void emitDispFrameRef(int varx, int disp, int offs, bool asmfm); void emitDispInsAddr(BYTE* code); diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index cac52d15da31e..00d5fa543b2e2 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -14714,7 +14714,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) assert(*dp != dst || emitInstHasNoCode(id)); #ifdef DEBUG - if (emitComp->opts.disAsm || emitComp->verbose) + if ((emitComp->opts.disAsm || emitComp->verbose) && !emitInstHasNoCode(id)) { emitDispIns(id, false, dspOffs, true, emitCurCodeOffs(*dp), *dp, (dst - *dp)); } @@ -15487,6 +15487,20 @@ emitter::insExecutionCharacteristics emitter::getInsExecutionCharacteristics(ins break; case INS_jmp: + if (emitInstHasNoCode(id)) + { + // a removed jmp to the next instruction + result.insThroughput = PERFSCORE_THROUGHPUT_ZERO; + result.insLatency = PERFSCORE_LATENCY_ZERO; + } + else + { + // branch to a constant address + result.insThroughput = PERFSCORE_THROUGHPUT_2C; + result.insLatency = PERFSCORE_LATENCY_BRANCH_DIRECT; + } + break; + case INS_l_jmp: // branch to a constant address result.insThroughput = PERFSCORE_THROUGHPUT_2C; From 23d6c35be9474a7cb886cb06a8e23c3036e92689 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 3 Jun 2022 14:43:23 +0100 Subject: [PATCH 17/22] remove emitDispJumpList parameter --- src/coreclr/jit/emit.cpp | 8 ++++---- src/coreclr/jit/emit.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 8dd1f8b4d6aaf..f75b710557edc 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -3888,7 +3888,7 @@ void emitter::emitDispGCinfo() //------------------------------------------------------------------------ // emitDispJumpList: displays the current emitter jump list // -void emitter::emitDispJumpList(bool showJumpRemovalCndidates) +void emitter::emitDispJumpList() { printf("Emitter Jump List:\n"); unsigned int jmpCount = 0; @@ -3898,7 +3898,7 @@ void emitter::emitDispJumpList(bool showJumpRemovalCndidates) codeGen->genInsDisplayName(jmp), jmp->idCodeSize(), ((insGroup*)emitCodeGetCookie(jmp->idAddr()->iiaBBlabel))->igNum, #if defined(TARGET_XARCH) - (showJumpRemovalCndidates && jmp->idjIsRemovableJmpCandidate) ? " ; removal candidate" : "" + jmp->idjIsRemovableJmpCandidate ? " ; removal candidate" : "" #else "" #endif @@ -4172,7 +4172,7 @@ void emitter::emitRemoveJumpToNextInst() } if (EMITVERBOSE) { - emitDispJumpList(/*showJumpRemovalCndidates = */ true); + emitDispJumpList(); } #endif // DEBUG @@ -4332,7 +4332,7 @@ void emitter::emitRemoveJumpToNextInst() } if (EMITVERBOSE) { - emitDispJumpList(/*showJumpRemovalCndidates = */ true); + emitDispJumpList(); } #endif // DEBUG JITDUMP("emitRemoveJumpToNextInst removed %u bytes of unconditional jumps\n", totalRemovedSize); diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 086d83bb583ed..5f48c0063fcbd 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -1732,7 +1732,7 @@ class emitter void emitDispIG(insGroup* ig, insGroup* igPrev = nullptr, bool verbose = false); void emitDispIGlist(bool verbose = false); void emitDispGCinfo(); - void emitDispJumpList(bool showJumpRemovalCndidates = false); + void emitDispJumpList(); void emitDispClsVar(CORINFO_FIELD_HANDLE fldHnd, ssize_t offs, bool reloc = false); void emitDispFrameRef(int varx, int disp, int offs, bool asmfm); void emitDispInsAddr(BYTE* code); From 511397c45610934a6ab0acbfe740803860171374 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 3 Jun 2022 18:43:48 +0100 Subject: [PATCH 18/22] change overall size calculation and allow 0 size align to be printed in dumps --- src/coreclr/jit/emit.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index f75b710557edc..ceffb451dee78 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -3822,7 +3822,7 @@ void emitter::emitDispIG(insGroup* ig, insGroup* igPrev, bool verbose) instrDesc* id = (instrDesc*)ins; #if defined(TARGET_XARCH) - if (emitInstHasNoCode(id)) + if ((id->idIns() != INS_jmp) && emitInstHasNoCode(id)) { // an instruction with no code prevents us being able to iterate to the // next instructions so we must be certain that when we find one it is @@ -6863,7 +6863,7 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, #ifdef DEBUG if (emitComp->opts.disAsm || emitComp->verbose) { - printf("\t\t\t\t\t\t;; size=%d bbWeight=%s PerfScore %.2f", ig->igSize, refCntWtd2str(ig->igWeight), + printf("\t\t\t\t\t\t;; size=%d bbWeight=%s PerfScore %.2f", (cp - bp), refCntWtd2str(ig->igWeight), ig->igPerfScore); } *instrCount += ig->igInsCnt; From a5a57580da0907ceeca4fd7fe041d281ecaa9621 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Fri, 3 Jun 2022 20:25:25 +0100 Subject: [PATCH 19/22] fix invorrect jmp check, remove inline for emitInstHasNoCode --- src/coreclr/jit/emit.cpp | 4 ++-- src/coreclr/jit/emitxarch.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index ceffb451dee78..1863de3525222 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -3821,8 +3821,8 @@ void emitter::emitDispIG(insGroup* ig, insGroup* igPrev, bool verbose) { instrDesc* id = (instrDesc*)ins; -#if defined(TARGET_XARCH) - if ((id->idIns() != INS_jmp) && emitInstHasNoCode(id)) +#ifdef TARGET_XARCH + if ((id->idIns() == INS_jmp) && emitInstHasNoCode(id)) { // an instruction with no code prevents us being able to iterate to the // next instructions so we must be certain that when we find one it is diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 00d5fa543b2e2..90ce509f3fa33 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -2064,7 +2064,7 @@ const emitJumpKind emitReverseJumpKinds[] = { * but the target register need not be byte-addressable */ -inline bool emitter::emitInstHasNoCode(instrDesc* id) +bool emitter::emitInstHasNoCode(instrDesc* id) { if (id->idIns() == INS_align) { From 40ddb6c41dd3c76862f1432f6179f1cab21d679c Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Sat, 4 Jun 2022 11:47:52 +0100 Subject: [PATCH 20/22] allow alignment printing even when zero --- src/coreclr/jit/codegenlinear.cpp | 2 +- src/coreclr/jit/emit.cpp | 7 +++- src/coreclr/jit/emit.h | 4 ++- src/coreclr/jit/emitxarch.cpp | 55 ++++++++++++++++++++++--------- 4 files changed, 49 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 2d7f49c0cb725..43894e1e3b243 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -754,7 +754,7 @@ void CodeGen::genCodeForBBlist() case BBJ_ALWAYS: inst_JMP(EJ_jmp, block->bbJumpDest -#ifdef TARGET_AMD64 +#ifdef TARGET_XARCH // AMD64 and requires an instruction after a call instruction for unwinding // inside an EH region so if the last instruction generated was a call instruction // do not allow this jump to be marked for possible later removal. diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 1863de3525222..892f8cbf93119 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -3822,7 +3822,7 @@ void emitter::emitDispIG(insGroup* ig, insGroup* igPrev, bool verbose) instrDesc* id = (instrDesc*)ins; #ifdef TARGET_XARCH - if ((id->idIns() == INS_jmp) && emitInstHasNoCode(id)) + if (emitJmpInstHasNoCode(id)) { // an instruction with no code prevents us being able to iterate to the // next instructions so we must be certain that when we find one it is @@ -6735,6 +6735,11 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, emitCurIG = ig; + if (ig->igNum == 5) + { + printf("hammer time\n"); + } + for (unsigned cnt = ig->igInsCnt; cnt > 0; cnt--) { #ifdef DEBUG diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index 5f48c0063fcbd..02911cc8b8d3a 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -2142,7 +2142,9 @@ class emitter #endif // !defined(JIT32_GCENCODER) #if defined(TARGET_XARCH) - bool emitInstHasNoCode(instrDesc* id); + static bool emitAlignInstHasNoCode(instrDesc* id); + static bool emitInstHasNoCode(instrDesc* id); + static bool emitJmpInstHasNoCode(instrDesc* id); #endif void emitGenIG(insGroup* ig); diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 90ce509f3fa33..6594836fd7de8 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -2059,24 +2059,47 @@ const emitJumpKind emitReverseJumpKinds[] = { return emitReverseJumpKinds[jumpKind]; } -/***************************************************************************** - * The size for these instructions is less than EA_4BYTE, - * but the target register need not be byte-addressable - */ +//------------------------------------------------------------------------ +// emitAlignInstHasNoCode: Returns true if the 'id' is an align instruction +// that was later removed and hence has codeSize==0. +// +// Arguments: +// id -- The instruction to check +// +/* static */ bool emitter::emitAlignInstHasNoCode(instrDesc* id) +{ + return (id->idIns() == INS_align) && (id->idCodeSize() == 0); +} -bool emitter::emitInstHasNoCode(instrDesc* id) +//------------------------------------------------------------------------ +// emitJmpInstHasNoCode: Returns true if the 'id' is a jump instruction +// that was later removed and hence has codeSize==0. +// +// Arguments: +// id -- The instruction to check +// +/* static */ bool emitter::emitJmpInstHasNoCode(instrDesc* id) { - if (id->idIns() == INS_align) - { - return true; - } + bool result = (id->idIns() == INS_jmp) && (id->idCodeSize() == 0); - if ((id->idCodeSize() == 0) && emitIsUncondJump(id) && (((instrDescJmp*)id)->idjIsRemovableJmpCandidate)) - { - return true; - } + // A zero size jump instruction can only be the one that is marked + // as removable candidate. + assert(!result || ((instrDescJmp*)id)->idjIsRemovableJmpCandidate); - return false; + return result; +} + +//------------------------------------------------------------------------ +// emitInstHasNoCode: Returns true if the 'id' is an instruction +// that was later removed and hence has codeSize==0. +// Currently it is one of `align` or `jmp`. +// +// Arguments: +// id -- The instruction to check +// +/* static */ bool emitter::emitInstHasNoCode(instrDesc* id) +{ + return emitAlignInstHasNoCode(id) || emitJmpInstHasNoCode(id); } /***************************************************************************** @@ -13681,7 +13704,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) case IF_RWR_LABEL: case IF_SWR_LABEL: assert(id->idGCref() == GCT_NONE); - assert(id->idIsBound() || emitInstHasNoCode(id)); + assert(id->idIsBound() || emitJmpInstHasNoCode(id)); // TODO-XArch-Cleanup: handle IF_RWR_LABEL in emitOutputLJ() or change it to emitOutputAM()? if (id->idCodeSize() != 0) @@ -14714,7 +14737,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) assert(*dp != dst || emitInstHasNoCode(id)); #ifdef DEBUG - if ((emitComp->opts.disAsm || emitComp->verbose) && !emitInstHasNoCode(id)) + if ((emitComp->opts.disAsm || emitComp->verbose) && !emitJmpInstHasNoCode(id)) { emitDispIns(id, false, dspOffs, true, emitCurCodeOffs(*dp), *dp, (dst - *dp)); } From 1fb11ebe7f170899d6f47f88393ffbbee6389886 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Sat, 4 Jun 2022 16:56:55 +0100 Subject: [PATCH 21/22] remove debug printf and fix x86 --- src/coreclr/jit/codegenlinear.cpp | 9 +++++++-- src/coreclr/jit/emit.cpp | 5 ----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 43894e1e3b243..6c0a37b6286f1 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -754,7 +754,7 @@ void CodeGen::genCodeForBBlist() case BBJ_ALWAYS: inst_JMP(EJ_jmp, block->bbJumpDest -#ifdef TARGET_XARCH +#ifdef TARGET_AMD64 // AMD64 and requires an instruction after a call instruction for unwinding // inside an EH region so if the last instruction generated was a call instruction // do not allow this jump to be marked for possible later removal. @@ -762,9 +762,14 @@ void CodeGen::genCodeForBBlist() // If a block has alignment the jump before the alignment can prevent the need // to decode the instructions used for alignment so we want to keep the jump // and it should not be marked for posible later removal. - , /* isRemovableJmpCandidate */ !GetEmitter()->emitIsLastInsCall() && !block->hasAlign() +#else +#ifdef TARGET_XARCH + , + /* isRemovableJmpCandidate */ !block->hasAlign() +#endif + #endif ); FALLTHROUGH; diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 892f8cbf93119..962ffecc60596 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -6735,11 +6735,6 @@ unsigned emitter::emitEndCodeGen(Compiler* comp, emitCurIG = ig; - if (ig->igNum == 5) - { - printf("hammer time\n"); - } - for (unsigned cnt = ig->igInsCnt; cnt > 0; cnt--) { #ifdef DEBUG From eda64b5c2376ec275a70c62b2b97c8866ab3ac67 Mon Sep 17 00:00:00 2001 From: Wraith2 Date: Mon, 6 Jun 2022 17:24:48 +0100 Subject: [PATCH 22/22] address feedback --- src/coreclr/jit/codegenarmarch.cpp | 12 ------------ src/coreclr/jit/codegenlinear.cpp | 7 +++---- src/coreclr/jit/emit.cpp | 5 +++-- src/coreclr/jit/emitxarch.cpp | 4 +--- 4 files changed, 7 insertions(+), 21 deletions(-) diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index 0d3bf4b2db4c7..edec9652fe9fa 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -4307,19 +4307,7 @@ void CodeGen::inst_SETCC(GenCondition condition, var_types type, regNumber dstRe void CodeGen::inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock) { #if !FEATURE_FIXED_OUT_ARGS - // On the x86 we are pushing (and changing the stack level), but on x64 and other archs we have - // a fixed outgoing args area that we store into and we never change the stack level when calling methods. - // - // Thus only on x86 do we need to assert that the stack level at the target block matches the current stack level. - // - CLANG_FORMAT_COMMENT_ANCHOR; - -#ifdef UNIX_X86_ABI - // bbTgtStkDepth is a (pure) argument count (stack alignment padding should be excluded). - assert((tgtBlock->bbTgtStkDepth * sizeof(int) == (genStackLevel - curNestedAlignment)) || isFramePointerUsed()); -#else assert((tgtBlock->bbTgtStkDepth * sizeof(int) == genStackLevel) || isFramePointerUsed()); -#endif #endif // !FEATURE_FIXED_OUT_ARGS GetEmitter()->emitIns_J(emitter::emitJumpKindToIns(jmp), tgtBlock); diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 6c0a37b6286f1..c2638c32daa28 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -755,13 +755,12 @@ void CodeGen::genCodeForBBlist() case BBJ_ALWAYS: inst_JMP(EJ_jmp, block->bbJumpDest #ifdef TARGET_AMD64 - // AMD64 and requires an instruction after a call instruction for unwinding + // AMD64 requires an instruction after a call instruction for unwinding // inside an EH region so if the last instruction generated was a call instruction // do not allow this jump to be marked for possible later removal. // - // If a block has alignment the jump before the alignment can prevent the need - // to decode the instructions used for alignment so we want to keep the jump - // and it should not be marked for posible later removal. + // If a block was selected to place an alignment instruction because it ended + // with a jump, do not remove jumps from such blocks. , /* isRemovableJmpCandidate */ !GetEmitter()->emitIsLastInsCall() && !block->hasAlign() #else diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 962ffecc60596..b3896945c9882 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -4320,9 +4320,9 @@ void emitter::emitRemoveJumpToNextInst() jmp = nextJmp; } +#ifdef DEBUG if (totalRemovedSize > 0) { -#ifdef DEBUG emitCheckIGoffsets(); if (EMIT_INSTLIST_VERBOSE) @@ -4334,13 +4334,14 @@ void emitter::emitRemoveJumpToNextInst() { emitDispJumpList(); } -#endif // DEBUG + JITDUMP("emitRemoveJumpToNextInst removed %u bytes of unconditional jumps\n", totalRemovedSize); } else { JITDUMP("emitRemoveJumpToNextInst removed no unconditional jumps\n"); } +#endif // DEBUG #endif // TARGET_XARCH } diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 6594836fd7de8..cac2e12e70bfd 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -9951,7 +9951,6 @@ void emitter::emitDispIns( { printf("L_M%03u_" FMT_BB, emitComp->compMethodID, id->idAddr()->iiaBBlabel->bbNum); } - break; case IF_METHOD: @@ -14731,8 +14730,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp) assert((int)emitCurStackLvl >= 0); - // Only epilog "instructions", some pseudo-instrs and block ending jumps - // are allowed not to generate any code + // Only epilog "instructions", some pseudo-instrs and blocks that ends with a jump to the next block assert(*dp != dst || emitInstHasNoCode(id));