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

[System.Runtime.Intrinsics] API suggestion: Introduce an intrinsic all bits set field to Vector intrinsic types #30659

Closed
john-h-k opened this issue Aug 22, 2019 · 5 comments · Fixed by #33924
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.Intrinsics help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@john-h-k
Copy link
Contributor

Currently we have Vector{nnn}<T>.Zero type which the JIT recognises and transforms into a xorps or similar. It would be useful to have a .AllBitsSet property, which is the inverse, having all bits set. The JIT could ideally generate this to be
cmpps <dest>, xmm0, xmm0, 0x0 which is recognised as dependency breaking on modern intel architectures for minimal cost. An all-ones value is useful, and can be frequently used for masking and mixing elements alongside the all-zero value.

Rational and Usage

It is commonly used, for example it can be used as Xor(left, AllBitsSet) to implement a Not using SIMD, as well as frequently for masking and mixing elements, e.g with Shuffle/And/Or

Proposed API

public struct Vector64<T> where T : struct
{
    public static Vector64<T> AllBitsSet { [Intrinsic] get; }
}

public struct Vector128<T> where T : struct
{
    public static Vector128<T> AllBitsSet { [Intrinsic] get; }
}

public struct Vector256<T> where T : struct
{
    public static Vector256<T> AllBitsSet { [Intrinsic] get; }
}

Details

It should be implemented as an intrinsic which results in

[v]cmpps xmmN, xmmN, xmmN

which is recognised as dependency breaking on Haswell+ archs. Software fallback is relatively simple, effectively Vector128.Create(-1).As<int, T>()

Open Questions

Is the name ok?

Pull Request

None yet

Updates

@GrabYourPitchforks
Copy link
Member

@tannergooding Weren't you looking into this a while back? Am I misremembering?

@tannergooding
Copy link
Member

Yes, and I agree this would be something useful to expose. However, One is not an appropriate name since that might imply the wrong thing.

Something that conveys AllBitsSet would be better, but I'm not sure I have a good suggestion.

@tannergooding tannergooding self-assigned this Sep 29, 2019
@GrabYourPitchforks
Copy link
Member

API review - AllBitsSet seems like a decent name given the scenario.

Suggestion: also rename the internal member Vector<T>.AllOnes at the same time so that if it does become public in the future we have consistent naming already.

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 5.0 milestone Feb 1, 2020
@tannergooding tannergooding added the help wanted [up-for-grabs] Good issue for external contributors label Feb 10, 2020
@Gnbrkm41
Copy link
Contributor

Gnbrkm41 commented Feb 12, 2020

Note: it's PCMPEQ[D/Q/W/B] that are dependency breaking idioms.

@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Mar 3, 2020
@tannergooding
Copy link
Member

This is being handled in #33924, I've co-assigned it to @Gnbrkm41 for the time being.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime.Intrinsics help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants