-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Port C# key hardware intrinsics APIs for SSE from SIMD native algorithms #562
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.
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
Merged
Merged
Changes from 36 commits
Commits
Show all changes
43 commits
Select commit
Hold shift + click to select a range
ff5ae22
Implemented SSE support and software fallbacks for key intrinsics
briancylui 8fc9b18
Implemented unit tests for key intrinsics with passing results
briancylui 8e8090d
Implemented performance tests on some key intrinsics with BenchmarkDo…
briancylui 6b3660c
Fixed array pinning issues and solved unreported latency of NativeDotSU
briancylui aa5ee2a
Minor syntax change for style consistency in fixed statements
briancylui f096eb5
Implemented performance tests for all key intrinsics
briancylui 40def43
Simulated user performance with large inputs
briancylui a8f2726
Allow CpuMath to reference C# Hardware Intrinsics APIs.
eerhardt 9355547
Added files for the hierarchical framework to prepare for multi-targe…
briancylui e617f05
Removed the redundant CpuMathUtils.cs file.
briancylui 94751d1
Cleaned up the primitive build constant for featuring intrinsics
briancylui 7a55fcc
Created a new helper class holding C# implementations of SSE intrinsi…
briancylui f76a028
Minor change in naming of variables
briancylui b84047a
Implemented more SSE intrinsics
briancylui bdd41dd
Changed version number of .NET Core App as target framework
briancylui 55dffa5
Cleaned up unit test file that needs to be split into two for multi-t…
briancylui a636173
Fixed seed in performance tests
briancylui 27a042a
Cleaned up unreferenced namespaces
briancylui 27e79a2
Split unit tests into two projects for multi-targetting
briancylui 2d8f373
Cleaned up new intrinsics that are not yet tested to prepare for PR
briancylui 6c3a3e7
Minor style changes
briancylui 8f1ef4c
Added the solution package that includes multi-targeting with UseIntr…
briancylui cf1e0d0
Included all files in the CpuMath project for display in Visual Studi…
briancylui f0f81a5
Removed irrelevant build line from CpuMath
briancylui 29ef073
Response to PR review
briancylui 584bb45
Removed deprecated src\Native\CpuMath working folder
briancylui 7d1f6d1
Removed unnecessary references in unit tests
briancylui 4191c6b
Minor style changes
briancylui aa28480
Fixed SLN file
briancylui 4e50945
Merge with dotnet/master
briancylui 6ef42da
Fixed build error with netcoreapp3.0 not supported
briancylui ff83d66
Minor style fixes
briancylui 2537f2d
Skip netcoreapp3.0 projects when not building for intrinsics
briancylui fa821aa
Merge with dotnet/master
briancylui c6f5f85
Exclude netcoreapp3.0 tests from running by overriding VSTest target
briancylui 0a50f46
Second response to PR feedback
briancylui 946d425
Removed NETCoreAppMaximumVersion tags with modification
briancylui f3ff9b4
Moved VSTest targets to Empty.targets, and parsed -Intrinsics configs…
briancylui f7f5fef
Modified VectorSum to fix perf results
briancylui 5a42d5d
Modified VectorSum to comply with latest C# language updates
briancylui 2d8fe26
Response to PR feedback: added a comment and removed unnecessary MSBu…
briancylui 980db82
Made private functions for SSE intrinsics inline
briancylui 3ff5c0b
Fix merge conflicts
briancylui File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| <Project> | ||
| <PropertyGroup> | ||
| <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects> | ||
| <VSTestTargets Condition="'$(UseIntrinsics)' != 'true' and '$(TargetFramework)' == 'netcoreapp3.0'">ignore.targets</VSTestTargets> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- | ||
| We use netcoreapp3.0 for C# intrinsics, but 3.0 isn't supported in CI or in normal development | ||
| environments yet. So when we are targeting netcoreapp3.0, but aren't building for intrinsics, | ||
| we need to skip the project. | ||
| --> | ||
| <Import Condition="'$(UseIntrinsics)' != 'true' and '$(TargetFramework)' == 'netcoreapp3.0'" | ||
| Project="$(RepoRoot)build\Empty.targets" /> | ||
| </Project> | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| <Project> | ||
| <PropertyGroup> | ||
| <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects> | ||
| </PropertyGroup> | ||
|
|
||
| <!-- | ||
| Copied from https://github.com/dotnet/arcade/blob/master/src/Microsoft.DotNet.Arcade.Sdk/tools/Empty.targets | ||
|
|
||
| Import this file to suppress all targets while allowing the project to participate in the build. | ||
| Workaround for https://github.com/dotnet/sdk/issues/2071. | ||
|
|
||
| The targets defined here are not sufficient for the project to be open in Visual Studio without issues though. | ||
| --> | ||
|
|
||
| <Target Name="_IsProjectRestoreSupported"/> | ||
| <Target Name="Restore"/> | ||
| <Target Name="Build"/> | ||
| <Target Name="Test"/> | ||
| <Target Name="VSTest"/> | ||
| <Target Name="Pack"/> | ||
| </Project> |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do? I'm not familiar. Also, since you are also overriding
VSTest- isn't that enough?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we keep this property setting, we should factor out
'$(UseIntrinsics)' != 'true' and '$(TargetFramework)' == 'netcoreapp3.0'into an MSBuild property so it is only declared once.In reply to: 206735143 [](ancestors = 206735143)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It turns out that the place where we try to blank
VSTest(https://github.com/dotnet/machinelearning/pull/562/files/c6f5f8569ea31bc30e0b7ae54870b9cbf51e2bdb#diff-77ebf330684fd059d7aecec3ce7e6218R19) is processed before the program builds the actualVSTest, so without this line, even though we would have blankedVSTest, the entireVSTestwould be built all over again. Because of this order of the build, one way to solve this issue is to ignore the built targets ofVSTestto ignore the test projects. Not sure if it is the best way to do it so feel free to suggest any optimization.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I helped him with this yesterday and by doing a pp log I found out that
Microsoft.TestPlatform.targetswhere imported after the actualEmpty.targetsfile even though they are specified to be imported after the common targets, I don't know if that is the way it is designed or if it is a bug.So I found out that there was a
VSTestTargetsoverridable property declared in:15.0\Microsoft.Common.targets\ImportAfter\Microsoft.TestPlatform.ImportAfter.targets:17that points toMicrosoft.TestPlatform.targetsand thisImportAfter.targetsfile will import that project if the specified path exists. So in order to override theVSTesttarget I suggested to just set that property to a non-existent targets file.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I'm reading this correctly, that means this line in Empty.targets:
<Target Name="VSTest"/>Isn't doing anything and can be removed, right?
Also - it would probably make sense to move setting this property into
Empty.targets. That would resolve my "we should factor this condition out" comment above as well.build/AfterCommonTargets.targetsis responsible for deciding whether Empty.targets should be imported. Empty.targets decides how to ensure no targets are run (which includes setting this property).Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actually needed because in run-tests.proj we're calling the VSTest target on every test project: https://github.com/dotnet/machinelearning/blob/master/test/run-tests.proj#L10
And if we don't declare it, it will complain saying that the VSTest target doesn't exist in both test projects targeting netcoreapp3.0.
That is right, no sense having the same condition twice, just declare it when
Empty.targetsis imported, which is conditioned already.I also thought of just simply excluding both projects from: https://github.com/dotnet/machinelearning/blob/master/test/run-tests.proj#L5
But it makes more sense to me to do everything inside
Empty.targetssince whenever the repo is ready to build and target netcoreapp3.0 we can just remove this files and we're good to go.