Port C# key hardware intrinsics APIs for SSE from SIMD native algorithms #562

briancylui · 2018-07-20T07:22:05Z

with unit tests and performance tests for two target frameworks: .NET Core App 3.0 and .NET Standard 2.0.
.NET Core App 3.0 gets the new C# hardware intrinsics APIs, while .NET Standard 2.0 gets the original native SIMD algorithms.
Several things have made this multi-targeting feature possible.
1. The new CpuMathUtils class that contains the new APIs is implemented as a partial class with method definitions split into two separate files (src\Microsoft.ML.CpuMath\CpuMathUtils.[target].cs), only one of which is compiled based on the target framework.
2. The Microsoft.ML.CpuMath.csproj file makes the switching decision to compile the right files based on the target framework.

Structure:

All new hardware intrinsics APIs are contained in src\Microsoft.ML.CpuMath.
Unit tests for the two target frameworks live in test\Microsoft.ML.CpuMath.UnitTests.[target], and contain the same content except for the target framework in .csproj.
Performance tests live in test\Microsoft.ML.CpuMath.PerformanceTests.

Changes to users:

Originally, users call intrinsics APIs via the SseUtils class in src\Microsoft.ML.CpuMath\Sse.cs, but now they call them via the new CpuMathUtils class, which will handle switching between SSE and AVX in the future.
CpuMathUtils methods and SseUtils methods share the same signature, but CpuMathUtils methods additionally call a new helper class (SseIntrinsics) for C# implementations of SSE operations.

Future follow-up for `CpuMath` enhancement

Suggestions on CpuMath enhancement in this PR scheduled for future follow-ups have been compiled into an issue page here.

…tNet

Need to multi-target CpuMath for netstandard and netcoreapp2.1. Also, since we are going to move CpuMath into its own NuGet package, remove the dependency from CpuMath to the ML.Core project.

…tting. Note: It will not compile until Microsoft.ML.CpuMath.csproj is changed to adapt to multi-targetting.

…cs to simplify CpuMathUtils.DotNetCoreApp.cs

…argeting

…insics attribute

…o regardless of target framework

- due to working in Mac OSX

dnfclas · 2018-07-20T07:22:16Z

All CLA requirements met.

eerhardt · 2018-07-20T13:53:03Z

Everything in this src\Native\CpuMath folder should be deleted. We don't need it in the official code base.

Refers to: src/Native/CpuMath/Intrinsics.csproj:1 in f0f81a5. [](commit_id = f0f81a5, deletion_comment = False)

eerhardt · 2018-07-20T13:54:07Z

test/Microsoft.ML.CpuMath.UnitTests.DotNetCoreApp/UnitTests.cs

@@ -0,0 +1,451 @@
+using System;


All code files need a copyright header on them. See other .cs files for what the header should look like.

Thank you. Some new files include the header, while some others don't. Will fix it right away.

eerhardt · 2018-07-20T13:55:15Z

test/Microsoft.ML.CpuMath.UnitTests.DotNetCoreApp/UnitTests.cs

+
+namespace Microsoft.ML.CpuMath.UnitTests.DotNetCoreApp
+{
+    public class UnitTests


I think we should give this class a more descriptive name of what it is testing. Probably something similar to CpuMathUtilsTests.

Edited thanks

briancylui · 2018-08-01T00:08:57Z

Issues at the end of today:

Removing NETCoreAppMaximumVersion tag from green build results in errors in running build.cmd -Release. Follow-ups needed.
Building and running perf tests in local machines fail because:
The application to execute does not exist: 'C:\Users\t-brlui\repos\machinelearning\bin\AnyCPU.Release\Microsoft.ML.CpuMath.PerformanceTests\netcoreapp3.0\Microsoft.ML.CpuMath.PerformanceTests.dll'. Follow-ups needed.

eerhardt · 2018-08-01T02:03:34Z

build/AfterCommonTargets.targets

+<Project>
+  <PropertyGroup> 
+    <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
+    <VSTestTargets Condition="'$(UseIntrinsics)' != 'true' and '$(TargetFramework)' == 'netcoreapp3.0'">ignore.targets</VSTestTargets>


What does this do? I'm not familiar. Also, since you are also overriding VSTest - isn't that enough?

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)

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 actual VSTest, so without this line, even though we would have blanked VSTest, the entire VSTest would be built all over again. Because of this order of the build, one way to solve this issue is to ignore the built targets of VSTest to ignore the test projects. Not sure if it is the best way to do it so feel free to suggest any optimization.

