Skip to content

Fix the unwind opcode for new APX registers - #112799

Merged
kunalspathak merged 2 commits into
dotnet:mainfrom
kunalspathak:unwind-apx
Feb 22, 2025
Merged

Fix the unwind opcode for new APX registers#112799
kunalspathak merged 2 commits into
dotnet:mainfrom
kunalspathak:unwind-apx

Conversation

@kunalspathak

Copy link
Copy Markdown
Contributor

APX added 8 new GPR registers and that shifted the base of floating point registers. However the shift was not getting accounted for properly while embedding the unwind code.

Also, while I was there, I realized that we didn't add the mapping of new registers for DWARF CFI unwind.

Fixes:

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Feb 21, 2025
@kunalspathak kunalspathak changed the title Fix the unwind opcode for new registers Fix the unwind opcode for new APX registers Feb 21, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Comment thread src/coreclr/jit/unwindamd64.cpp
@amanasifkhalid

Copy link
Copy Markdown
Contributor

This might also fix #112286

@saucecontrol

Copy link
Copy Markdown
Member

And #112755 😄

@jakobbotsch

jakobbotsch commented Feb 21, 2025

Copy link
Copy Markdown
Member

Any idea why this didn't show up as 0-size diffs in the original PR? We should be diffing unwind codes in SPMI.

Edit: It looks like we don't:

cr2->applyRelocs(&rc, hotCodeBlock_2, hotCodeSize_2, orig_hotCodeBlock_2);
cr2->applyRelocs(&rc, coldCodeBlock_2, coldCodeSize_2, orig_coldCodeBlock_2);
cr2->applyRelocs(&rc, roDataBlock_2, roDataSize_2, orig_roDataBlock_2);
if (!compareCodeSection(mc, cr1, cr2, hotCodeBlock_1, hotCodeSize_1, roDataBlock_1, roDataSize_1,
orig_hotCodeBlock_1, orig_roDataBlock_1, orig_coldCodeBlock_1, coldCodeSize_1,
hotCodeBlock_2, hotCodeSize_2, roDataBlock_2, roDataSize_2, orig_hotCodeBlock_2,
orig_roDataBlock_2, orig_coldCodeBlock_2, coldCodeSize_2))
return false;
if (!compareCodeSection(mc, cr1, cr2, coldCodeBlock_1, coldCodeSize_1, roDataBlock_1, roDataSize_1,
orig_coldCodeBlock_1, orig_roDataBlock_1, orig_hotCodeBlock_1, hotCodeSize_1,
coldCodeBlock_2, coldCodeSize_2, roDataBlock_2, roDataSize_2, orig_coldCodeBlock_2,
orig_roDataBlock_2, orig_hotCodeBlock_2, hotCodeSize_2))
return false;
if (!compareReadOnlyDataBlock(mc, cr1, cr2, roDataBlock_1, roDataSize_1, orig_roDataBlock_1, roDataBlock_2,
roDataSize_2, orig_roDataBlock_2))
return false;
if (!compareEHInfo(mc, cr1, cr2))
return false;
if (!compareGCInfo(mc, cr1, cr2))
return false;
if (!compareVars(mc, cr1, cr2))
return false;
if (!compareBoundaries(mc, cr1, cr2))
return false;
return true;
. I opened #112807 for it.

@kunalspathak

Copy link
Copy Markdown
Contributor Author

/azp run runtime-coreclr libraries-jitstress2-jitstressregs, runtime-coreclr r2r-extra

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@kunalspathak

kunalspathak commented Feb 22, 2025

Copy link
Copy Markdown
Contributor Author

It seems there are few failures that needs investigation but are unrelated to the change from this PR. I will investigate to make sure and open GH issues, if needed. But this should be ready for review.

r2rextra failures: https://dev.azure.com/dnceng-public/public/_build/results?buildId=959524&view=ms.vss-test-web.build-test-results-tab&runId=25542994&resultId=123196&paneView=debug

@kunalspathak
kunalspathak marked this pull request as ready for review February 22, 2025 03:23
@kunalspathak

Copy link
Copy Markdown
Contributor Author

@dotnet/jit-contrib

code->UnwindOp = (genIsValidFloatReg(reg)) ? UWOP_SAVE_XMM128_FAR : UWOP_SAVE_NONVOL_FAR;
}
code->OpInfo = (BYTE)reg;
code->OpInfo = (BYTE)(genIsValidFloatReg(reg) ? reg - XMMBASE : reg);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we add an assert that genIsValidIntReg(reg) for the else case, to help catch any future bugs or expansions in this space?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could even assert on the actual range of the computed value, since the unwind codes are well-defined and if they change, we want the assert to hit.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

let me do it as a follow up PR and merge this as is to unblock weekend runs.

@kunalspathak

Copy link
Copy Markdown
Contributor Author

/ba-g osx/arm64 seems to be timeout errors.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants