Skip to content

Allow CpuMath to reference C# Hardware Intrinsics APIs. #542

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 2 commits into from
Jul 19, 2018

Conversation

eerhardt
Copy link
Member

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

Add a build parameter to enable building against .NET Core 3.0's Runtime Intrinsics APIs.

Fix #534

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

Add a build parameter to enable building against .NET Core 3.0's Runtime Intrinsics APIs.

Fix dotnet#534
</ItemGroup>
<ItemGroup Condition="'$(UseIntrinsics)' == 'true'">
<NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)CpuMathNative$(NativeLibExtension)"
RelativePath="Microsoft.ML.CpuMath\runtimes\$(PackageRid)\lib\netstandard2.0" />
Copy link
Member

Choose a reason for hiding this comment

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

Don't put native files under lib. .NETCore will not like this. For native libraries that must be constrained by both tfm and RID use nativeassets.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, perfect. I was unaware of this and looked on the docs, but didn't find it.

Thanks for the info. I'll make the switch.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I poked the doc issue on it.


<PropertyGroup>
<TargetFramework>netstandard2.0</TargetFramework>
<IncludeInPackage>Microsoft.ML</IncludeInPackage>
<Configurations>Debug;Release;Debug-Intrinsics;Release-Intriniscs</Configurations>
Copy link
Member

Choose a reason for hiding this comment

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

Who sets this configuration?

Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: "Release-Intriniscs" could probably be "Release-Intrinsics".

Copy link
Member Author

Choose a reason for hiding this comment

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

@briancylui - nice catch.

@ericstj - the idea is that this build switch is turned off by default. While developing his feature, @briancylui can switch VS to use these 2 new configurations using the configuration manager to build/run tests locally.

Copy link
Member

Choose a reason for hiding this comment

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

I see, and you aren't going to turn it on in CI or official build yet?

Copy link
Member Author

Choose a reason for hiding this comment

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

For sure, "no" for official build. I don't want to affect the official product until we are ready to affect it (3.0 doesn't even have a preview out yet, so getting it into customers hands isn't a critical concern for me).

We will eventually turn CI on for it, especially as we move closer to wanting to put it in the official product.

@TomFinley
Copy link
Contributor

Hi @eerhardt ! So, I might have expected that there would be some factoring out of CpuMath to be perhaps some sort of nuget dependency, and whether or not it uses .NET intrinsics or native code would not be done by a global build configuration, but a build configuration local to that (factored out) project. Is this sort of "global" build configuration a temporary measure, or do we think this is an actual final state for the code?

There's something just a little unsettling about having this sort of multiplicative effect of "doubling" the number of targets, since it seems like a similar doubling would occur every time we have a situation where we want to exploit special functionality in some .NET non-Standard (in this case .NET Core 3.0 I guess?). Is this really the best way?

@eerhardt
Copy link
Member Author

Is this sort of "global" build configuration a temporary measure, or do we think this is an actual final state for the code?

This is definitely a temporary measure to unblock @briancylui while still allowing the existing devs and users to develop and consume the NuGet packages as they do today. Typically, the way multi-targeting works is that a .csproj would just have to say <TargetFrameworks>net45;netstandard2.0;netcoreapp2.0</TargetFrameworks> and not need the "Configuration" changes I am proposing here. Then when building, 3 .dlls would be produced, one for each of those frameworks.

The issue lies in that we need to target netcoreapp3.0, which has not shipped yet, and you can only build for netcoreapp3.0 if you have a "3.0" .NET Core SDK installed, which most people don't have. So it is not possible to build for netcoreapp3.0 by default, without requiring the current devs to have to install pre-release "3.0" .NET Core SDK bits on their machine. That's why the $(UseIntrinsics) property is being introduced. Only when that property is set will it attempt to build for netcoreapp3.0, and the property isn't set by default.

Now the question becomes: How to set $(UseIntrinsics) when @briancylui (or anyone else) wants to develop and experiment with the C# intrinsics feature? Options I know of:

  • Set an environment variable (which turns into a msbuild property) before they launch VS
  • Manually change the .csproj file to set the variable, and remember to not check that change in
  • Use VS Configurations

It felt like VS Configurations were the most natural way to enable this (just like Debug vs. Release builds - all they really do is change some msbuild properties, which then affects what the compiler does).

There's something just a little unsettling about having this sort of multiplicative effect of "doubling" the number of targets, since it seems like a similar doubling would occur every time we have a situation where we want to exploit special functionality in some .NET non-Standard (in this case .NET Core 3.0 I guess?). Is this really the best way?

I agree doubling each time is not a great experience. Eventually, when we officially support .NET Core 3.0, these configurations will go away, and the C# intrinsics will be compiled by default.
The other side of this story is that C# intrinsics are the only .NET Core 3.0-specific feature that I've heard we are going to support for ML.NET v1.0. And even if there are more for v1, we would only consider taking it if it was cheap to maintain in our build system.

@TomFinley
Copy link
Contributor

All right that sounds fine @eerhardt !

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

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

Thank you @eerhardt and @briancylui

@@ -78,9 +78,9 @@
</ItemGroup>
<ItemGroup Condition="'$(UseIntrinsics)' == 'true'">
<NativePackageAsset Include="$(NativeAssetsBuiltPath)\$(NativeLibPrefix)CpuMathNative$(NativeLibExtension)"
RelativePath="Microsoft.ML.CpuMath\runtimes\$(PackageRid)\lib\netstandard2.0" />
RelativePath="Microsoft.ML.CpuMath\runtimes\$(PackageRid)\nativeassets\netstandard2.0" />
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't nativeassets be under x86, x64, or arm folders (rather than netstandard2.0?

Copy link
Member Author

Choose a reason for hiding this comment

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

The CpuMathNative native binaries are under both RID and TFM folders. Both RID and TFM can be used to decide which asset to be picked. NuGet supports pivoting on both of these properties.

runtimes\$(Rid)\nativeassets\$(Tfm)

The idea is that on netcoreapp3.0, we won't even need this native binary, because it will no longer be called. All of the logic in it will be replaced with C# intrinsic methods.

So, when running on a non-netcoreapp3.0 platform (like .NET Core 2.1, or netfx), use the CpuMathNative native binary for the appropriate RID. But when running on netcoreapp3.0, we use a placeholder _._ file to tell NuGet no asset should be picked (since we don't need a native asset on this TFM).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I missed the rid section, which covers the bitness of the architecture

👍

@eerhardt eerhardt merged commit a307c6a into dotnet:master Jul 19, 2018
@eerhardt eerhardt deleted the RefactorCpuMath branch July 19, 2018 16:48
eerhardt added a commit to eerhardt/machinelearning that referenced this pull request Jul 27, 2018
* Allow CpuMath to reference C# Hardware Intrinsics APIs.

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

Add a build parameter to enable building against .NET Core 3.0's Runtime Intrinsics APIs.

Fix dotnet#534

* Respond to PR feedback.
codemzs pushed a commit to codemzs/machinelearning that referenced this pull request Aug 1, 2018
* Allow CpuMath to reference C# Hardware Intrinsics APIs.

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

Add a build parameter to enable building against .NET Core 3.0's Runtime Intrinsics APIs.

Fix dotnet#534

* Respond to PR feedback.
@ghost ghost locked as resolved and limited conversation to collaborators Mar 29, 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.

5 participants