Refactor abstract layer for matmul_veclib() and enable blas on Windows#835
Conversation
998822d to
b6febde
Compare
ThreeMonth03
left a comment
There was a problem hiding this comment.
@yungyuc Please take a look.
| # Reference: https://www.openmathlib.org/OpenBLAS/docs/install/ | ||
| - name: install OpenBLAS | ||
| run: | | ||
| vcpkg install openblas:x64-windows | ||
| $openblas_bin_path = @( | ||
| "C:\vcpkg\installed\x64-windows\bin", | ||
| "C:\vcpkg\installed\x64-windows\debug\bin" | ||
| ) -join ";" | ||
| echo "$openblas_bin_path" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append | ||
|
|
There was a problem hiding this comment.
This library doesn't includes lapack, so I think we need another pull request to write the script about source build on Windows.
There was a problem hiding this comment.
Openblas includes LAPACK. Why this openblas you install using vcpkg does not?
There was a problem hiding this comment.
Remove the check of cblas because we assume that every machine has installed cblas now.
| def assert_matmul(self, lhs, rhs, expected): | ||
| result = lhs.matmul(rhs) | ||
|
|
||
| self.assertEqual(list(result.shape), list(expected.shape)) | ||
| np.testing.assert_array_almost_equal(result.ndarray, expected) | ||
| return result | ||
|
|
||
| def assert_matmul_fast(self, lhs, rhs, expected, matmul_result): | ||
| fast_result = lhs.matmul_fast(rhs) | ||
|
|
||
| self.assertEqual(list(fast_result.shape), list(expected.shape)) | ||
| np.testing.assert_array_almost_equal(fast_result.ndarray, expected) | ||
| np.testing.assert_array_almost_equal(fast_result.ndarray, | ||
| matmul_result.ndarray) | ||
| return fast_result | ||
|
|
||
| def assert_matmul_veclib(self, lhs, rhs, expected, matmul_result): | ||
| veclib_unavailable = ( | ||
| "SimpleArray::matmul_veclib(): " | ||
| "Accelerate/CBLAS matmul is only " | ||
| "available on Apple Silicon" | ||
| ) | ||
|
|
||
| try: | ||
| veclib_result = lhs.matmul_veclib(rhs) | ||
| except RuntimeError as exc: | ||
| # FIXME: Split veclib backend coverage into dedicated tests and | ||
| # mark unsupported platforms as expected failures once a follow-up | ||
| # issue is filed. | ||
| self.assertEqual(str(exc), veclib_unavailable) | ||
| return | ||
| veclib_result = lhs.matmul_veclib(rhs) |
There was a problem hiding this comment.
Add helper function for checking same patterns.
There was a problem hiding this comment.
I'm not sure the abstract layer should be put in folder /math.
There was a problem hiding this comment.
It's OK to keep lapack_compact.hpp here under math/ for now.
There was a problem hiding this comment.
Abstract layer of blas.
| namespace modmesh | ||
| { | ||
|
|
||
| namespace blas |
There was a problem hiding this comment.
I think these api should be wrapped in something like namespace blas, but I barely see another namespace in namespace modmesh, except namespace detail;.
There was a problem hiding this comment.
No, we do not use an additional namespace for them. Please remove the namespace blas.
| Complex<double> const * matrix, | ||
| Complex<double> const * vector, | ||
| Complex<double> * result, | ||
| MatrixOperation operation = MatrixOperation::none); |
There was a problem hiding this comment.
These functions are used in matmul_vec_vec_veclib();, matmul_vec_mat_veclib(); and return matmul_mat_vec_veclib();.
There was a problem hiding this comment.
Remove abstract layer here.
| # Reference: https://www.openmathlib.org/OpenBLAS/docs/install/ | ||
| - name: install OpenBLAS | ||
| run: | | ||
| vcpkg install openblas:x64-windows | ||
| $openblas_bin_path = @( | ||
| "C:\vcpkg\installed\x64-windows\bin", | ||
| "C:\vcpkg\installed\x64-windows\debug\bin" | ||
| ) -join ";" | ||
| echo "$openblas_bin_path" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append | ||
|
|
There was a problem hiding this comment.
Openblas includes LAPACK. Why this openblas you install using vcpkg does not?
There was a problem hiding this comment.
It's OK to keep lapack_compact.hpp here under math/ for now.
| #pragma once | ||
|
|
||
| /* | ||
| * Copyright (c) 2026, modmesh contributors. |
| namespace modmesh | ||
| { | ||
|
|
||
| namespace blas |
There was a problem hiding this comment.
No, we do not use an additional namespace for them. Please remove the namespace blas.
| */ | ||
|
|
||
| #include <modmesh/buffer/small_vector.hpp> | ||
| #include <modmesh/math/Complex.hpp> |
| @@ -0,0 +1,402 @@ | |||
| /* | |||
| * Copyright (c) 2026, modmesh contributors. | |||
| void check_inner(size_t lhs_idx, size_t rhs_idx) const; | ||
| void check_tiles() const; | ||
| A matmul_vec_vec(); | ||
| A matmul_vec_vec_veclib(); |
There was a problem hiding this comment.
Name the wrapper to vendor code as *_blas(). veclib is for Apple accelerate/veclib, not all vendor library.
veclib was used elsewhere for vendor lib, because initially the only vendor lib was only accelerate/veclib. A separate issue should be created to refactor.
matmul_veclib() with cblas on Linux and Windows platformmatmul_veclib() and enable blas on Windows
fcc2654 to
9fcfa1b
Compare
ThreeMonth03
left a comment
There was a problem hiding this comment.
- Clarify: why vcpkg openblas does not have lapack.
- Remove the namespace blas.
- Name the wrapper to vendor code as *_blas().
I think there are some existing problem in CI now. @yungyuc, please review this pull request after CI works as usual.
| # vcpkg's openblas:x64-windows provides CBLAS but not LAPACK, | ||
| # since the vcpkg port explicitly builds openblas without LAPACK. | ||
| # References: | ||
| # - https://github.com/microsoft/vcpkg/blob/9733af874ef1d4c357ae0d7b4c0fc669e5a5e012/ports/openblas/portfile.cmake#L48-L56 | ||
| - name: install OpenBLAS | ||
| run: | | ||
| vcpkg install openblas:x64-windows | ||
| $openblas_bin_path = @( | ||
| "C:\vcpkg\installed\x64-windows\bin", | ||
| "C:\vcpkg\installed\x64-windows\debug\bin" | ||
| ) -join ";" | ||
| echo "$openblas_bin_path" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append | ||
|
|
There was a problem hiding this comment.
In the reference, vcpkg port builds openblas without turning on the flag -DBUILD_WITHOUT_LAPACK=ON.
It means that vcpkg install openblas:x64-windows doesn't install lapack.
There was a problem hiding this comment.
Please also clarify why vcpkg openblas turns off lapack.
modmesh uses both BLAS and LAPACK. The Linux installation has both. If Windows installation of openblas does not include both, please provide a way to supplement LAPACK.
We can discuss more.
There was a problem hiding this comment.
vcpkg also affords lapack port for windows, so lapack could be installed by the following command:
vcpkg install lapack:x64-windows
vcpkg's developers mentioned that they prefer dividing LAPACK and OpenBLAS to two ports, so vcpkg openblas turns off lapack. However, they don't explain the motivation furthermore.
There was a problem hiding this comment.
In that case, we should install both blas and lapack so that the dependency is aligned across macos, linux, and windows.
| float dot_blas(size_t size, float const * lhs, float const * rhs); | ||
| double dot_blas(size_t size, double const * lhs, double const * rhs); | ||
| Complex<float> dot_blas(size_t size, | ||
| Complex<float> const * lhs, | ||
| Complex<float> const * rhs); | ||
| Complex<double> dot_blas(size_t size, | ||
| Complex<double> const * lhs, | ||
| Complex<double> const * rhs); |
There was a problem hiding this comment.
Remove namespace blas; and add suffix _blas for function names.
9fcfa1b to
dd3dcaf
Compare
There was a problem hiding this comment.
Remaining points:
- Clarify: why vcpkg openblas does not have lapack.
-
*_veclib()should also be renamed to*_blaswhen they are on the same call stack of the wrappers already renamed.
@yungyuc, please review this pull request after CI works as usual.
It's OK. The passing CI jobs provided sufficient confidence.
| # vcpkg's openblas:x64-windows provides CBLAS but not LAPACK, | ||
| # since the vcpkg port explicitly builds openblas without LAPACK. | ||
| # References: | ||
| # - https://github.com/microsoft/vcpkg/blob/9733af874ef1d4c357ae0d7b4c0fc669e5a5e012/ports/openblas/portfile.cmake#L48-L56 | ||
| - name: install OpenBLAS | ||
| run: | | ||
| vcpkg install openblas:x64-windows | ||
| $openblas_bin_path = @( | ||
| "C:\vcpkg\installed\x64-windows\bin", | ||
| "C:\vcpkg\installed\x64-windows\debug\bin" | ||
| ) -join ";" | ||
| echo "$openblas_bin_path" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append | ||
|
|
There was a problem hiding this comment.
Please also clarify why vcpkg openblas turns off lapack.
modmesh uses both BLAS and LAPACK. The Linux installation has both. If Windows installation of openblas does not include both, please provide a way to supplement LAPACK.
We can discuss more.
| * Perform matrix multiplication using vendor BLAS when available. | ||
| */ | ||
| template <typename A, typename T> | ||
| A SimpleArrayMixinCalculators<A, T>::matmul_veclib(A const & other) const |
There was a problem hiding this comment.
SimpleArrayMixinClaculator::matmul_veclib() should also be renamed to matmul_blas like that in SimpleArrayMatmulHelper, because they are on the same call stack.
dd3dcaf to
30c3008
Compare
ThreeMonth03
left a comment
There was a problem hiding this comment.
- Clarify: why vcpkg openblas does not have lapack.
-
*_veclib()should also be renamed to*_blas()when they are on the same call stack of the wrappers already renamed.
I missed to modify the public api name, and I fix it now. @yungyuc , please see the discussion.
| # vcpkg's openblas:x64-windows provides CBLAS but not LAPACK, | ||
| # since the vcpkg port explicitly builds openblas without LAPACK. | ||
| # References: | ||
| # - https://github.com/microsoft/vcpkg/blob/9733af874ef1d4c357ae0d7b4c0fc669e5a5e012/ports/openblas/portfile.cmake#L48-L56 | ||
| - name: install OpenBLAS | ||
| run: | | ||
| vcpkg install openblas:x64-windows | ||
| $openblas_bin_path = @( | ||
| "C:\vcpkg\installed\x64-windows\bin", | ||
| "C:\vcpkg\installed\x64-windows\debug\bin" | ||
| ) -join ";" | ||
| echo "$openblas_bin_path" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append | ||
|
|
There was a problem hiding this comment.
vcpkg also affords lapack port for windows, so lapack could be installed by the following command:
vcpkg install lapack:x64-windows
vcpkg's developers mentioned that they prefer dividing LAPACK and OpenBLAS to two ports, so vcpkg openblas turns off lapack. However, they don't explain the motivation furthermore.
| @@ -434,8 +434,8 @@ class MODMESH_PYTHON_WRAPPER_VISIBILITY WrapSimpleArray | |||
| { self.idiv(scalar); }) | |||
| .def("imatmul", [](wrapped_type & self, wrapped_type const & other) | |||
| { self.imatmul(other); }) | |||
| .def("imatmul_veclib", [](wrapped_type & self, wrapped_type const & other) | |||
| { self.imatmul_veclib(other); }) | |||
| .def("imatmul_blas", [](wrapped_type & self, wrapped_type const & other) | |||
| { self.imatmul_blas(other); }) | |||
There was a problem hiding this comment.
Rename suffix *_veclib() to *_blas().
| # vcpkg's openblas:x64-windows provides CBLAS but not LAPACK, | ||
| # since the vcpkg port explicitly builds openblas without LAPACK. | ||
| # References: | ||
| # - https://github.com/microsoft/vcpkg/blob/9733af874ef1d4c357ae0d7b4c0fc669e5a5e012/ports/openblas/portfile.cmake#L48-L56 | ||
| - name: install OpenBLAS | ||
| run: | | ||
| vcpkg install openblas:x64-windows | ||
| $openblas_bin_path = @( | ||
| "C:\vcpkg\installed\x64-windows\bin", | ||
| "C:\vcpkg\installed\x64-windows\debug\bin" | ||
| ) -join ";" | ||
| echo "$openblas_bin_path" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append | ||
|
|
There was a problem hiding this comment.
In that case, we should install both blas and lapack so that the dependency is aligned across macos, linux, and windows.
| @@ -434,8 +434,8 @@ class MODMESH_PYTHON_WRAPPER_VISIBILITY WrapSimpleArray | |||
| { self.idiv(scalar); }) | |||
| .def("imatmul", [](wrapped_type & self, wrapped_type const & other) | |||
| { self.imatmul(other); }) | |||
| .def("imatmul_veclib", [](wrapped_type & self, wrapped_type const & other) | |||
| { self.imatmul_veclib(other); }) | |||
| .def("imatmul_blas", [](wrapped_type & self, wrapped_type const & other) | |||
| { self.imatmul_blas(other); }) | |||
445efd0 to
a43243c
Compare
a43243c to
a4a77cb
Compare
ThreeMonth03
left a comment
There was a problem hiding this comment.
- Install both blas and lapack on windows.
@yungyuc I install LAPACK on windows runner. Please review it.
| - name: install BLAS and LAPACK | ||
| run: | | ||
| vcpkg install openblas:x64-windows lapack:x64-windows | ||
| $vcpkg_bin_path = @( | ||
| "C:\vcpkg\installed\x64-windows\bin", | ||
| "C:\vcpkg\installed\x64-windows\debug\bin" | ||
| ) -join ";" | ||
| echo "$vcpkg_bin_path" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append | ||
|
|
||
| - name: dependency by pip |
There was a problem hiding this comment.
Install LAPACK and BLAS in windows runner.
| # Let CMake find vcpkg-provided OpenBLAS/LAPACK headers, import libraries, and | ||
| # package metadata during configure and link on Windows. | ||
| ifeq ($(OS),Windows_NT) | ||
| CMAKE_TOOLCHAIN_FILE ?= C:/vcpkg/scripts/buildsystems/vcpkg.cmake | ||
| CMAKE_ARGS += -DCMAKE_TOOLCHAIN_FILE=$(CMAKE_TOOLCHAIN_FILE) | ||
| CMAKE_ARGS += -DVCPKG_TARGET_TRIPLET=x64-windows | ||
| endif | ||
|
|
There was a problem hiding this comment.
To ensure makefile works as usual on Windows, additional flags are added in makefile.
| - name: install BLAS and LAPACK | ||
| run: | | ||
| vcpkg install openblas:x64-windows lapack:x64-windows | ||
| $vcpkg_bin_path = @( | ||
| "C:\vcpkg\installed\x64-windows\bin", | ||
| "C:\vcpkg\installed\x64-windows\debug\bin" | ||
| ) -join ";" | ||
| echo "$vcpkg_bin_path" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append | ||
|
|
||
| - name: dependency by pip |
| # Let CMake find vcpkg-provided OpenBLAS/LAPACK headers, import libraries, and | ||
| # package metadata during configure and link on Windows. | ||
| ifeq ($(OS),Windows_NT) | ||
| CMAKE_TOOLCHAIN_FILE ?= C:/vcpkg/scripts/buildsystems/vcpkg.cmake | ||
| CMAKE_ARGS += -DCMAKE_TOOLCHAIN_FILE=$(CMAKE_TOOLCHAIN_FILE) | ||
| CMAKE_ARGS += -DVCPKG_TARGET_TRIPLET=x64-windows | ||
| endif | ||
|
|
Installs openblas on the Windows platform, includes cblas when implementing
matmul_veclib(), and refactors testcases.Implement abstract layer
math/blas_compat.hpp. Move the existing abstraction layerlinalg/lapack_compat.hpptolinalg/math_compat.hpp.For issue #830