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

[API Implementation]: Expose general purpose Crc32 APIs #61558

Merged
merged 60 commits into from
Oct 3, 2022

Conversation

deeprobin
Copy link
Contributor

@deeprobin deeprobin commented Nov 14, 2021

Proposal implementation of #2036 (closes #2036)

Proposed API

namespace System.Numerics
{
    public static class BitOperations
    {
        public static uint Crc32C(uint crc, byte data);
        public static uint Crc32C(uint crc, ushort data);
        public static uint Crc32C(uint crc, uint data);
        public static uint Crc32C(uint crc, ulong data);
    }
}

Current state of implementation

  • Ref Assembly
  • Documentation
  • Implementation
    • Hardware Intrinsics
      • SSE 4.2
      • SSE 4.2 x64
      • ARM
      • ARM x64
    • Software Fallback
  • Tests

Usage of Hardware Intrinsics

uint32_t __crc32b (uint32_t a, uint8_t b); // SSE 4.2
uint32_t __crc32cb (uint32_t a, uint8_t b); // ARM

unsigned int _mm_crc32_u16 (unsigned int crc, unsigned short v); // SSE 4.2
uint32_t __crc32ch (uint32_t a, uint16_t b); // ARM

unsigned int _mm_crc32_u32 (unsigned int crc, unsigned int v); // SSE 4.2
uint32_t __crc32ch (uint32_t a, uint32_t b); // ARM

unsigned __int64 _mm_crc32_u64 (unsigned __int64 crc, unsigned __int64 v); // SSE 4.2 x64
uint32_t __crc32cd (uint32_t a, uint64_t b); // ARM x64

Software Fallback

            foreach (byte b in bytes)
            {
                int tableIndex = (int)((crc ^ b) & 0xFF);
                crc = s_crcTable[tableIndex] ^ (crc >> 8);
            }

/cc @tannergooding

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 14, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added new-api-needs-documentation and removed community-contribution Indicates that the PR has been added by a community member labels Nov 14, 2021
@dotnet-issue-labeler
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, to 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-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Nov 14, 2021

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

Issue Details

Closes #2036

Proposed API

namespace System.Numerics
{
    public static class BitOperations
    {
        public static uint Crc32(uint crc, byte data);
        public static uint Crc32(uint crc, ushort data);
        public static uint Crc32(uint crc, uint data);
        public static uint Crc32(uint crc, ulong data);
    }
}

Current state of implementation

  • Ref Assembly
  • Implementation
  • Tests

/cc @tannergooding

Author: deeprobin
Assignees: -
Labels:

area-System.Numerics, new-api-needs-documentation

Milestone: -

@deeprobin deeprobin marked this pull request as ready for review November 14, 2021 09:45
@deeprobin

This comment has been minimized.

Copy link
Member

@gfoidl gfoidl left a comment

Choose a reason for hiding this comment

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

Some note -- not for implementation of algorithm though.

@deeprobin deeprobin requested review from tannergooding and removed request for stephentoub September 29, 2022 06:00
Copy link
Contributor

@dakersnar dakersnar left a comment

Choose a reason for hiding this comment

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

Couple clarifying questions, but overall LGTM

@dakersnar dakersnar merged commit 56bbc7c into dotnet:main Oct 3, 2022
@danmoseley
Copy link
Member

Thank you for the contribution @deeprobin !

@dakersnar
Copy link
Contributor

One of these tests seems to be failing on tvOS:
#76830
#76952

@deeprobin
Copy link
Contributor Author

I don't think the test is the issue, I think the issue is the Crc32C implementation in clr. These were implemented previously (I think it was Tanner).

Tanner can you locate where and what the issue is?

/cc @tannergooding

@ghost ghost locked as resolved and limited conversation to collaborators Nov 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose general purpose Crc32 APIs