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.

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

Reenable unified instruction set logic #33936

Merged
merged 3 commits into from
Mar 24, 2020
Merged

Reenable unified instruction set logic #33936

merged 3 commits into from
Mar 24, 2020

Conversation

davidwrighton
Copy link
Member

Revert #33901 to reenable #33730. The issue was that the runtime was incorrectly disabling use in the jit compiler for all hardware intrinsics on 64 bit platforms at runtime. Fixed by calling Set64BitInstructionSetVariants before EnsureValidInstructionSetSupport.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 22, 2020
@davidwrighton
Copy link
Member Author

@dotnet/jit-contrib Note, this changes the jit interface

@davidwrighton
Copy link
Member Author

/azp list

@azure-pipelines
Copy link

CI/CD Pipelines for this repository:

@sandreenko
Copy link
Contributor

sandreenko commented Mar 23, 2020 •
edited
Loading

@davidwrighton azp run is useless now, see #32777

I have triggered outerloop for this PR manually https://dev.azure.com/dnceng/public/_build/results?buildId=569342, but it won't merge master changes (if there were any) with your branch if that is important.

@jaredpar jaredpar mentioned this pull request Mar 23, 2020
Closed
lpereira
lpereira reviewed Mar 23, 2020
View reviewed changes
src/coreclr/src/inc/corinfoinstructionset.h
Comment on lines +32 to +39
InstructionSet_SSE=1,
InstructionSet_SSE2=2,
InstructionSet_SSE3=3,
InstructionSet_SSSE3=4,
InstructionSet_SSE41=5,
InstructionSet_SSE42=6,
InstructionSet_AVX=7,
InstructionSet_AVX2=8,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed at this point (still plenty of room in the bitmask), but for instruction sets that are implied by other instruction sets, a better strategy could be used here to pack these bits. For instance, for x86 SIMD, a "SIMD level" thing could be used (e.g. InstructionSet_SIMD_Level >= InstructionSet_SSE42 to test if SSE 4.2 is supported, with InstructionSet_SIMD_Level being in this case a 3-bit mask).

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, one thing to note is that the representation as a 64bit bitmask is an internal implementation detail, and we can change it fairly easily when we reach to having more than 62 instruction sets on a specific architecture. I believe that this scheme should last for at least 5 more years, which is long enough I don't care to plan further ahead.

@davidwrighton
Revert "Revert "Unify instruction set definition (dotnet#33730)""
7c98b9c 
This reverts commit 5a71f14.
@davidwrighton
Ensure that 64 bit variants of instruction sets are handled correctly
b5b96cc 
- Make sure to enable them based on the related 32bit instruction sets before disabling instruction sets that are enabled but not compatible with the instruction set hierarchy the runtime is designed for.
@davidwrighton
Update jit EE version interface as this revert changes the interface …
6eeb819 
…back to the pre-revert state.
@davidwrighton davidwrighton force-pushed the reenable_unified_instruction_set_logic branch from 651b4a7 to 6eeb819 Compare March 23, 2020 19:00
@dotnet dotnet deleted a comment from azure-pipelines bot Mar 23, 2020
@dotnet dotnet deleted a comment from azure-pipelines bot Mar 23, 2020
@dotnet dotnet deleted a comment from azure-pipelines bot Mar 23, 2020
fadimounir
fadimounir approved these changes Mar 23, 2020
View reviewed changes
Copy link
Contributor

@fadimounir fadimounir left a comment

Choose a reason for hiding this comment

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

LGTM. I did not review the actual contents from the original PR that was reverted. I just looked at the EnsureValidInstructionSetSupport fix, and the JIT interface guid change

@davidwrighton davidwrighton merged commit d0c892d into dotnet:master Mar 24, 2020
This was referenced Mar 26, 2020
Merged
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 2020
@davidwrighton davidwrighton deleted the reenable_unified_instruction_set_logic branch April 20, 2021 17:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lpereira lpereira lpereira left review comments

@fadimounir fadimounir fadimounir approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@davidwrighton @sandreenko @lpereira @fadimounir @Dotnet-GitSync-Bot