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: GetActiveElementCount #102813

Merged
merged 9 commits into from
Jun 1, 2024

Conversation

a74nh
Copy link
Contributor

@a74nh a74nh commented May 29, 2024

Also added some Helper functions to get random data for a mask element. These can eventually be used in existing templates.

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label May 29, 2024
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.

@a74nh
Copy link
Contributor Author

a74nh commented May 29, 2024

@dotnet/arm64-contrib @kunalspathak This is ready now

@a74nh a74nh marked this pull request as ready for review May 29, 2024 12:51
@kunalspathak kunalspathak added the arm-sve Work related to arm64 SVE/SVE2 support label May 29, 2024
argType = JITtype2varType(strip(info.compCompHnd->getArgType(sig, arg1, &argClass)));
op1 = impPopStack().val;

// Op1 and Op2 are masks. Op1 mask handling will be handled via IsExplicitMaskedOperation.
Copy link
Member

Choose a reason for hiding this comment

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

What cannot we handle op2 handling next to the place where we handle op1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would require an if (id ==NI_Sve_GetActiveElementCount) in hwintrinsic.cpp. I'm trying to avoid adding any Arm specific code in that file.

Copy link
Member

@kunalspathak kunalspathak May 29, 2024

Choose a reason for hiding this comment

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

currently we have this:

    if (HWIntrinsicInfo::IsExplicitMaskedOperation(intrinsic))
    {
        assert(numArgs > 0);
        GenTree* op1 = retNode->AsHWIntrinsic()->Op(1);
        if (intrinsic == NI_Sve_ConditionalSelect)
        {
            if (op1->IsVectorAllBitsSet() || op1->IsMaskAllBitsSet())
            {
                return retNode->AsHWIntrinsic()->Op(2);
            }
            else if (op1->IsVectorZero())
            {
                return retNode->AsHWIntrinsic()->Op(3);
            }
        }

        if (!varTypeIsMask(op1))
        {
            // Op1 input is a vector. HWInstrinsic requires a mask.
            retNode->AsHWIntrinsic()->Op(1) = gtNewSimdConvertVectorToMaskNode(retType, op1, simdBaseJitType, simdSize);
        }
}

we can have varTypeIsMask(op2) after the op1, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but would still require a NI_Sve_GetActiveElementCount check, as we don't want to convert for other nodes.

Copy link
Member

@kunalspathak kunalspathak May 29, 2024

Choose a reason for hiding this comment

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

hhm, are there many intrinsic that will have op2 as mask? we can add assert or something to make sure that it is NI_Sve_GetActiveElementCount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've simplified this handling. There's now just a small extra bit in IsExplicitMaskedOperation()

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

@kunalspathak
Copy link
Member

/ba-g build errors are timeout

@kunalspathak kunalspathak merged commit 5c741d7 into dotnet:main Jun 1, 2024
147 of 167 checks passed
@a74nh a74nh deleted the GetActiveElementCount_github branch June 3, 2024 08:57
@github-actions github-actions bot locked and limited conversation to collaborators Jul 4, 2024
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 new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants