Skip to content

Always set InstructionSet_ArmBase in PAL_GetJitCpuCapabilityFlags - #34107

Merged
echesakov merged 1 commit into
dotnet:masterfrom
echesakov:GetJitCpuCapabilityFlags-Add-InstructionSet-ArmBase
Mar 26, 2020
Merged

Always set InstructionSet_ArmBase in PAL_GetJitCpuCapabilityFlags #34107
echesakov merged 1 commit into
dotnet:masterfrom
echesakov:GetJitCpuCapabilityFlags-Add-InstructionSet-ArmBase

Conversation

@echesakov

@echesakovechesakov commented Mar 26, 2020

Copy link
Copy Markdown
Contributor

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
@echesakovechesakov added arch-arm64 os-linux Linux OS (any supported distro) area-PAL-coreclr only for closed issues labels Mar 26, 2020
@echesakovechesakov mentioned this pull request Mar 26, 2020
Merged
@jaredparjaredpar mentioned this pull request Mar 26, 2020
Closed
CarolEidt
CarolEidt approved these changes Mar 26, 2020
View reviewed changes

@CarolEidtCarolEidt left a comment

Copy link
Copy Markdown
Contributor

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

davidwrighton commented Mar 26, 2020

Copy link
Copy Markdown
Member

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.

@davidwrightondavidwrighton mentioned this pull request Mar 26, 2020
Merged
@echesakov

echesakov commented Mar 26, 2020

Copy link
Copy Markdown
ContributorAuthor

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

@echesakov
echesakov merged commit 24e10f6 into dotnet:masterMar 26, 2020
@echesakov
echesakov deleted the GetJitCpuCapabilityFlags-Add-InstructionSet-ArmBase branch March 26, 2020 19:17
@echesakovechesakov mentioned this pull request Apr 1, 2020
Merged
@kunalspathakkunalspathak mentioned this pull request Apr 3, 2020
Merged
@ghostghost locked as resolved and limited conversation to collaborators Dec 10, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@tannergoodingtannergoodingtannergooding approved these changes
+1 more reviewer
@CarolEidtCarolEidtCarolEidt approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

arch-arm64area-PAL-coreclronly for closed issuesos-linuxLinux 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