Skip to content

Add SVE2 API skeleton and implement BitwiseClearXor #115428

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

Merged
merged 2 commits into from
May 13, 2025

Conversation

snickolls-arm
Copy link
Contributor

@snickolls-arm snickolls-arm commented May 9, 2025

This is the first of the SVE2 intrinsics, laying out the test framework and adding the SVE2 API class.

@a74nh @kunalspathak

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 9, 2025
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics
See info in area-owners.md if you want to be subscribed.

[Intrinsic]
[CLSCompliant(false)]
[Experimental(Experimentals.ArmSveDiagId, UrlFormat = Experimentals.SharedUrlFormat)]
public abstract class Sve2 : Sve
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just need to confirm that this inheritance matches the agreed design, as there was some discrepancy in our code generation scripts? I assumed it should be like this, as the architecture has this dependency and some of the API design issues had this inheritance.

Copy link
Member

Choose a reason for hiding this comment

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

This makes sense to me.

@@ -2649,6 +2649,12 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node)
break;
}

case NI_Sve2_BitwiseClearXor:
assert(targetReg == op1Reg);
Copy link
Member

Choose a reason for hiding this comment

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

you need to add movprfx if targetReg != op1Reg something like:

                    if (targetReg != op1Reg)
                    {
                        assert(targetReg != op2Reg);

                        GetEmitter()->emitInsSve_R_R(INS_sve_movprfx, EA_SCALABLE, targetReg, op1Reg);
                    }

See how we handle TrigonometricMultiplyAddCoefficient in this file.

Copy link
Member

Choose a reason for hiding this comment

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

And it seems that's why we are seeing test failure too:

Assert failure(PID 60 [0x0000003c], Thread: 60 [0x003c]): Assertion failed 'targetReg == op1Reg' in 'JIT.HardwareIntrinsics.Arm._Sve2.SimpleTernaryOpTest__Sve2_BitwiseClearXor_sbyte:RunLclVarScenario_UnsafeRead():this' during 'Generate code' (IL size 113; hash 0xa6f94412; FullOpts)

    File: /__w/1/s/src/coreclr/jit/hwintrinsiccodegenarm64.cpp:2653
    Image: /root/helix/work/correlation/corerun

Copy link
Member

Choose a reason for hiding this comment

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

The need for movprfx for RMW handling is notably why x64 has helpers like emitIns_SIMD_R_R_R: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/emitxarch.cpp#L9766-L9791

Such helpers allow codegen to mostly ignore the concept and just thing in terms of number of input/outputs instead. It allows for emit to centralize the handling instead. This makes it much simpler to update if we have new considerations in the future, to avoid accidentally missing the handling in various locations, etc.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. The movprfx logic needs to be centralized for sure.

Copy link
Member

Choose a reason for hiding this comment

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

using System;
using System.Collections.Generic;

namespace JIT.HardwareIntrinsics.Arm._Sve2
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering if there is a need to create Sve2 folder and new project for it or just use existing Sve folder and projects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's slightly better for running the tests while developing, as it's faster to run the SVE2 suite on it's own and easier to filter down.

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. Thanks!

@kunalspathak kunalspathak merged commit 3104b88 into dotnet:main May 13, 2025
153 of 158 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.Intrinsics arm-sve Work related to arm64 SVE/SVE2 support community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants