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

[mono] Implement Arm intrinsics: ArmBase, Crc32 #34240

Merged
merged 10 commits into from
Apr 7, 2020

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Mar 28, 2020

Implement System.Runtime.Intrinsics.Arm.ArmBase and *.Crc32 intrinsics for Arm64 with LLVM-backend. ArmBase is enabled by default, Crc32 for LLVM-AOT can be enabled either via mcpu=native or mattr=+crc (e.g. when we crosscompile for iOS).

Asm: (--aot=llvm,mcpu=native System.Private.CoreLib.dll on Cortex-A57):

;; ArmBase

; int LeadingZeroCount(int value)
0000000000292c84 <System_Runtime_Intrinsics_Arm_ArmBase_LeadingZeroCount_int>:
  292c84:	clz	w0, w0
  292c88:	ret

; int LeadingZeroCount(uint value)
0000000000292c8c <System_Runtime_Intrinsics_Arm_ArmBase_LeadingZeroCount_uint>:
  292c8c:	clz	w0, w0
  292c90:	ret

; int ReverseElementBits(int value)
0000000000292c94 <System_Runtime_Intrinsics_Arm_ArmBase_ReverseElementBits_int>:
  292c94:	rbit	w0, w0
  292c98:	ret

; uint ReverseElementBits(uint value)
0000000000292c9c <System_Runtime_Intrinsics_Arm_ArmBase_ReverseElementBits_uint>:
  292c9c:	rbit	w0, w0
  292ca0:	ret

;; ArmBase.Arm64

; int LeadingSignCount(int value)
0000000000292cac <System_Runtime_Intrinsics_Arm_ArmBase_Arm64_LeadingSignCount_int>:
  292cac:	cls	w0, w0
  292cb0:	ret

; int LeadingSignCount(long value)
0000000000292cb4 <System_Runtime_Intrinsics_Arm_ArmBase_Arm64_LeadingSignCount_long>:
  292cb4:	cls	x0, x0
  292cb8:	ret

; int LeadingZeroCount(long value)
0000000000292cbc <System_Runtime_Intrinsics_Arm_ArmBase_Arm64_LeadingZeroCount_long>:
  292cbc:	clz	x0, x0
  292cc0:	ret

; int LeadingZeroCount(ulong value)
0000000000292cc4 <System_Runtime_Intrinsics_Arm_ArmBase_Arm64_LeadingZeroCount_ulong>:
  292cc4:	clz	x0, x0
  292cc8:	ret

; long ReverseElementBits(long value)
0000000000292ccc <System_Runtime_Intrinsics_Arm_ArmBase_Arm64_ReverseElementBits_long>:
  292ccc:	rbit	x0, x0
  292cd0:	ret

; ulong ReverseElementBits(ulong value)
0000000000292cd4 <System_Runtime_Intrinsics_Arm_ArmBase_Arm64_ReverseElementBits_ulong>:
  292cd4:	rbit	x0, x0
  292cd8:	ret

;; Crc32

; uint ComputeCrc32(uint crc, byte data)
0000000000292ce4 <System_Runtime_Intrinsics_Arm_Crc32_ComputeCrc32_uint_byte>:
  292ce4:	crc32b	w0, w0, w1
  292ce8:	ret

; uint ComputeCrc32(uint crc, ushort data)
0000000000292cec <System_Runtime_Intrinsics_Arm_Crc32_ComputeCrc32_uint_uint16>:
  292cec:	crc32h	w0, w0, w1
  292cf0:	ret

; uint ComputeCrc32(uint crc, uint data)
0000000000292cf4 <System_Runtime_Intrinsics_Arm_Crc32_ComputeCrc32_uint_uint>:
  292cf4:	crc32w	w0, w0, w1
  292cf8:	ret

; uint ComputeCrc32C(uint crc, byte data)
0000000000292cfc <System_Runtime_Intrinsics_Arm_Crc32_ComputeCrc32C_uint_byte>:
  292cfc:	crc32cb	w0, w0, w1
  292d00:	ret

; uint ComputeCrc32C(uint crc, ushort data)
0000000000292d04 <System_Runtime_Intrinsics_Arm_Crc32_ComputeCrc32C_uint_uint16>:
  292d04:	crc32ch	w0, w0, w1
  292d08:	ret

; uint ComputeCrc32C(uint crc, uint data)
0000000000292d0c <System_Runtime_Intrinsics_Arm_Crc32_ComputeCrc32C_uint_uint>:
  292d0c:	crc32cw	w0, w0, w1
  292d10:	ret

;; Crc32.Arm64

; uint ComputeCrc32(uint crc, ulong data)
0000000000292d1c <System_Runtime_Intrinsics_Arm_Crc32_Arm64_ComputeCrc32_uint_ulong>:
  292d1c:	crc32x	w0, w0, x1
  292d20:	ret

; uint ComputeCrc32C(uint crc, ulong data)
0000000000292d24 <System_Runtime_Intrinsics_Arm_Crc32_Arm64_ComputeCrc32C_uint_ulong>:
  292d24:	crc32cx	w0, w0, x1
  292d28:	ret

@tannergooding @echesakovMSFT Could you please take a quick look at asm ^ if it's ok or not.

Vector64<T>, Vector128<T>, Vector<T> are not enabled yet.

@tannergooding
Copy link
Member

ArmBase is enabled by default, Crc32 for LLVM-AOT can be enabled either via mcpu=native or mattr=+crc (e.g. when we crosscompile for iOS).

Is mattr=+crc something from LLVM or something from Mono? If the latter, is it worth making this consistent with the names we were going to use for crossgen? (CC. @davidwrighton)

@EgorBo
Copy link
Member Author

EgorBo commented Mar 28, 2020

