Enable SymbolicSgdLogisticRegression (SymSgdNative) on arm64 - #7671
Open
vladimir-aubrecht wants to merge 2 commits into
Open
Enable SymbolicSgdLogisticRegression (SymSgdNative) on arm64#7671vladimir-aubrecht wants to merge 2 commits into
vladimir-aubrecht wants to merge 2 commits into
Conversation
Build SymSgdNative and a small self-contained libMklImports shim on arm/arm64 so the SymbolicSgdLogisticRegression trainer works there without Intel MKL. - MklImportsArm: implement the four CBLAS routines SymSGD needs (sdot, saxpy, sdoti, saxpyi) as portable C with no external BLAS dependency, plus DFTI stubs. Drop find_package(BLAS) so it configures in the CI cross-compilation sysroots (which ship no BLAS). Export the symbols explicitly since the native build uses -fvisibility=hidden. - CMake: build MklImportsArm + SymSgdNative on arm, link SymSgdNative against the shim, and make the CBLAS calling convention portable. - Directory.Build.targets: ship libMklImports and libSymSgdNative next to the managed assemblies on arm. - SymSgdClassificationTrainer: marshal the native bool parameters of LearnAll as I1. The default 4-byte bool marshalling corrupts later stack arguments and segfaults on arm64. Fixes dotnet#5798 Co-authored-by: Anna Maresova <anicka@anicka.net> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Author
|
@dotnet-policy-service agree company="Microsoft" |
Contributor
There was a problem hiding this comment.
Pull request overview
Enables SymbolicSgdLogisticRegression / SymSgdNative on arm/arm64 by adding an ARM-specific MklImports shim (implementing the small subset of CBLAS APIs SymSGD needs), wiring it into the native build, and fixing managed P/Invoke marshalling that can crash on arm64.
Changes:
- Add
src/Native/MklImportsArm/to build a self-containedMklImportsshim on arm/arm64 and linkSymSgdNativeagainst it. - Update native build gating so
SymSgdNativeis built on arm/arm64 and shipped next to managed assemblies. - Fix
LearnAllP/Invokeboolmarshalling to avoid stack corruption / SIGSEGV on arm64.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Native/SymSgdNative/SparseBLAS.h | Makes CBLAS calling convention portable across Windows vs non-Windows builds. |
| src/Native/SymSgdNative/CMakeLists.txt | Adjusts how MklImports is resolved/linked on arm platforms. |
| src/Native/MklImportsArm/MklImportsArm.c | Implements the minimal CBLAS subset + DFTI stubs for arm/arm64 as a self-contained shim. |
| src/Native/MklImportsArm/CMakeLists.txt | Adds CMake target to build/install the ARM MklImports shim. |
| src/Native/CMakeLists.txt | Enables building SymSgdNative on arm/arm64 and adds MklImportsArm to the build graph. |
| src/Microsoft.ML.Mkl.Components/SymSgdClassificationTrainer.cs | Fixes P/Invoke bool marshalling for LearnAll to prevent arm64 crashes. |
| Directory.Build.targets | Ensures MklImports and SymSgdNative are copied for arm/arm64 outputs (no longer removed). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…Descriptor, fix misleading comment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Enables the
SymbolicSgdLogisticRegressiontrainer (backed bySymSgdNative) on arm/arm64, where Intel MKL is unavailable. Fixes #5798.SymSGD uses only four CBLAS routines (
sdot,saxpy,sdoti,saxpyi). This PR provides them via a tiny, self-containedlibMklImportsshim built from portable C — with no external BLAS dependency — so it configures and links in the CI cross-compilation sysroots (which ship no OpenBLAS/BLAS).Changes
src/Native/MklImportsArm/(new): implements the four CBLAS routines as plain C loops (-O3autovectorizes the dense paths to NEON) plus MKL DFTI stubs. Nofind_package(BLAS)— zero external deps. Symbols are exported explicitly because the native build uses-fvisibility=hidden.src/Native/CMakeLists.txt: buildMklImportsArm+SymSgdNativeon arm.src/Native/SymSgdNative/CMakeLists.txt: linkSymSgdNativeagainst the shim on arm.src/Native/SymSgdNative/SparseBLAS.h: make the CBLAS calling convention portable (__cdeclonly on_WIN32).Directory.Build.targets: shiplibMklImportsandlibSymSgdNativenext to the managed assemblies on arm.SymSgdClassificationTrainer.cs: marshalLearnAll's nativeboolparameters asUnmanagedType.I1. The default 4-byteboolmarshalling corrupts later stack arguments and segfaults on arm64 (works on x64 by luck).Testing
Built native (Release + Debug) and ran the SymSGD trainer tests on Apple Silicon (arm64):
TestEstimatorSymSgdClassificationTrainerTestEstimatorSymSgdInitPredictorSimpleTrainAndPredictSymSGD3 passed, 0 failed. Without the
boolmarshalling fix, these crash the test host with a SIGSEGV insideLearnAll.Notes
SparseBLAS.hcalling-convention guard) is based on prior work by @anicka-net (credited as co-author).Microsoft.ML.Mkl.Components(OLS,VectorWhitening) andMicrosoft.ML.TimeSeriesstill require a real BLAS/LAPACK and are out of scope here.