-
Notifications
You must be signed in to change notification settings - Fork 5k
JIT: ARM64 SVE format encodings, SVE_HX_3A_B
to SVE_JL_3A
and SVE_IC_3A
to SVE_IC_3A_C
#98332
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
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsContributes to #94549 Adds formats:
|
SVE_HX_3A_B
to SVE_JL_3A' and
SVE_IC_3A to
SVE_IC_3A_C`SVE_HX_3A_B
to SVE_JL_3A
and SVE_IC_3A
to SVE_IC_3A_C
@dotnet/jit-contrib @dotnet/arm64-contrib @kunalspathak @a74nh this is ready. |
Diff results for #98332Throughput diffsThroughput diffs for linux/arm64 ran on windows/x64Overall (+0.04% to +0.08%)
MinOpts (+0.06% to +0.18%)
FullOpts (+0.04% to +0.08%)
Throughput diffs for osx/arm64 ran on windows/x64Overall (+0.04% to +0.08%)
MinOpts (+0.06% to +0.15%)
FullOpts (+0.03% to +0.08%)
Throughput diffs for windows/arm64 ran on windows/x64Overall (+0.04% to +0.08%)
MinOpts (+0.07% to +0.18%)
FullOpts (+0.04% to +0.08%)
Details here Throughput diffs for linux/arm64 ran on linux/x64MinOpts (-0.00% to +0.01%)
Details here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise everything else looks fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
src/coreclr/jit/emitarm64.cpp
Outdated
* | ||
* Prints the encoding for format [<Zn>.D{, #<imm>}] | ||
*/ | ||
void emitter::emitDispSveImmIndex(regNumber reg1, insOpts opt, ssize_t imm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this logic be merged into emitDispSveImm
in #98468 (or the other way around)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't matter either way. I'll have to resolve any conflicts when my PRs get merged, and that will include some cleanup.
Also do you know where the TP impact is coming from on Windows x64? Your changes look like they only affect SVE paths. |
It's probably just the additional cases in emitIns. Should split it out to see if the TP regressions clear up. |
Diff results for #98332Throughput diffsThroughput diffs for linux/arm64 ran on windows/x64MinOpts (-0.00% to +0.01%)
Throughput diffs for osx/arm64 ran on windows/x64MinOpts (-0.01% to +0.01%)
Details here |
Merging, failures are not related. |
Contributes to #94549
Adds formats:
Left: Capstone,

Right: Jit