Add OneMATH support as an alternative to oneMKL - #323
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a OneMath-based build/runtime path (behind USE_ONEMATH) across several SYCL benchmarks, updating BLAS/RNG calls and Makefiles to support selecting OneMath vs oneMKL.
Changes:
- Introduces
USE_ONEMATHcompile-time switch andmath_nsalias to route calls to OneMath or oneMKL. - Updates Makefiles to link against OneMath libraries (including CUDA/HIP backends) when enabled.
- Adjusts/extends verification logic and output (PASS/FAIL) in several benchmarks.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rayleighBenardConvection-sycl/main.cpp | Routes BLAS calls through math_ns to support OneMath/oneMKL. |
| src/rayleighBenardConvection-sycl/Makefile | Adds OneMath build/link options and USE_ONEMATH define. |
| src/norm2-sycl/main.cpp | Switches to math_ns for nrm2 and updates verification threshold. |
| src/norm2-sycl/Makefile | Adds OneMath knobs and adjusts includes/linking logic accordingly. |
| src/dropout-sycl/main.cpp | Adds OneMath RNG headers and routes device RNG calls through math_ns. |
| src/dropout-sycl/Makefile | Adds OneMath knobs and USE_ONEMATH compile path. |
| src/blas-gemmStridedBatched-sycl/main.cpp | Routes batched GEMM calls through math_ns and improves verification output. |
| src/blas-gemmStridedBatched-sycl/Makefile | Adds OneMath build/link options and USE_ONEMATH define. |
| src/blas-gemmEx-sycl/main.cpp | Adds OneMath support, AdaptiveCpp guards, and new reference/verification helpers. |
| src/blas-gemmEx-sycl/Makefile | Adds OneMath build/link options and USE_ONEMATH define. |
| src/blas-gemmBatched-sycl/main.cpp | Routes batched GEMM calls through math_ns and improves verification output. |
| src/blas-gemmBatched-sycl/Makefile | Adds OneMath build/link options and USE_ONEMATH define. |
| src/blas-gemm-sycl/main.cpp | Adds OneMath support and replaces MKL allocation + updates verification logic. |
| src/blas-gemm-sycl/Makefile | Adds OneMath build/link options and USE_ONEMATH define. |
| src/blas-dot-sycl/main.cpp | Routes dot to math_ns, adds OneMath timing label, and adjusts thresholds/AdaptiveCpp guards. |
| src/blas-dot-sycl/Makefile | Adds OneMath knobs and linking, plus USE_ONEMATH compile path. |
Suppressed comments (2)
src/blas-gemm-sycl/main.cpp:14
sycl::malloc_host<T>expects an element count, butA_size/B_size/C_sizeare byte sizes, which leads to incorrect allocations and will also make the later comparison loop incorrect because it iteratesi < C_size(bytes) while indexingc[i]/r[i]as elements. Allocate with element counts (m*k,k*n,m*n) and iterate overm*nelements. Also, the code usescolumn_major::gemmfor the correctness check butrow_major::gemmfor the timed loop with the same pointers/leading dimensions—this is a behavioral change that can make timing run a different computation/layout than what was validated; use a consistent layout for both paths. Lastly, the output string has a typo: 'missmatch' should be 'mismatch'.
src/blas-gemmEx-sycl/main.cpp:85modeis accepted (and callers pass bf16/tf32 modes), but it is not used when dispatching togemm, so the requested compute mode has no effect. Either (mandatory) call the API variant that accepts/implements compute mode for the backend(s) that support it, or (optional) remove the parameter/feature flag and corresponding test prints to avoid reporting modes that aren’t actually applied.
status = math_ns::blas::column_major::gemm(
q,
transA, transB,
m, n, k,
alpha, A, lda,
B, ldb, beta,
C, ldc);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ONEMKL_PATH ?= /path/to/oneMKL | ||
|
|
||
| # path to the install of the oneMath library | ||
| ONEMATH_PATH = /path/to/oneMath |
There was a problem hiding this comment.
This is done for every benchmark
|
@thomasRoglin Thank you for the pull requests. Are Copilot's reviews helpful ? |
- Use OneMath instead of OneMKL if the USE_ONEMATH macro is define - Support AdaptiveCpp by skipping compilation of FP16 and BF16 if compiler is not dpcpp - Change verification threshold from 1e-1 to 1% of the sum, otherwise fp32 would fail
- Use standard way to link to onemkl in Makefile
- Use OneMath instead of OneMKL if the USE_ONEMATH macro is define
- Change verification threshold from 1e-1 to 1% of the sum, otherwise
verification fail
- Use OneMath instead of OneMKL if the USE_ONEMATH macro is define - Support AdaptiveCpp by skipping bf16 and int8 tests if compiler is not DPCPP
- Use OneMath instead of OneMKL if the USE_ONEMATH macro is define
- Use OneMath instead of OneMKL if the USE_ONEMATH macro is define
- Use OneMath instead of OneMKL if the USE_ONEMATH macro is define
- Replace mkl_malloc by standard sycl alloc
- Improve verification to add tolerance
- Use OneMath instead of OneMKL if the USE_ONEMATH macro is define
- Clarify verification by printing PASS or FAIL
- Use OneMath instead of OneMKL if the USE_ONEMATH macro is define
- Clarify verification by printing PASS or FAIL
e7c8b5f to
9c90f6f
Compare
|
Hello, Copilot reviews were relevant and are now implemented.. Let me know if there is anything else to modify |
|
@thomasRoglin Thank you for the updates! I have some comments about oneMath itself if that is right: • oneMath lacks |
Summary
This PR adds OneMath API support to multiple benchmarks and improves verification.
USE_ONEMATHmacro as an alternative to OneMKLDetail by benchmarks :
gemmStridedBatched & gemmBatched
USE_ONEMATHmacrogemm
USE_ONEMATHmacromkl_mallocwith standard SYCL allocationgemmEx
USE_ONEMATHmacrodot
norm2
USE_ONEMATHmacrodropout
USE_ONEMATHmacrorayleighBenardConvection
USE_ONEMATHmacro