Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

ComputeCrc32, ComputeCrc32C #27421

Merged
merged 1 commit into from
Oct 30, 2019
Merged

ComputeCrc32, ComputeCrc32C #27421

merged 1 commit into from
Oct 30, 2019

Conversation

echesakov
Copy link

@echesakov echesakov changed the title Crc32, Crc32C [WIP] Crc32, Crc32C Oct 24, 2019
@echesakov
Copy link
Author

There is an issue with the name of ISA class being the same as one of the methods.
This should be address on the next API review meeting

@TamarChristinaArm
Copy link

Looks good to me, the name clash is an interesting one. One way could be to name the class Crc instead. It would differ from what the kernel and the architecture feature bit is called but would match with the extension flag clang and gcc name it (+crc.)

@TamarChristinaArm
Copy link

Just so I know I'm understanding this correctly, after splitting the Crc32 from ArmBase, don't you need to change lookupInstructionSet to have the new Crc32 class return InstructionSet_Crc32? That seems what compSetProcessor seems to be setting when crc is available.

And also change Arm64VersionOfIsa to return a new InstructionSet_Crc32_Base64 for the A64 only parts?

@tannergooding
Copy link
Member

don't you need to change lookupInstructionSet

Due to the repo split between CoreFX and CoreCLR, the API exposure and the JIT handling needs to be in separate PRs (as we typically don't want to add code to the JIT that can't be tested at the same time).

@tannergooding
Copy link
Member

One way could be to name the class Crc instead.

We'll be able to discuss this more fully in API review; but I'm not convinced renaming the class differently from the ISA is the correct thing to do. We don't want to risk having clashing names in the future.

I would likely prefer keeping the class name "correct" and then modifying the method name instead. We could likely call it ComputeCrc32 or maybe something like Crc32Step (given that this just performs a single Crc32 iteration).

@echesakov
Copy link
Author

Just so I know I'm understanding this correctly, after splitting the Crc32 from ArmBase, don't you need to change lookupInstructionSet to have the new Crc32 class return InstructionSet_Crc32? That seems what compSetProcessor seems to be setting when crc is available.
And also change Arm64VersionOfIsa to return a new InstructionSet_Crc32_Base64 for the A64 only parts?

@TamarChristinaArm As Tanner pointed this will be part of my another PR in coreclr after the change propagates to corefx and back.

@echesakov echesakov changed the title [WIP] Crc32, Crc32C ComputeCrc32, ComputeCrc32C Oct 30, 2019
@echesakov echesakov merged commit 74ec37a into dotnet:master Oct 30, 2019
@echesakov echesakov deleted the GitHub_26220 branch October 30, 2019 05:37
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corefx that referenced this pull request Oct 30, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
stephentoub pushed a commit to dotnet/corefx that referenced this pull request Oct 30, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/corert that referenced this pull request Oct 30, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Dotnet-GitSync-Bot pushed a commit to Dotnet-GitSync-Bot/mono that referenced this pull request Oct 30, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
marek-safar pushed a commit to mono/mono that referenced this pull request Oct 30, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/corert that referenced this pull request Oct 31, 2019
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
dotnet-maestro bot added a commit to dotnet/corefx that referenced this pull request Nov 2, 2019
* Update dependencies from https://github.com/dotnet/coreclr build 20191101.3

- Microsoft.NET.Sdk.IL - 5.0.0-alpha1.19551.3
- Microsoft.NETCore.ILAsm - 5.0.0-alpha1.19551.3
- Microsoft.NETCore.Runtime.CoreCLR - 5.0.0-alpha1.19551.3

* Update System.Runtime.Intrinsics.Experimental: dotnet/coreclr#27533 dotnet/coreclr#27421

* Update System.Runtime.Intrinsics.Experimental: dotnet/coreclr#27430
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Update dependencies from https://github.com/dotnet/coreclr build 20191101.3

- Microsoft.NET.Sdk.IL - 5.0.0-alpha1.19551.3
- Microsoft.NETCore.ILAsm - 5.0.0-alpha1.19551.3
- Microsoft.NETCore.Runtime.CoreCLR - 5.0.0-alpha1.19551.3

* Update System.Runtime.Intrinsics.Experimental: dotnet/coreclr#27533 dotnet/coreclr#27421

* Update System.Runtime.Intrinsics.Experimental: dotnet/coreclr#27430


Commit migrated from dotnet/corefx@7aac09e
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.

5 participants