Fix #70849: Print LARGEJMP instruction on ARM64 #71094
Conversation
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThese changes address #70849 by implementing the special case of printing the LARGEJMP pseudo-instruction in
|
|
@BruceForstall PTAL. I thought I'd tackle this in my spare time between the ARM64 hot/cold splitting PR and getting started on crossgen2. Thanks! |
| pidJmp->idIns(INS_b); | ||
| pidJmp->idInsFmt(IF_T2_J2); | ||
| pidJmp->idInsSize(emitInsSize(IF_T2_J2)); | ||
| pidJmp->idjShort = 0; |
There was a problem hiding this comment.
This is set to 1 above and then 0 here. Are both needed? Does pidJmp point to a different piece of memory in these cases?
There was a problem hiding this comment.
idjShort is initially set to 1 to satisfy an assert in emitDispInsHelp , as we emit a short conditional branch first for the pseudo-instruction. While the following unconditional branch is long (thus suggesting idjShort should be 0), I don't see any asserts in its control flow through emitDispInsHelp that require idjShort == 0, though that could change in the future.
This looks weird because pidJmp is used to emit both the short conditional and the long unconditional branches, but I think the logic is correct.
There was a problem hiding this comment.
The memory pointed to gets zeroed between uses (so zeroing isn't required, perhaps, but in this case it's best to be clear and have it).
There was a problem hiding this comment.
I agree, clarity is important. It might be worth a comment to explain that two different things are being done through the ptr but it's not a problem without.
ghost
added
the
needs-author-action
emitter::emitDispIns supports printing LARGEJMP pseudo-instructions on ARM32, but not on ARM64. This method has been re-architected on ARM64 to support printing LARGEJMP instructions on ARM64.
amanasifkhalid
force-pushed
the
emit-disp-largejmp
branch
from
7645db7
to
27b79c7
Compare
ghost
removed
the
needs-author-action
Sure thing. Consider the following (large) conditional branch: The JIT converts this instruction to a "jump stub" by inverting the condition and emitting a short branch, followed by an unconditional branch to the original target. Previously, (note the missing register argument, and no hex encoding of the instruction) This jump stub now prints correctly, as so: The hex encodings are now present, and both instructions in the jump stub are printed correctly. |
These changes address #70849 by implementing the special case of printing the LARGEJMP pseudo-instruction in
emitter::emitDispInson ARM64. To achieve functional and organizational parity between the ARM32 and ARM64 implementations,emitter::emitDispInshas been re-architected as so:emitter::emitDispInsHelp(just like on ARM32). The method now checks for the special LARGEJMP case, and calls the appropriate method.