Runtime: API Proposal : Arm64 or Generic Intrinsics Crc32

[sdmaclea]

Target framework netcoreapp 2.1 or later

namespace System.Runtime.Intrinsics.Arm.Arm64
{
    public static class Crc32
    {
        public static bool IsSupported { get { throw new NotImplementedException(); } }

        public static uint Crc32(uint crc, byte data) { throw new NotImplementedException(); }
        public static uint Crc32(uint crc, ushort data) { throw new NotImplementedException(); }
        public static uint Crc32(uint crc, uint data) { throw new NotImplementedException(); }
        public static uint Crc32(uint crc, ulong data) { throw new NotImplementedException(); }

        public static uint Crc32C(uint crc, byte data) { throw new NotImplementedException(); }
        public static uint Crc32C(uint crc, ushort data) { throw new NotImplementedException(); }
        public static uint Crc32C(uint crc, uint data) { throw new NotImplementedException(); }
        public static uint Crc32C(uint crc, ulong data) { throw new NotImplementedException(); }
    }
}

Arm32 supports similar instructions but does not support the 64 bit forms.

[sdmaclea]

@CarolEidt @eerhardt @RussKeldorph PTAL
@tannergooding @4creators @jkotas @dotnet/jit-contrib FYI

[sdmaclea]

@fiigii FYI

[tannergooding]

IIRC this was partially discussed in one of the previous API reviews (possibly the one for the x86 intrinsics).

I believe it was determined that intrinsics which are shared or those that would be generally useful should not be exposed any differently (i.e. just expose Arm64.Crc32 and Sse42.Crc32). However, these intrinsics represent opportunities to add a general API for the functionality that was exposed in more logical location (ex: https://github.com/dotnet/corefx/issues/12425). Said API could then be implemented in terms of:

if (Arm64.Crc32.IsSupported)
{
}
else if (x86.Sse42.IsSupported)
{
}
else
{
    // Software Impementation
}

[sdmaclea]

Re dotnet/runtime#18876, I just read through the discussion. This subject was not discussed significantly enough to form a consensus or to establish a precedent.

https://github.com/dotnet/designs/blame/master/accepted/platform-intrinsics.md#L24-L25 Suggests we shouldn't create intrinsics for things which are common to all platforms.

I am certainly OK making this a ARM64 proposal, but we should formally establish a precedent.

[sdmaclea]

@CarolEidt @eerhardt I did not see either of you weigh in on dotnet/runtime#18876. Can you comment on what preferred practice should be.

[tannergooding]

To be clear, I gave dotnet/runtime#18876 as an example of something (specifically, Bit Manipulation Instructions) for which:

1. Intrinsics are going to be exposed and
2. Which is useful as a general API

It was not given as something for which an explicit design decision had been made (AFAIK).

I'm also not sure that the platform-intrinsics.md doc is entirely up-to-date. It is missing many other decisions that have been made in the past 5 months since the initial review (several have been made in the past week in the PR threads).

From what I remember, the specific section you linked isn't relevant. For cases where an instruction represents processor specific support, the JIT will end up either supporting it or not. If it is supporting it, it can either expose an explicit API (which is what the intrinsics are doing) or it can expose a general API and hardcode the JIT to support that (which is what something like System.Numerics.Vectors are doing).

The explicit API ends up being more flexible in the long-run, not only because other APIs (including the general APIs themselves) can be implemented in terms of it, but because users who have more explicit requirements can make use of it independently. It also allows the general purpose API to be iterated on and improved outside of a runtime update (which is a very limiting factor for something like System.Numerics.Vectors today)

[fiigii]

General APIs should be in System.Numerics or somewhere else.

BTW, x86 HW intrinsic does not have the API below:

public static uint Crc32(uint crc, ulong data) { throw new NotImplementedException(); }

[sdmaclea]

It was not given as something for which an explicit design decision had been made (AFAIK).

I will wait for an explicit design decision by API champions.

I'm also not sure that the platform-intrinsics.md doc is entirely up-to-date

No it is not. @caroleidt will be updating

x86 HW intrinsic does not have the APIs below:

In CoreClr origin/master X86.Sse42 does. Although the ulong form has a the Crc32 types as 64 bit int despite crc32 being a 32bit quantity by definition.

[fiigii]

In CoreClr origin/master X86.Sse42 does.

My apologies, I meant public static uint Crc32(uint crc, ulong data)

[sdmaclea]

Ok I just looked and found Arm64 also supports crc32c. I will therefore revise the proposal above independent of precedent.

[4creators]

General APIs should be in System.Numerics or somewhere else.

IMO HW intrinsic philosophy was to be close to original CPU instructions. Larger abstractions of CPU functionality should go to dedicated classes/namespaces, as a result I agree with @fiigii and @tannergooding.

Anyway BitManipulation dotnet/runtime#18876 should be revived and implemented in parallel to HW intrinsics. That is a sufficiently small API to not be a high burden for the team.

[eerhardt]

IMO HW intrinsic philosophy was to be close to original CPU instructions. Larger abstractions of CPU functionality should go to dedicated classes/namespaces

I agree. Here we simply expose the ARM intrinsic just like every other intrinsic.

Higher level APIs should get separate API proposals and implementations. We shouldn't worry about a higher level CRC function here. We should only need to worry about exposes the ARM intrinsic.

[CarolEidt]

A few comments:

• I am in the process of updating platform-intrinsics.md. I was about to do a PR on Monday, but then it seemed that a bunch of issues were up in the air again. I'll attempt to push what I have tomorrow, and then we can discuss.
• I think the right direction is to support a general capability if it 1) is available on multiple targets, and 2) has a reasonably performant implementation for when the intrinsics aren't available.

[CarolEidt]

@sdmaclea @eerhardt @fiigii @tannergooding - shall I tag this as ready-for-review?

[sdmaclea]

Yes

[morganbr]

Would it make sense for CRC to be covered by dotnet/runtime#24328 or is there a reason to treat it differently than other hashing?

[tannergooding]

@morganbr, this is for exposing the Hardware Intrinsic API for directly emitting the underlying crc32 instruction for a given platform.

dotnet/runtime#24328 is about exposing a general purpose API (with software implementation). Such a general purpose API may end up using the hardware intrinsics to provide better performance.

[morganbr]

I guess my question is whether there's a need for direct usage of the intrinsic if we have an API that may be implemented by hardware (though perhaps we need the intrinsic in order to implement that API).

[sdmaclea]

perhaps we need the intrinsic in order to implement that API

@morganbr 👍 That's the plan

There will also be a polynomial multiply intrinsics, as well as cryptograhic intrinsics in future HW Intrinsic API proposals

[terrajobst]

Looks good as proposed. Feedback:

• The type should be renamed A64Crc32
  
  
  
  • Should be copied the 32-bit ones into A32Crc32
  
  

[echesakovMSFT]

There is an issue with the name of ISA class being the same as one of the methods

'Crc32': member names cannot be the same as their enclosing type

[tannergooding]

I've removed the api-approved label and will take this back to API review.

We will need to determine whether the ISA name or method names change.

[echesakovMSFT]

Closed in https://github.com/dotnet/runtime/pull/117