I helped him with this yesterday and by doing a pp log I found out that Microsoft.TestPlatform.targets where imported after the actual Empty.targets file 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 VSTestTargets overridable property declared in: 15.0\Microsoft.Common.targets\ImportAfter\Microsoft.TestPlatform.ImportAfter.targets:17 that points to Microsoft.TestPlatform.targets and this ImportAfter.targets file will import that project if the specified path exists. So in order to override the VSTest target I suggested to just set that property to a non-existent targets file.

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.targets is responsible for deciding whether Empty.targets should be imported. Empty.targets decides how to ensure no targets are run (which includes setting this property).

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?

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.

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.targets is responsible for deciding whether Empty.targets should be imported. Empty.targets decides how to ensure no targets are run (which includes setting this property).

That is right, no sense having the same condition twice, just declare it when Empty.targets is 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.targets since whenever the repo is ready to build and target netcoreapp3.0 we can just remove this files and we're good to go.

eerhardt · 2018-08-01T02:07:38Z

test/Microsoft.ML.CpuMath.UnitTests.netcoreapp/Microsoft.ML.CpuMath.UnitTests.netcoreapp.csproj

+    <TargetFramework>netcoreapp3.0</TargetFramework>
+    <IsPackable>false</IsPackable>
+    <!-- The SDK we currently restore and use has 2.0 as the maximum supported version -->
+    <NETCoreAppMaximumVersion>3.0</NETCoreAppMaximumVersion>


This should be able to be removed.

eerhardt · 2018-08-01T02:07:57Z

test/Microsoft.ML.CpuMath.PerformanceTests/Microsoft.ML.CpuMath.PerformanceTests.csproj

+    <TargetFramework>netcoreapp3.0</TargetFramework>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+    <!-- The SDK we currently restore and use has 2.0 as the maximum supported version -->
+    <NETCoreAppMaximumVersion>3.0</NETCoreAppMaximumVersion>


This can be removed.

eerhardt · 2018-08-01T02:10:07Z

Let's not merge this until after we branch for v0.4. We shouldn't add risk to v0.4 with this change...

… for Native build

briancylui · 2018-08-03T17:52:53Z

src/Microsoft.ML.CpuMath/SseIntrinsics.cs

+                vector = Sse3.HorizontalAdd(vector, vector);
+                vector = Sse3.HorizontalAdd(vector, vector);
+            }
+            else


@tannergooding: (Responding to your previous PR comment) Factoring the VectorSum function out of the intrinsics has worked quite well, but my old implementation of VectorSum (which outputs a Vector128<float>) seemed to cause some perf issues (doubled the running time of 4 intrinsics that call VectorSum). I changed it to ToVectorSum, which changes vector in place, but the same perf issue persisted. Then, I (thanks to Santi's help) tried the AggressiveInlining attribute above to make the function inline, which has improved the perf results back to normal. Curious about how it sounds to you, and if there is any better way to do it.

Side question: Should I also make the other private functions - Load1, Load4, Rotate, RotateReverse, and Store4 - inline with the AggressiveInlining attribute to improve perf?

We'll probably want to change private static void ToVectorSum(ref Vector128<float> vector) to private static Vector128<T> ToVectorSum(in Vector128<float> vector), as Vector128<T> was made into a readonly struct, but it otherwise looks good.

Should I also make the other private functions - Load1, Load4, Rotate, RotateReverse, and Store4 - inline with the AggressiveInlining attribute to improve perf

I would think they are already being inlined (except for Store4), if that isn't the case, then the methods are probably small enough that inlining would help. I would recommend benchmarking and looking at the codegen for both having them explicitly inlined and not.

It does not hurt to put AggressiveInlining attribute on these short and internal functions.

For your old code, even though VectorSum is inlined, the codegen is not optimized due to register spill and reload. It seems JIT has optimization opportunity over there. Do you mind opening an issue to discuss about it on CoreCLR github repo?

Thanks for the input! Adding AggressiveInlining attributes for those short functions to mimic the behavior of the existing native code.

I'll pick up logging a bug for CoreCLR. We should get a minimal repro and ensure it isn't already tracked by one of the "first class struct" issues.

eerhardt

Looking good. 2 last things (plus resolving the merge conflict on the .sln file), and I think this can be merged.

Nice work, @briancylui

eerhardt · 2018-08-03T16:21:27Z

src/Native/build.cmd

@@ -86,6 +88,9 @@ if %__IntermediatesDir% == "" (
 set "__CMakeBinDir=%__CMakeBinDir:\=/%"
 set "__IntermediatesDir=%__IntermediatesDir:\=/%"

+if [%CMAKE_BUILD_TYPE:~-11%] == [-Intrinsics] (
+	set CMAKE_BUILD_TYPE=%CMAKE_BUILD_TYPE:~0,-11%


Can you stick a comment here saying what is happening (it isn't obvious from the code).

"Strip the -Intrinsics from the build type"

eerhardt · 2018-08-03T20:57:32Z

src/Microsoft.ML.CpuMath/Microsoft.ML.CpuMath.csproj

    <TargetFramework Condition="'$(UseIntrinsics)' != 'true'">netstandard2.0</TargetFramework>
    <TargetFrameworks Condition="'$(UseIntrinsics)' == 'true'">netstandard2.0;netcoreapp3.0</TargetFrameworks>
    <IncludeInPackage>Microsoft.ML.CpuMath</IncludeInPackage>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <DefineConstants>$(DefineConstants);CORECLR;PRIVATE_CONTRACTS</DefineConstants>
+    <LangVersion>7.3</LangVersion>
+    <!-- The SDK we currently restore and use has 2.0 as the maximum supported version -->
+    <NETCoreAppMaximumVersion>3.0</NETCoreAppMaximumVersion>


This can be removed.

Agree with removing the NETCoreAppMaximumVersion tag.

In a recent fix of the VectorSum function in SseIntrinsics.cs, since we need to rely on the read-only feature of Vector128 introduced in C# 7.2 to change the vector in-place, the Lang Version has to be at least 7.2. However, since we use the feature of extensible fixed statements in SseIntrinsics.cs, which is a feature in C# 7.3, the LangVersion restriction has been set to 7.3. Reference: dotnet/roslyn#24469

Please let me know how I should proceed.

sorry, I was specifically talking about <NETCoreAppMaximumVersion>3.0</NETCoreAppMaximumVersion>. The LangVersion change is needed.

…ild tags

briancylui · 2018-08-03T22:33:31Z

Responded to the second round of review, and now this PR is ready for a third round if any. Gathering thoughts on this PR before fixing the merge conflicts in the sln file. Please let me know if there is anywhere else I should improve my code. How does it sound to everyone?

briancylui · 2018-08-03T23:26:08Z

Merge conflicts have been fixed. Green builds for all 5 checks.

eerhardt

Looks great. Nice work, Brian.

eerhardt · 2018-08-03T23:43:59Z

test/Microsoft.ML.CpuMath.PerformanceTests/Program.cs

+                .Run(null, CreateClrVsCoreConfig());
+        }
+
+        private static IConfig CreateClrVsCoreConfig()


No worries. We can leave it like this for now.

safern · 2018-08-04T00:45:24Z

test/Microsoft.ML.CpuMath.PerformanceTests/SsePerformanceTests.cs

+        private const int DEFAULT_SEED = 253421;
+        private const float DEFAULT_SCALE = 1.11f;
+
+        private float[] src, dst, original, src1, src2;


Nit: This all non constant privates should be prefixed with _ but it can be updated in your next PR to not block this one.

Updated the issue page (briancylui#2) to include this action item for future follow-up. Thank you for your input!

safern

Not an expert in the intrinsic operations but looks good to me. Nice work Brian!

briancylui · 2018-08-04T02:16:01Z

Thank you all reviewers for your time reviewing this PR and making great suggestions to fix issues or improve performance. I have learnt a lot from all of you.

Since I don't have write access to the dotnet/machinelearning repo, may anyone with write access please help merging this PR to the master? Much appreciated!

After the merging, I can then start writing more SSE intrinsics in a new branch.

briancylui and others added 24 commits July 19, 2018 16:06

Implemented SSE support and software fallbacks for key intrinsics

ff5ae22

Implemented unit tests for key intrinsics with passing results

8fc9b18

Implemented performance tests on some key intrinsics with BenchmarkDo…

8e8090d

…tNet

Fixed array pinning issues and solved unreported latency of NativeDotSU

6b3660c

Minor syntax change for style consistency in fixed statements

aa5ee2a

Implemented performance tests for all key intrinsics

f096eb5

Simulated user performance with large inputs

40def43

Allow CpuMath to reference C# Hardware Intrinsics APIs.

a8f2726

Need to multi-target CpuMath for netstandard and netcoreapp2.1. Also, since we are going to move CpuMath into its own NuGet package, remove the dependency from CpuMath to the ML.Core project.

Added files for the hierarchical framework to prepare for multi-targe…

9355547

…tting. Note: It will not compile until Microsoft.ML.CpuMath.csproj is changed to adapt to multi-targetting.

Removed the redundant CpuMathUtils.cs file.

e617f05

Cleaned up the primitive build constant for featuring intrinsics

94751d1

Created a new helper class holding C# implementations of SSE intrinsi…

7a55fcc

…cs to simplify CpuMathUtils.DotNetCoreApp.cs

Minor change in naming of variables

f76a028

Implemented more SSE intrinsics

b84047a

Changed version number of .NET Core App as target framework

bdd41dd

Cleaned up unit test file that needs to be split into two for multi-t…

55dffa5

…argeting

Fixed seed in performance tests

a636173

Cleaned up unreferenced namespaces

27a042a

Split unit tests into two projects for multi-targetting

27e79a2

Cleaned up new intrinsics that are not yet tested to prepare for PR

2d8f373

Minor style changes

6c3a3e7

Added the solution package that includes multi-targeting with UseIntr…

8f1ef4c

…insics attribute

Included all files in the CpuMath project for display in Visual Studi…

cf1e0d0

…o regardless of target framework

Removed irrelevant build line from CpuMath

f0f81a5

- due to working in Mac OSX

This was referenced Jul 20, 2018

Port native SIMD algorithms for SSE to managed code #552

Closed

Progress on porting ML.NET native SIMD algorithms to managed code briancylui/machinelearning#1

Open

eerhardt reviewed Jul 20, 2018

View reviewed changes

briancylui added 3 commits July 31, 2018 13:01

Merge with dotnet/master

fa821aa

Exclude netcoreapp3.0 tests from running by overriding VSTest target

c6f5f85

Second response to PR feedback

0a50f46

eerhardt reviewed Aug 1, 2018

View reviewed changes

briancylui added 3 commits August 1, 2018 12:38

Removed NETCoreAppMaximumVersion tags with modification

946d425

Moved VSTest targets to Empty.targets, and parsed -Intrinsics configs…

f3ff9b4

… for Native build

Modified VectorSum to fix perf results

f7f5fef

briancylui commented Aug 3, 2018

View reviewed changes

Modified VectorSum to comply with latest C# language updates

5a42d5d

eerhardt reviewed Aug 3, 2018

View reviewed changes

briancylui added 2 commits August 3, 2018 15:02

Response to PR feedback: added a comment and removed unnecessary MSBu…

2d8fe26

…ild tags

Made private functions for SSE intrinsics inline

980db82

Fix merge conflicts

3ff5c0b

eerhardt approved these changes Aug 3, 2018

View reviewed changes

safern reviewed Aug 4, 2018

View reviewed changes

safern approved these changes Aug 4, 2018

View reviewed changes

tannergooding approved these changes Aug 6, 2018

View reviewed changes

eerhardt merged commit ac4fead into dotnet:master Aug 6, 2018

briancylui changed the title ~~Port C# hardware intrinsics APIs for SSE from SIMD native algorithms~~ Port C# key hardware intrinsics APIs for SSE from SIMD native algorithms Aug 8, 2018

briancylui mentioned this pull request Aug 8, 2018

Port all active C# hardware intrinsics APIs for SSE from SIMD native algorithms #668

Merged

eerhardt mentioned this pull request Aug 15, 2018

Optimize some Matrix4x4 operations with SSE dotnet/corefx#31779

Merged

briancylui mentioned this pull request Aug 18, 2018

Port all relevant AVX hardware intrinsics C# APIs from SIMD native algorithms #691

Merged

ghost locked as resolved and limited conversation to collaborators Mar 29, 2022

Port C# key hardware intrinsics APIs for SSE from SIMD native algorithms #562

Port C# key hardware intrinsics APIs for SSE from SIMD native algorithms #562

Uh oh!

Conversation

briancylui commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Structure:

Changes to users:

Future follow-up for CpuMath enhancement

Uh oh!

dnfclas commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eerhardt commented Jul 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briancylui commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briancylui commented Aug 3, 2018

Uh oh!

briancylui commented Aug 3, 2018

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

safern left a comment

briancylui commented Jul 20, 2018 •

edited

Loading

Future follow-up for `CpuMath` enhancement

dnfclas commented Jul 20, 2018 •

edited

Loading

safern Aug 1, 2018 •

edited

Loading