-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Arm build changes #5789
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
Arm build changes #5789
Changes from 1 commit
07acc68
b813fb9
c945a05
37ca2d7
745088b
a50ba08
f277fbf
266208a
3e27e67
0b461b2
955c40b
630e024
07e1fe0
73c3f0f
629b4e6
b207b56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
<IncludeInPackage>Microsoft.ML.FastTree</IncludeInPackage> | ||
<PackageDescription>ML.NET component for FastTree</PackageDescription> | ||
<DefineConstants>$(DefineConstants);NO_STORE;CORECLR</DefineConstants> | ||
<DefineConstants Condition="'$(TargetArchitecture)' != 'Arm64' And '$(TargetArchitecture)' != 'Arm'">$(DefineConstants);USE_FASTTREENATIVE</DefineConstants> | ||
<DefineConstants Condition="'$(TargetArchitecture)' != 'arm64' And '$(TargetArchitecture)' != 'arm'">$(DefineConstants);USE_FASTTREENATIVE</DefineConstants> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is going to work. We only put 1 managed Microsoft.ML.FastTree.dll in our NuGet package. We will either need to build the C# code twice, and include each in a RID-specific section of the NuGet package. Or we will need to continue building the C# code once, and then have a switch at runtime whether we are running on ARM or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I think that having the runtime switch is better. Instead of checking if we are running on arm or not I'll just check if the native dll is present or not. That way even blazer would work with this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Alright, I have added a runtime switch for this. It uses a delegate to pick between the native/managed version so it shouldn't have any performance impact, but if you could take a look I would appreciate it. I am gonig to run the benchmark tests before/after as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the benchmarks are basically identical speedwise before and after these changes. I had our benchmark run 30 times and the mean of the current code and the new code are within .15 seconds of each other on my local machine. So there shouldn't be noticeable performance impact from doing it this way. |
||
<AllowUnsafeBlocks>true</AllowUnsafeBlocks> | ||
<TargetsForTfmSpecificBuildOutput>$(TargetsForTfmSpecificBuildOutput);CopyProjectReferencesToPackage</TargetsForTfmSpecificBuildOutput> | ||
</PropertyGroup> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,8 @@ popd | |
:DoGen | ||
if /i "%3" == "x64" (set __ExtraCmakeParams=%__ExtraCmakeParams% -A x64) | ||
if /i "%3" == "x86" (set __ExtraCmakeParams=%__ExtraCmakeParams% -A Win32) | ||
if /i "%3" == "Arm64" (set __ExtraCmakeParams=%__ExtraCmakeParams% -A Arm64) | ||
if /i "%3" == "arm64" (set __ExtraCmakeParams=%__ExtraCmakeParams% -A arm64) | ||
if /i "%3" == "arm" (set __ExtraCmakeParams=%__ExtraCmakeParams% -A arm) | ||
"%CMakePath%" "-DCMAKE_BUILD_TYPE=%CMAKE_BUILD_TYPE%" "-DCMAKE_INSTALL_PREFIX=%__CMakeBinDir%" "-DMKL_LIB_PATH=%MKL_LIB_PATH%" "-DARCHITECTURE=%3" -G "Visual Studio %__VSString%" %__ExtraCmakeParams% -B. -H%1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you looked how it is done in other repos? For example There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm checking that out right now. They pass the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That eventually turns into this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So it actually looks like they do the same thing. https://github.com/dotnet/runtime/blob/e4b4807e2fae2164d9116fbcdd49ba9044461e7e/src/coreclr/build-runtime.cmd#L450 they a flag There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. Thanks for verifying. |
||
endlocal | ||
GOTO :DONE | ||
|
Uh oh!
There was an error while loading. Please reload this page.