ArmBase is enabled by default, Crc32 for LLVM-AOT can be enabled either via mcpu=native or mattr=+crc (e.g. when we crosscompile for iOS).

Is mattr=+crc something from LLVM or something from Mono? If the latter, is it worth making this consistent with the names we were going to use for crossgen? (CC. @davidwrighton)

It's mono, but it's based on LLVM naming convention for features (see llvm::sys::getHostCPUFeatures)
However, we also pass it as is to LLVM's opt and llc tools. (e.g. opt -mattr=+crc mono.ll && llc -mattr=+crc mono.ll)

@EgorBo
Copy link
Member Author

EgorBo commented Mar 30, 2020

@vargaz could you please re-review?

@EgorBo
Copy link
Member Author

EgorBo commented Apr 1, 2020

@tannergooding I've just tried the latest daily .NET Core SDK (5.0.100-preview.3.20181.13) with System.Runtime.Intrinsics.Experimental (5.0.0-preview.3.20181.9) on my Cortex A57:

ArmBase.IsSupported: True
ArmBase.Arm64.IsSupported: True
AdvSimd.IsSupported: True
AdvSimd.Arm64.IsSupported: True
Aes.IsSupported: True
Aes.Arm64.IsSupported: True
Crc32.IsSupported: True
Crc32.Arm64.IsSupported: False
Sha1.IsSupported: True
Sha1.Arm64.IsSupported: True
Sha256.IsSupported: True
Sha256.Arm64.IsSupported: True

Any idea why Crc32.Arm64.IsSupported is only thing to return False here ? (arm64 arch).
CoreCLR runtime. Looks suspicious.

/proc/cpuinfo

Features        : fp asimd evtstrm aes pmull sha1 sha2 crc32 cpuid

@tannergooding
Copy link
Member

@EgorBo, looks like it probably doesn't have some of the fixes that went in recently.

The SDK reports the following, which is 13 days old (bc28cbef85):

Host (useful for support):
  Version: 5.0.0-preview.3.20169.1
  Commit:  bc28cbef85

There were some important fixes (such as #34139) that only went in about 5 days ago.

@EgorBo
Copy link
Member Author

EgorBo commented Apr 1, 2020

@tannergooding thanks, forgot that daily sdks are not entirely daily :)

I noticed an API problem here it took me a while to understand.
So imagine my hw doesn't support Sha256.

Sha256.IsSupported returns false (expected)
Sha256.Arm64.IsSupported returns true (unexpected) - because Sha256 doesn't define a separate Arm64 impl it returns ArmBase.Arm64.IsSupported for my Sha256.Arm64.IsSupported call.

@echesakov
Copy link
Contributor

I noticed an API problem here it took me a while to understand.
So imagine my hw doesn't support Sha256.
Sha256.IsSupported returns false (expected)
Sha256.Arm64.IsSupported returns true (unexpected) - because Sha256 doesn't define a separate Arm64 impl it returns ArmBase.Arm64.IsSupported for my Sha256.Arm64.IsSupported call.

@EgorBo This sounds as the issue I faced recently (see my comment #34107 (comment)). Should be fixed now.

@EgorBo
Copy link
Member Author

EgorBo commented Apr 1, 2020

I noticed an API problem here it took me a while to understand.
So imagine my hw doesn't support Sha256.
Sha256.IsSupported returns false (expected)
Sha256.Arm64.IsSupported returns true (unexpected) - because Sha256 doesn't define a separate Arm64 impl it returns ArmBase.Arm64.IsSupported for my Sha256.Arm64.IsSupported call.

@EgorBo This sounds as the issue I faced recently (see my comment #34107 (comment)). Should be fixed now.

um.. I don't see how it can be implemented in Jit, here is what I mean: https://sharplab.io/#v2:EYLgHgbALANALiAlgGwD4AEBMBGAsAKAKP3QGYACLcgQQCcBbAIQEMBnAUwIG8Dy/KK6bBHLAA9mOTkAkqwDKAVwAOSsbTjsAJuQC8APnJxaC9gG5e/C3zKVMNBtCvke+fm4GVhoiVNmKVahra+obGZk4AvgRRhCSCdnIAFsyYAKwiIPZMbJz4Lm42QiLikjLyyqrqWroGAGbMyBym5AD0LeTsAHQA5p3k9ACe5IkA7uSaYuysAHYA5HDkrBWB5Ekp6U5ObavJaSITU3MLyQBu7OSIcKzkYiPTWY74MQSFdkIA7Nxb7XJiFwvICQAa2uyEQQPO0nIAGNEuxoUDyLU1Dt1iJmCdmChmMAUJchnDaLk3NtgAoFoh7tIADL/WbXABE0IaUhKUjo2Q4LQ50BAIG67DgAH0/MsqpoABQASgZTkKXjZ5AAYhJpTVUXtOjyoJ1RQFxc1tuh3tcjCZokA===
So I'd add an empty Arm64 nested class with the only property - IsSupported (to return what Sha256.IsSupported returns)

@echesakov
Copy link
Contributor

So I'd add an empty Arm64 nested class with the only property - IsSupported (to return what Sha256.IsSupported returns)

@EgorBo Ah okay - thanks for explanation! I mixed this up with another issue. I guess we could workaround this by defining Sha256.Arm64 class that contains only one property IsSupported always returning false but this just doesn't sound right to me.

Perhaps deriving Sha256 class from ArmBase wasn't a good idea.

@tannergooding
Copy link
Member

Yes, we would need to have an explicit class that plays into the normal lookup checks.

This is probably something that should have an issue and should go through API review.

@EgorBo EgorBo merged commit 81fe6be into dotnet:master Apr 7, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
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.

6 participants