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

ARM64-SVE: Emit AddSequentialAcross correctly #106292

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

amanasifkhalid
Copy link
Member

Fixes #106180. AddSequentialAcross now looks like this in JIT disasms:

ptrue   p0.d
fadda   v17.d, p0, v17.d, z16.d

We no longer emit movprfx, and the destination SIMD register is printed correctly (I think this is everything we needed to fix for this intrinsic). I also found a couple of other nearby instruction formats that needed to be fixed in emitDispInsSveHelp.

AddSequentialAcross stress tests are passing. @dotnet/arm64-contrib PTAL, thanks!

@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 Aug 12, 2024
@amanasifkhalid amanasifkhalid self-assigned this Aug 12, 2024
@amanasifkhalid amanasifkhalid added this to the 9.0.0 milestone Aug 12, 2024
emitDispReg(id->idReg1(), size, true); // ddddd
emitDispLowPredicateReg(id->idReg2(), insGetPredicateType(fmt), id->idInsOpt(), true); // ggg
emitDispReg(id->idReg1(), size, true); // ddddd
emitDispSveReg(id->idReg3(), id->idInsOpt(), false); // mmmmm
break;

// <V><dn>, <Pg>, <V><dn>, <Zm>.<T>
case IF_SVE_HJ_3A: // ........xx...... ...gggmmmmmddddd -- SVE floating-point serial reduction (predicated)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised this was never caught in the codegen testing. Although it's possible something else changed around it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I imagine this was refactored at some point after it was first implemented

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.

This LGTM

@amanasifkhalid
Copy link
Member Author

@TIHan can you PTAL? Thanks!

@JulieLeeMSFT JulieLeeMSFT requested a review from TIHan August 13, 2024 18:40
@JulieLeeMSFT
Copy link
Member

@TIHan, @dotnet/arm64-contrib, please review this PR before RC1 snap.

Copy link
Contributor

@TIHan TIHan left a comment

Choose a reason for hiding this comment

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

LGTM, straightforward change.

@amanasifkhalid amanasifkhalid merged commit 7d3cc13 into dotnet:main Aug 13, 2024
111 checks passed
@amanasifkhalid amanasifkhalid deleted the fix-fadda branch August 13, 2024 23:25
@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 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.

ARM-SVE: AddSequentialAcross() is defined incorrectly
4 participants