Skip to content

Always set InstructionSet_ArmBase in PAL_GetJitCpuCapabilityFlags #34107

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 26, 2020
Merged

Always set InstructionSet_ArmBase in PAL_GetJitCpuCapabilityFlags #34107

merged 1 commit into from
Mar 26, 2020

Conversation

@echesakov
Copy link
Contributor

@echesakov echesakov commented Mar 26, 2020

PAL_GetJitCpuCapabilityFlags should always set InstructionSet_ArmBase.
Otherwise based on an algorithm added in #33936

runtime/src/coreclr/src/inc/corinfoinstructionset.h

Lines 180 to 195 in 951bc70

if (resultflags.HasInstructionSet(InstructionSet_ArmBase) && !resultflags.HasInstructionSet(InstructionSet_ArmBase_Arm64))
resultflags.RemoveInstructionSet(InstructionSet_ArmBase);
if (resultflags.HasInstructionSet(InstructionSet_AdvSimd) && !resultflags.HasInstructionSet(InstructionSet_AdvSimd_Arm64))
resultflags.RemoveInstructionSet(InstructionSet_AdvSimd);
if (resultflags.HasInstructionSet(InstructionSet_Crc32) && !resultflags.HasInstructionSet(InstructionSet_Crc32_Arm64))
resultflags.RemoveInstructionSet(InstructionSet_Crc32);
if (resultflags.HasInstructionSet(InstructionSet_AdvSimd) && !resultflags.HasInstructionSet(InstructionSet_ArmBase))
resultflags.RemoveInstructionSet(InstructionSet_AdvSimd);
if (resultflags.HasInstructionSet(InstructionSet_Aes) && !resultflags.HasInstructionSet(InstructionSet_ArmBase))
resultflags.RemoveInstructionSet(InstructionSet_Aes);
if (resultflags.HasInstructionSet(InstructionSet_Crc32) && !resultflags.HasInstructionSet(InstructionSet_ArmBase))
resultflags.RemoveInstructionSet(InstructionSet_Crc32);
if (resultflags.HasInstructionSet(InstructionSet_Sha1) && !resultflags.HasInstructionSet(InstructionSet_ArmBase))
resultflags.RemoveInstructionSet(InstructionSet_Sha1);
if (resultflags.HasInstructionSet(InstructionSet_Sha256) && !resultflags.HasInstructionSet(InstructionSet_ArmBase))
resultflags.RemoveInstructionSet(InstructionSet_Sha256);
we will end up with AdvSimd, Crc32 removed from the set of supported ISAs, but not their 64-bit counterparts AdvSimd_Arm64 and Crc32_Arm64.

This was a reason of failure as seen in #33749 (comment) where code guarded with AdvSimd.IsSupported was just removed by JIT as dead but code guarded with AdvSimd.Arm64.IsSupported as in System.Collections.BitArray..ctor(Boolean[] values) was throwing System.PlatformNotSupportedException from JIT.

cc @davidwrighton @CarolEidt @tannergooding

@echesakov
Unconditionally enable InstructionSet_ArmBase in PAL_GetJitCpuCapabil…
75f37f9 
…ityFlags in src/coreclr/src/pal/src/misc/jitsupport.cpp
@echesakov echesakov added arch-arm64 os-linux Linux OS (any supported distro) area-PAL-coreclr labels Mar 26, 2020
@echesakov echesakov mentioned this pull request Mar 26, 2020
Merged
@jaredpar jaredpar mentioned this pull request Mar 26, 2020
Closed
CarolEidt
CarolEidt approved these changes Mar 26, 2020
View reviewed changes
Copy link
Contributor

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM - thanks for finding and fixing this!

@davidwrighton
Copy link
Member

davidwrighton commented Mar 26, 2020

Sigh, this is a good fix but missing ArmBase should also be removing the 64bit variants of everything too. I'll address that in another pr.

@davidwrighton davidwrighton mentioned this pull request Mar 26, 2020
Merged
@echesakov
Copy link
Contributor Author

echesakov commented Mar 26, 2020

Failure on win-x86 rel is unrelated and tracked by #34128

@echesakov echesakov merged commit 24e10f6 into dotnet:master Mar 26, 2020
@echesakov echesakov deleted the GetJitCpuCapabilityFlags-Add-InstructionSet-ArmBase branch March 26, 2020 19:17
@echesakov echesakov mentioned this pull request Apr 1, 2020
Merged
@kunalspathak kunalspathak mentioned this pull request Apr 3, 2020
Merged
@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.

Reviewers

@tannergooding tannergooding tannergooding approved these changes

+1 more reviewer

@CarolEidt CarolEidt CarolEidt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

arch-arm64 area-PAL-coreclr os-linux Linux OS (any supported distro)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@echesakov @davidwrighton @CarolEidt @tannergooding