Skip to content
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

Draft: Attempt to specialize matrix_vector_product for parallel_policy #255

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

mhoemmen
Copy link
Contributor

@mhoemmen mhoemmen commented Jun 20, 2023

@crtrott @dalg24

@amklinv-nnl has been working on parallel specialization of stdblas algorithms. The two of us tried to specialize matrix_vector_product for std::execution::parallel_policy, but keep getting run-time recursion. We're guessing that the compiler thinks the generic ExecutionPolicy&& overload is "more specialized."

The test lives in tests/native/gemv_no_ambig.cpp. We're building using the following CMake options:

-DLINALG_ENABLE_TESTS=ON -DLINALG_ENABLE_EXAMPLES=ON -DLINALG_ENABLE_TBB=ON -DTBB_DIR=<PATH_TO_TBB_INSTALLATION>

Please don't merge this branch, btw; it will almost certainly conflict with other PRs.

amklinv-nnl and others added 14 commits June 12, 2023 16:38
Changes to the namespace (std::experimental to std) broke the existing
unit tests and examples. This commit should fix the problem.
Was getting a test error about a missing glibc because the tests were being run in a container. Removed all references to the container.
The include guards were failing for certain versions of GCC (such as
11.2.0) without TBB. This commit adds TBB as an optional dependency and
disables anything involving execution if the build would fail.
LINALG_HAS_TBB should have been LINALG_ENABLE_TBB. This change causes
the only test using executors to fail.
@mhoemmen
Copy link
Contributor Author

mhoemmen commented Jun 20, 2023

FYI, TBB can be built and installed from scratch using the following repo: https://github.com/oneapi-src/oneTBB . @amklinv-nnl tested with GCC 13 and it still requires TBB for parallel algorithms to compile, alas.

@mhoemmen
Copy link
Contributor Author

@amklinv-nnl Christian Trott explained offline how specializations for different policies work.

  1. Don't try to specialize *_is_avail.
  2. Only write specializations for an internal execution policy. Never write specializations for any of the Standard policies.
  3. If needed, overload execpolicy_mapper to map from std::execution::parallel_policy to a built-in policy (e.g., impl::some_happy_parallel_policy). Then, overload matrix_vector_product to take impl::some_happy_parallel_policy.

I don't have time to try this at the moment, but it sounds like this should fix the recursion issue (because users wouldn't ever pass in one of the internal execution policies).

@@ -102,8 +102,6 @@ jobs:

test-stdBLAS:
runs-on: ubuntu-latest
container:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change has already been merged into the main branch.

@@ -121,6 +121,14 @@ option(LINALG_ENABLE_BLAS
"Assume that we are linking with a BLAS library."
${BLAS_FOUND})

find_package(TBB)
Copy link
Contributor Author

@mhoemmen mhoemmen Jun 27, 2023

Choose a reason for hiding this comment

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

PR #256 incorporates the changes in this file.

@@ -28,6 +28,8 @@ Other compilers, including MSVC 2019, have been tested in the past.
- If you want to build examples, set LINALG_ENABLE_EXAMPLES=ON
- If you have a BLAS installation, set LINALG_ENABLE_BLAS=ON.
BLAS support is currently experimental.
- If you have a TBB installation, set LINALG_ENABLE_TBB=ON.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR #256 incorporates the changes in this file.

using std::experimental::mdspan;
using std::experimental::extents;
using std::experimental::dynamic_extent;
using MDSPAN_IMPL_STANDARD_NAMESPACE::mdspan;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

An equivalent fix is already in the main branch.

@@ -30,7 +22,7 @@ int main(int argc, char* argv[]) {

// Call linalg::scale x = 2.0*x;
std::experimental::linalg::scale(2.0, x);
#ifdef MDSPAN_EXAMPLES_USE_EXECUTION_POLICIES
#ifdef LINALG_HAS_EXECUTION
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See PR #256.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants