Skip to content
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.

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

Fix STLUR for 0 imm #106760

Merged
merged 2 commits into from
Aug 24, 2024
Merged

Fix STLUR for 0 imm #106760

merged 2 commits into from
Aug 24, 2024

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Aug 21, 2024

Fixes #105906

@EgorBo
Copy link
Member Author

EgorBo commented Aug 22, 2024

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
Copy link
Member Author

EgorBo commented Aug 23, 2024

PTAL @amanasifkhalid @TIHan as emitter experts

Copy link
Member

@amanasifkhalid amanasifkhalid left a comment

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!

@@ -5633,9 +5631,7 @@ void emitter::emitIns_R_R_I(instruction ins,
break;

case INS_ldurh:
case INS_ldapurh:
case INS_sturh:
Copy link
Member

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

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 🙂

@EgorBo EgorBo merged commit 205adae into dotnet:main Aug 24, 2024
104 of 108 checks passed
@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.
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.

osx-arm64 NativeAOT Debug build failing with encoding_found assert
2 participants