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

Expose 64-bit only hardware intrinsic in nested classes #20146

Merged
merged 1 commit into from
Nov 8, 2018

Conversation

fiigii
Copy link

@fiigii fiigii commented Sep 26, 2018

This PR contribute to https://github.com/dotnet/coreclr/issues/20062

  1. Each ISA class may have a nested class named X64 to contain 64-bit only hardware intrinsic. .
  2. This PR only moves "intrinsic" into the nested classes, and helper intrinsic will support 32-bit platforms by fallback or JIT in the next steps.

@fiigii
Copy link
Author

fiigii commented Sep 26, 2018

I only added new APIs in this PR and not remove the old ones to avoid disabling so many tests. After the APIs get reflected back to CoreCLR and implemented in JIT, I will migrate tests to new APIs and remove the old APIs in one shot. That would make the workflow simpler.

@fiigii
Copy link
Author

fiigii commented Sep 26, 2018

@tannergooding
Copy link
Member

I think just X64 would be a better name, and would still convey the appropriate meaning.

@fiigii
Copy link
Author

fiigii commented Sep 26, 2018

Thanks for the suggestions. I am worrying that X64 may be confusing sometimes. We are using X86 to represent all the Intel/AMD platforms, X64 may let users think X86 is for 32-bit only.

@fiigii
Copy link
Author

fiigii commented Sep 26, 2018

IMO, we should emphasize that X86 covers 32-bit and 64-bit both.

@4creators
Copy link

We are using X86 to represent all the Intel/AMD platforms, X64 may let users think X86 is for 32-bit only.

You are right, this could be a problem. How about Arch64Bit?

@4creators
Copy link

Or actually ArchX64

@tannergooding
Copy link
Member

X64 may let users think X86 is for 32-bit only

I think, given the targeted users for these APIs, any documentation we make available, and the limited scope of the x64 classes, the majority of users will be able to easily figure out that the types in Sse are generally available and the types in Sse.X64 are for 64-bit computers only.

@4creators
Copy link

will be able to easily figure out that the types in Sse are generally available and the types in Sse.X64 are for 64-bit computers only.

IMO we do not have to stick to acronyms. Adding short extension to X64 like Only suffix or Arch prefix creates self documenting, easy to understand code.

@eerhardt
Copy link
Member

eerhardt commented Oct 1, 2018

    public static ulong AndNot(ulong left, ulong right) => AndNot(left, right);

Do you want to remove this method? I assume the ulong overload should only exist in the nested X64 class.


Refers to: src/System.Private.CoreLib/shared/System/Runtime/Intrinsics/X86/Bmi1.cs:85 in 86d7c37. [](commit_id = 86d7c37, deletion_comment = False)

@fiigii
Copy link
Author

fiigii commented Oct 1, 2018

Do you want to remove this method? I assume the ulong overload should only exist in the nested X64 class.

@eerhardt I only added new APIs in this PR and not remove the old ones to avoid disabling so many tests. After the APIs get reflected back to CoreCLR and implemented in JIT, I will migrate tests to new APIs and remove the old APIs in one shot. That would make the workflow simpler.

@fiigii
Copy link
Author

fiigii commented Oct 3, 2018

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test

@fiigii
Copy link
Author

fiigii commented Oct 8, 2018

@CarolEidt Do you have comments or suggestions for this PR? If it looks good to you, I think it ready to merge.

@fiigii
Copy link
Author

fiigii commented Oct 8, 2018

Changed the X64 class inheritance, now the hierarchy becomes:

Bmi1.X64 -> Object
Bmi2.X64 -> Object
Lzcnt.X64 -> Object
Popcnt.X64 -> Sse42.X64 -> Sse41.X64 -> Sse2.X64 -> Sse.X64 -> Object

@CarolEidt
Copy link

Sorry for being a bit late to the discussion on this.
IMO, the use of x64 could indeed be confusing, as it is used in numerous coreclr contexts as a shorthand for x86-64. I think something like 64Bit would be less ambiguous.

