Skip to content

Fix STLUR for 0 imm #106760

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

Jump to bottom
Merged
EgorBo merged 2 commits into from
Aug 24, 2024
Merged

Fix STLUR for 0 imm #106760

EgorBo merged 2 commits into from
Aug 24, 2024

Conversation

@EgorBo
Copy link
Member

@EgorBo EgorBo commented Aug 21, 2024 •
edited
Loading

Fixes #105906

@ghost ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Aug 21, 2024
This was referenced Aug 21, 2024
Closed
Closed
@EgorBo
Copy link
Member Author

EgorBo commented Aug 22, 2024 •
edited
Loading

PTAL @dotnet/jit-contrib this unblocks Debug build for NativeAOT on macOS (or any other RCPC2 arm64 hardware).

Some code creates LEA(addr, 0) not sure if we should create such unscaled LEAs with 0 offset and zero index in the first place, but looks like it's valid (in this case the 0 was a field sequence ICON) - this led to stlur with 0 offset. stlur (and it load counterpart - ldapur) always use IF_LS_2C encoding, so we shouldn't use the same path as for other loads/stores.

Generally, we emit ldapur/stlur only for volatile loads/stores with LEA and a small offset (no index, no scale) since these instructions don't support any other addressing mode.

PS: I wasn't able to come up with a test which creates LEA(base + 0) tree, so leaving Debug repo build as a test.

@EgorBo EgorBo changed the title Don't use STLUR with 0 offset Fix STLUR for 0 imm Aug 23, 2024
@EgorBo EgorBo requested review from TIHan and amanasifkhalid August 23, 2024 15:47
@EgorBo
Copy link
Member Author

EgorBo commented Aug 23, 2024

PTAL @amanasifkhalid @TIHan as emitter experts

amanasifkhalid
amanasifkhalid approved these changes Aug 23, 2024
View reviewed changes
Copy link
Contributor

@amanasifkhalid amanasifkhalid left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't know about "emitter expert," but this LGTM.

PS: I wasn't able to come up with a test which creates LEA(base + 0) tree, so leaving Debug repo build as a test.

It might be a good idea to add tests for this pattern to codegenarm64test.cpp. You can then run those tests with DOTNET_JitEmitUnitTests="*" and DOTNET_JitEmitUnitTestsSections="general", and just replay a single method with SPMI.

Thanks!

src/coreclr/jit/emitarm64.cpp Outdated

case INS_ldurh:
case INS_ldapurh:
case INS_sturh:
Copy link
Contributor

@amanasifkhalid amanasifkhalid Aug 23, 2024

Choose a reason for hiding this comment

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

While you're here, could you please move these cases up to INS_ldurb/INS_sturb since they have the same behavior?

Copy link
Member Author

@EgorBo EgorBo Aug 23, 2024

Choose a reason for hiding this comment

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

Addressed. They indeed have the same issue but we never use them 🙂

@build-analysis build-analysis bot mentioned this pull request Aug 23, 2024
Closed
@EgorBo EgorBo merged commit 205adae into dotnet:main Aug 24, 2024
@EgorBo EgorBo deleted the fix-stlur branch August 24, 2024 11:35
@github-actions github-actions bot locked and limited conversation to collaborators Sep 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@TIHan TIHan Awaiting requested review from TIHan

1 more reviewer

@amanasifkhalid amanasifkhalid amanasifkhalid approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@EgorBo EgorBo

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

osx-arm64 NativeAOT Debug build failing with encoding_found assert

2 participants

@EgorBo @amanasifkhalid