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

JIT ARM64-SVE: Move SVE-specific emitIns logic to emitarm64sve.cpp #99983

Merged
merged 14 commits into from Mar 20, 2024

Conversation

amanasifkhalid
Copy link
Member

We already had a few SVE-specific emitIns_* methods; this PR strips out the remaining SVE instructions from our general emitIns methods, and moves them to corresponding emitInsSve methods in emitarm64sve.cpp. I plan to move over our SVE-specific helper methods, and extract the SVE cases in other shared emitter methods to SVE methods, but this PR is big enough that this seemed like a natural stopping point. There are no diffs in the SVE unit tests from this change.

cc @dotnet/arm64-contrib

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 19, 2024
@amanasifkhalid amanasifkhalid changed the title JIT ARM64-SVE: Move SVE-specific emitIns_* logic to emitarm64sve.cpp JIT ARM64-SVE: Move SVE-specific emitIns logic to emitarm64sve.cpp Mar 19, 2024
Copy link
Contributor

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

@amanasifkhalid amanasifkhalid added the arm-sve Work related to arm64 SVE/SVE2 support label Mar 19, 2024
Copy link
Contributor

@a74nh a74nh left a comment

Choose a reason for hiding this comment

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

It's fairly tricky for me to confirm the code in emitarm64sve.cpp is just a copy/paste of the code from emitarm64.cpp. However, the changes good.

I'm assuming:

  • No difference in codegen test output
  • The contents of the functions in emitarm64sve.cpp are identical to that cut from emitarm64.cpp, plus potentially some repeated parts/boilerplate.

Additional thoughts:

  • We should merge this asap
  • In another PR, we could move the other SVE parts from the sanity check, disp instr, perf score etc functions into here too.
  • We have the choice to eventually use emitInsSve_ functions directly instead of calling from emitIns_. No need to make that choice yet - depends on the IR code.

@amanasifkhalid
Copy link
Member Author

@a74nh thanks for the review! Your assumptions are correct. This is a no-diff change, and I did a diff of the SVE unit test output with and without this change, and didn't see any diffs there either. I plan to do similar transition work for other methods (emitOutputInstr is my next priority, since that should yield TP improvements too).

We have the choice to eventually use emitInsSve_ functions directly instead of calling from emitIns_. No need to make that choice yet - depends on the IR code.

Agreed. It would be slightly more performant if we could call the emitInsSve methods directly instead of falling into the default case in emitIns, but I didn't want to change the API surface just yet.

cc @kunalspathak @TIHan

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

We have the choice to eventually use emitInsSve_ functions directly instead of calling from emitIns_

Agree.

I am hoping to see 0 or very few references of "sve_" text in emitarm64.cpp. Currently, there are still methods like insEncodeSveElemsize_tszh_tszl_and_imm , insGetSveReg1ListSize, etc. in emitarm64.cpp. Is there a reason why they can't be moved to emitarm64sve.cpp?

@amanasifkhalid
Copy link
Member Author

I am hoping to see 0 or very few references of "sve_" text in emitarm64.cpp. Currently, there are still methods like insEncodeSveElemsize_tszh_tszl_and_imm , insGetSveReg1ListSize, etc. in emitarm64.cpp. Is there a reason why they can't be moved to emitarm64sve.cpp?

I plan to move those too. I just thought that would be better suited for a follow-up PR, since this one is already pretty big.

Copy link
Member

@kunalspathak kunalspathak left a comment

Choose a reason for hiding this comment

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

LGTM

@amanasifkhalid amanasifkhalid merged commit ffe7e26 into dotnet:main Mar 20, 2024
102 of 110 checks passed
@amanasifkhalid amanasifkhalid deleted the emitarm64sve.cpp branch March 20, 2024 14:09
@kunalspathak
Copy link
Member

image

@amanasifkhalid
Copy link
Member Author

For reference, the MinOpts regression is only in the smoke_tests.nativeaot collection (diffs). I imagine it's some outlier method...

@github-actions github-actions bot locked and limited conversation to collaborators Apr 23, 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 arm-sve Work related to arm64 SVE/SVE2 support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants