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

Add Intel hardware intrinsic API implementation to mscorlib#13576

Merged
jkotas merged 1 commit into
dotnet:masterfrom
fiigii:intelintrinsic
Sep 5, 2017
Merged

Add Intel hardware intrinsic API implementation to mscorlib#13576
jkotas merged 1 commit into
dotnet:masterfrom
fiigii:intelintrinsic

Conversation

@fiigii
Copy link
Copy Markdown

@fiigii fiigii commented Aug 24, 2017

This PR is the complete design of API Proposal: Add Intel hardware intrinsic functions and namespace #22940 and the mscorlib counterpart of Add Intel hardware intrinsic APIs to CoreFX #23489.

For building with the current code base, const parameter modifiers and [intrinsic] is temporarily removed.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Aug 24, 2017

// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.
// ------------------------------------------------------------------------------
// Changes to this file must follow the http://aka.ms/api-review process.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This comment does not apply here.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see. Will fix.

/// <summary>
/// This class provides access to Intel AES hardware instructions via intrinsics
/// </summary>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: Unnecessary blank line

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Will remove.

{
public static bool IsSupported {get;}
}
} No newline at end of file
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: Add new lines at the end of the file

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

[StructLayout(LayoutKind.Sequential, Size = 32)]
public struct Vector256<T> where T : struct
{
public static bool IsSupported {get;}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The IsSupported properties should return false. (And the intrinsic JIT implementation should override it to return true.)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

@fiigii fiigii force-pushed the intelintrinsic branch 3 times, most recently from 627ed78 to 8e6acef Compare August 31, 2017 17:54
@fiigii
Copy link
Copy Markdown
Author

fiigii commented Aug 31, 2017

Change build condition to AnyCPU for leaving these intrinsics as stubs on other platforms. Successfully build with CoreFX change. @jkotas PTAL.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 1, 2017

Update

Separate SSE intrinsics from Sse2 class. New file Sse.cs and class Sse added.

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 1, 2017

@jkotas I have addressed all the feedback and rebased to solve conflicts. Could you please take a look?

@jkotas
Copy link
Copy Markdown
Member

jkotas commented Sep 1, 2017

LGTM

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 4, 2017

@dotnet-bot test OSX10.12 x64 Checked Build and Test
@dotnet-bot test Windows_NT x86 full_opt legacy_backend CoreCLR Perf Tests Correctness

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 5, 2017

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

@fiigii
Copy link
Copy Markdown
Author

fiigii commented Sep 5, 2017

@jkotas Could you please merge this PR? Then we can test and build the CoreFX counterpart in the CI system.

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.

5 participants