Skip to content

Remove unneeded unconditional jumps for Arm/Arm64 #70389

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Closed
kunalspathak wants to merge 4 commits into from
+198 −151
Closed

Remove unneeded unconditional jumps for Arm/Arm64 #70389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
40f8c2c
Extend jmp removal for arm64
kunalspathak Jun 7, 2022
d759665
fix build failure
kunalspathak Jun 7, 2022
3e1e37f
fallthrough
kunalspathak Jun 7, 2022
8de8abe
add one more missing fallthrough
kunalspathak Jun 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions src/coreclr/jit/codegen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1472,12 +1472,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX

public:
void instGen(instruction ins);
#if defined(TARGET_XARCH)
void inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock, bool isRemovableJmpCandidate = false);
#else
void inst_JMP(emitJumpKind jmp, BasicBlock* tgtBlock);
#endif

void inst_SET(emitJumpKind condition, regNumber reg);

void inst_RV(instruction ins, regNumber reg, var_types type, emitAttr size = EA_UNKNOWN);
Expand Down
12 changes: 0 additions & 12 deletions src/coreclr/jit/codegenarmarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4288,18 +4288,6 @@ 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
assert((tgtBlock->bbTgtStkDepth * sizeof(int) == genStackLevel) || isFramePointerUsed());
#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.
//
Expand Down
7 changes: 2 additions & 5 deletions src/coreclr/jit/codegenlinear.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -754,7 +754,7 @@ void CodeGen::genCodeForBBlist()

case BBJ_ALWAYS:
inst_JMP(EJ_jmp, block->bbJumpDest
#ifdef TARGET_AMD64
#if defined(TARGET_AMD64)
// 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.
Expand All @@ -764,12 +764,9 @@ void CodeGen::genCodeForBBlist()
,
/* isRemovableJmpCandidate */ !GetEmitter()->emitIsLastInsCall() && !block->hasAlign()
#else
#ifdef TARGET_XARCH
,
/* isRemovableJmpCandidate */ !block->hasAlign()
#endif

#endif
#endif // defined(TARGET_AMD64)
);
FALLTHROUGH;

Expand Down
24 changes: 0 additions & 24 deletions src/coreclr/jit/codegenxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1436,30 +1436,6 @@ 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 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
// 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, isRemovableJmpCandidate);
}

//------------------------------------------------------------------------
// genCodeForReturnTrap: Produce code for a GT_RETURNTRAP node.
//
Expand Down
39 changes: 23 additions & 16 deletions src/coreclr/jit/emit.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1006,14 +1006,19 @@ 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)

#ifdef TARGET_XARCH
instruction jmpInstr = INS_jmp;
#else // TARGET_ARM64
instruction jmpInstr = INS_b;
#endif
Comment on lines +1011 to +1015
Copy link
Contributor

@MichalPetryka MichalPetryka Jun 7, 2022

Choose a reason for hiding this comment

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

Won't the assumption that it's either ARM or XARCH be invalid on LoongArch64 builds?

Copy link
Contributor Author

@kunalspathak kunalspathak Jun 7, 2022

Choose a reason for hiding this comment

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

they will be and I need to do fix/validate it. This is draft and I would like to get a sense of its state before I spend time on other archs.


if (emitLastIns->idIns() == jmpInstr)

{
ig->igFlags |= IGF_HAS_REMOVABLE_JMP;
}
#endif

emitLastIns = (instrDesc*)((BYTE*)id + ((BYTE*)emitLastIns - (BYTE*)emitCurIGfreeBase));
}
Expand Down Expand Up @@ -3821,7 +3826,6 @@ void emitter::emitDispIG(insGroup* ig, insGroup* igPrev, bool verbose)
{
instrDesc* id = (instrDesc*)ins;

#ifdef TARGET_XARCH
if (emitJmpInstHasNoCode(id))
{
// an instruction with no code prevents us being able to iterate to the
Expand All @@ -3830,12 +3834,10 @@ void emitter::emitDispIG(insGroup* ig, insGroup* igPrev, bool verbose)
assert(cnt == 1);
break;
}
#endif
emitDispIns(id, false, true, false, ofs, nullptr, 0, ig);

ins += emitSizeOfInsDsc(id);
ofs += id->idCodeSize();

} while (--cnt);

printf("\n");
Expand Down Expand Up @@ -3897,12 +3899,7 @@ void emitter::emitDispJumpList()
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->idjIsRemovableJmpCandidate ? " ; removal candidate" : ""
#else
""
#endif
);
jmp->idjIsRemovableJmpCandidate ? " ; removal candidate" : "");
jmpCount += 1;
}
printf(" total jump count: %u\n", jmpCount);
Expand Down Expand Up @@ -4157,7 +4154,6 @@ void emitter::emitDispCommentForHandle(size_t handle, GenTreeFlags flag)
//
void emitter::emitRemoveJumpToNextInst()
{
#ifdef TARGET_XARCH
if (!emitContainsRemovableJmpCandidates)
{
return;
Expand Down Expand Up @@ -4189,8 +4185,16 @@ void emitter::emitRemoveJumpToNextInst()
insGroup* jmpGroup = jmp->idjIG;
instrDescJmp* nextJmp = jmp->idjNext;

if (jmp->idInsFmt() == IF_LABEL && emitIsUncondJump(jmp) && jmp->idjIsRemovableJmpCandidate)
if (
#ifdef TARGET_XARCH
jmp->idInsFmt() == IF_LABEL &&
#endif
emitIsUncondJump(jmp) && jmp->idjIsRemovableJmpCandidate)
{
#ifdef TARGET_ARMARCH
assert(jmp->idInsOpt() == INS_OPTS_JMP);
#endif

#if DEBUG
assert((jmpGroup->igFlags & IGF_HAS_ALIGN) == 0);
assert((jmpGroup->igNum > previousJumpIgNum) || (previousJumpIgNum == (UNATIVE_OFFSET)-1) ||
Expand Down Expand Up @@ -4261,7 +4265,11 @@ void emitter::emitRemoveJumpToNextInst()
}

UNATIVE_OFFSET codeSize = jmp->idCodeSize();
#ifdef TARGET_XARCH
jmp->idCodeSize(0);
#else
jmp->idInsOpt(INS_OPTS_NONE);
#endif

jmpGroup->igSize -= (unsigned short)codeSize;
jmpGroup->igFlags |= IGF_UPD_ISZ;
Expand Down Expand Up @@ -4342,7 +4350,6 @@ void emitter::emitRemoveJumpToNextInst()
JITDUMP("emitRemoveJumpToNextInst removed no unconditional jumps\n");
}
#endif // DEBUG
#endif // TARGET_XARCH
}

/*****************************************************************************
Expand Down
82 changes: 70 additions & 12 deletions src/coreclr/jit/emit.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -983,6 +983,12 @@ class emitter
return (idIns() == INS_align) && (idInsOpt() == INS_OPTS_NONE);
}

inline bool idIsEmptyJmp() const
{
return (idIns() == INS_b) && (((instrDescJmp*)this)->idjIsRemovableJmpCandidate) &&
(idInsOpt() == INS_OPTS_NONE);
}

unsigned idCodeSize() const
{
int size = 4;
Expand Down Expand Up @@ -1012,6 +1018,12 @@ class emitter
size = 0;
}
break;
case IF_BI_0A:
if (idIsEmptyJmp())
{
size = 0;
}
FALLTHROUGH;
default:
break;
}
Expand Down Expand Up @@ -1539,15 +1551,10 @@ class emitter
// 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 idjOffs : 29;
// 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
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)
};
Expand Down Expand Up @@ -2137,19 +2144,70 @@ class emitter
void emitInitIG(insGroup* ig);
void emitInsertIGAfter(insGroup* insertAfterIG, insGroup* ig);

#if defined(TARGET_XARCH)
//------------------------------------------------------------------------
// 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
//
inline bool emitAlignInstHasNoCode(instrDesc* id)
{
return (id->idIns() == INS_align) && (id->idCodeSize() == 0);
}

//------------------------------------------------------------------------
// 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
//
inline bool emitJmpInstHasNoCode(instrDesc* id)
{
bool result = (id->idIns() == INS_jmp) && (id->idCodeSize() == 0);

// A zero size jump instruction can only be the one that is marked
// as removable candidate.
assert(!result || ((instrDescJmp*)id)->idjIsRemovableJmpCandidate);

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
inline bool emitInstHasNoCode(instrDesc* id)
{
return emitAlignInstHasNoCode(id) || emitJmpInstHasNoCode(id);
}
#else
//------------------------------------------------------------------------
// emitJmpInstHasNoCode: Returns true if the 'id' is a 'branch' instruction
// that was later removed
//
// Arguments:
// id -- The instruction to check
//
inline bool emitJmpInstHasNoCode(instrDesc* id)
{
return emitIsUncondJump(id) && (((instrDescJmp*)id)->idjIsRemovableJmpCandidate) &&
(id->idInsOpt() == INS_OPTS_NONE);
}
#endif

void emitNewIG();

#if !defined(JIT32_GCENCODER)
void emitDisableGC();
void emitEnableGC();
#endif // !defined(JIT32_GCENCODER)

#if defined(TARGET_XARCH)
static bool emitAlignInstHasNoCode(instrDesc* id);
static bool emitInstHasNoCode(instrDesc* id);
static bool emitJmpInstHasNoCode(instrDesc* id);
#endif

void emitGenIG(insGroup* ig);
insGroup* emitSavIG(bool emitAdd = false);
void emitNxtIG(bool extend = false);
Expand Down
35 changes: 29 additions & 6 deletions src/coreclr/jit/emitarm.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4334,7 +4334,10 @@ 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 isRemovableJmpCandidate /* = false */)
{
insFormat fmt = IF_NONE;

Expand Down Expand Up @@ -4394,6 +4397,18 @@ void emitter::emitIns_J(instruction ins, BasicBlock* dst, int instrCount /* = 0
/* Assume the jump will be long */

id->idjShort = 0;
emitContainsRemovableJmpCandidates |= isRemovableJmpCandidate;

if (isRemovableJmpCandidate)
{
id->idInsOpt(INS_OPTS_JMP);
id->idjIsRemovableJmpCandidate = 1;
}
else
{
id->idjIsRemovableJmpCandidate = 0;
}

if (dst != NULL)
{
id->idAddr()->iiaBBlabel = dst;
Expand Down Expand Up @@ -6406,10 +6421,18 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp)
case IF_T2_N1: // T2_N .....i......iiii .iiiddddiiiiiiii R1 imm16
case IF_LARGEJMP:
assert(id->idGCref() == GCT_NONE);
assert(id->idIsBound());

dst = emitOutputLJ(ig, dst, id);
sz = sizeof(instrDescJmp);
if (!emitJmpInstHasNoCode(id))
{
assert(id->idIsBound());
dst = emitOutputLJ(ig, dst, id);
}
else
{
assert(((instrDescJmp*)id)->idjIsRemovableJmpCandidate);
}

sz = sizeof(instrDescJmp);
break;

case IF_T1_D1: // T1_D1 .........mmmm... R1*
Expand Down Expand Up @@ -6661,7 +6684,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp)
size_t expected = emitSizeOfInsDsc(id);
assert(sz == expected);

if (emitComp->opts.disAsm || emitComp->verbose)
if ((emitComp->opts.disAsm || emitComp->verbose) && !emitJmpInstHasNoCode(id))
{
emitDispIns(id, false, dspOffs, true, emitCurCodeOffs(odst), *dp, (dst - *dp), ig);
}
Expand Down Expand Up @@ -6700,7 +6723,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp)

/* All instructions are expected to generate code */

assert(*dp != dst);
assert(*dp != dst || emitJmpInstHasNoCode(id));

*dp = dst;

Expand Down
6 changes: 0 additions & 6 deletions src/coreclr/jit/emitarm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,12 +192,6 @@ 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 */
/************************************************************************/
Expand Down
Loading