Also, I think that this sort of naming issue should be discussed on the corefx issue (https://github.com/dotnet/coreclr/issues/20062), as it is really an ABI discussion, which IMO is not appropriate for a coreclr PR.

@fiigii
Copy link
Author

fiigii commented Oct 9, 2018

@CarolEidt Thanks for the comments. I opened a CoreFX issue https://github.com/dotnet/corefx/issues/32721 to discuss this kind of naming issues.

@pentp
Copy link

pentp commented Nov 6, 2018

@CarolEidt

IMO, the use of x64 could indeed be confusing, as it is used in numerous coreclr contexts as a shorthand for x86-64. I think something like 64Bit would be less ambiguous.

But that is the exact intended meaning here (e.g., Intrinsics.X86.Bmi1.X64 = BMI on x86-64), these intrinsics are not applicable to any other 64-bit platform.
To me X64 looks concise and clear.

@fiigii
Copy link
Author

fiigii commented Nov 6, 2018

@dotnet-bot test this please

@fiigii fiigii closed this Nov 6, 2018
@fiigii fiigii reopened this Nov 6, 2018
@fiigii
Copy link
Author

fiigii commented Nov 7, 2018

Fixed build warnings and comments (platforms->processes).

@fiigii
Copy link
Author

fiigii commented Nov 7, 2018

CoreFX surface change at dotnet/corefx#33286

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

:shipit:

@tannergooding
Copy link
Member

Closing and re-opening to ensure all jobs run (rather than just 7). I doubt this broke anything, but better to be safe than sorry 😄

@fiigii
Copy link
Author

fiigii commented Nov 7, 2018

@dotnet-bot test this please

@fiigii
Copy link
Author

fiigii commented Nov 7, 2018

CI gets stuck again 😂

@fiigii fiigii closed this Nov 7, 2018
@fiigii fiigii reopened this Nov 7, 2018
@fiigii
Copy link
Author

fiigii commented Nov 7, 2018

@dotnet-bot test Ubuntu arm Cross Checked crossgen_comparison Build and Test
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked no_tiered_compilation_innerloop Build and Test
@dotnet-bot test Ubuntu x64 Checked Innerloop Build and Test

@tannergooding
Copy link
Member

test Ubuntu arm Cross Checked Innerloop Build and Test please
test Ubuntu arm Cross Checked crossgen_comparison Build and Test please
test Ubuntu arm Cross Checked no_tiered_compilation_innerloop Build and Test please
test Ubuntu x64 Checked Innerloop Build and Test (Jit - TieredCompilation=0) please
test Ubuntu16.04 arm64 Cross Checked no_tiered_compilation_innerloop Build and Test please
test OSX10.12 x64 Checked Innerloop Build and Test please
test Ubuntu x64 Checked Innerloop Build and Test please
test Ubuntu x64 Formatting please
test Windows_NT x64 Checked Innerloop Build and Test please
test Windows_NT x64 Formatting please

@tannergooding
Copy link
Member

Jenkins seems to be working again, so hopefully the jobs retrigger and don't fail

@tannergooding tannergooding merged commit 002603e into dotnet:master Nov 8, 2018
@tannergooding
Copy link
Member

Thanks @fiigii

@fiigii fiigii deleted the x8664 branch November 8, 2018 06:26
dotnet-maestro-bot pushed a commit to dotnet-maestro-bot/corefx that referenced this pull request Nov 8, 2018
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
dotnet-maestro-bot pushed a commit to dotnet-maestro-bot/corert that referenced this pull request Nov 8, 2018
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
stephentoub pushed a commit to dotnet/corefx that referenced this pull request Nov 8, 2018
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
jkotas pushed a commit to dotnet/corert that referenced this pull request Nov 8, 2018
Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
A-And pushed a commit to A-And/coreclr that referenced this pull request Nov 20, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants