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

[libc++][PSTL] Dispatching to default algorithms is broken in some cases (at least std::any_of) #70718

Open
ldionne opened this issue Oct 30, 2023 · 2 comments · May be fixed by #88131
Open
Assignees
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. pstl Issues related to the C++17 Parallel STL

Comments

@ldionne
Copy link
Member

ldionne commented Oct 30, 2023

The default implementation of std::none_of in the PSTL is (here):

      [&](_ForwardIterator __g_first, _ForwardIterator __g_last, _Pred __g_pred) -> optional<bool> {
        auto __res = std::__any_of(__policy, __g_first, __g_last, __g_pred);
        if (!__res)
          return nullopt;
        return !*__res;
      },

Notice how this calls std::__any_of with lvalues __g_first, __g_last, __g_pred. Now, __any_of is implemented like (here):

template <class _ExecutionPolicy, class _ForwardIterator, class _Predicate, ...>
optional<bool>
__any_of(_ExecutionPolicy&& __policy, _ForwardIterator&& __first, _ForwardIterator&& __last, _Predicate&& __pred) {
  return std::__pstl_frontend_dispatch(
      _LIBCPP_PSTL_CUSTOMIZATION_POINT(__pstl_any_of, _RawPolicy),
      [&](_ForwardIterator __g_first, _ForwardIterator __g_last, _Predicate __g_pred) -> optional<bool> {
        auto __res = std::__find_if(__policy, __g_first, __g_last, __g_pred);
        if (!__res)
          return nullopt;
        return *__res != __g_last;
      },
      std::move(__first),
      std::move(__last),
      std::move(__pred));
}

Since __any_of is called with lvalues and we're taking universal references, when none_of calls __any_of we get the following deduction: _ForwardIterator&& == _Iter& && for some ref-unqualified type _Iter. As a result, the lambda declaration inside __any_of basically has signature [&](_Iter&, _Iter&, _Predicate) and that doesn't work since we're trying to pass it a rvalue (std::move(__first) & al).

This issue happens whenever the default implementation of std::any_of is used. It can be seen by removing:

libcxx/include/__algorithm/pstl_backends/cpu_backends/any_of.h

Normally, things should work even in that case because any_of is not a mandatory algorithm for general backends.

@ldionne ldionne added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 30, 2023
@AntonRydahl
Copy link
Contributor

Was the solution that we talked about just to change std::move to std::forward for each implementation that uses _LIBCPP_PSTL_CUSTOMIZATION_POINT , or do you think more changes are needed?

@ldionne
Copy link
Member Author

ldionne commented Oct 31, 2023

I am not sure forward is what we want -- I wanted to talk to @philnik777 about it in our next 1:1.

@ldionne ldionne added the pstl Issues related to the C++17 Parallel STL label Apr 8, 2024
@ldionne ldionne self-assigned this Apr 8, 2024
ldionne added a commit to ldionne/llvm-project that referenced this issue Apr 9, 2024
The experimental PSTL's current dispatching mechanism was designed with
flexibility in mind. However, while reviewing the in-progress OpenMP
backend, I realized that the dispatching mechanism based on ADL and
default definitions in the frontend had several downsides. To name a
few:

1. The dispatching of an algorithm to the back-end and its default
   implementation is bundled together via `_LIBCPP_PSTL_CUSTOMIZATION_POINT`.
   This makes the dispatching really confusing and leads to annoyances
   such as variable shadowing and weird lambda captures in the front-end.
2. The distinction between back-end functions and front-end algorithms
   is not as clear as it could be, which led us to call one where we meant
   the other in a few cases. This is bad due to the exception requirements
   of the PSTL.
3. There are two levels of back-end dispatching in the PSTL, which treat
   CPU backends as a special case. This was confusing and not as flexible
   as we'd like. For example, there was no straightforward way to dispatch
   all uses of `unseq` to a specific back-end from the OpenMP backend,
   or for CPU backends to fall back on each other.

This patch rewrites the backend dispatching mechanism to solve these
problems, but doesn't touch any of the actual implementation of
algorithms. Specifically, this rewrite has the following characteristics:

- All back-ends are full top-level backends defining all the basis operations
  required by the PSTL. This is made realistic for CPU backends by providing
  the CPU-based basis operations as simple helpers that can easily be reused
  when defining the PSTL basis operations.

- The default definitions for algorithms are separated from their dispatching
  logic and grouped in families instead, based on the basis operation they
  require for their default implementation.

- The front-end is thus simplified a whole lot and made very consistent
  for all algorithms, which makes it easier to audit the front-end for
  things like exception-correctness.

Fixes llvm#70718
@ldionne ldionne linked a pull request Apr 9, 2024 that will close this issue
ldionne added a commit to ldionne/llvm-project that referenced this issue Apr 12, 2024
The experimental PSTL's current dispatching mechanism was designed with
flexibility in mind. However, while reviewing the in-progress OpenMP
backend, I realized that the dispatching mechanism based on ADL and
default definitions in the frontend had several downsides. To name a
few:

1. The dispatching of an algorithm to the back-end and its default
   implementation is bundled together via `_LIBCPP_PSTL_CUSTOMIZATION_POINT`.
   This makes the dispatching really confusing and leads to annoyances
   such as variable shadowing and weird lambda captures in the front-end.
2. The distinction between back-end functions and front-end algorithms
   is not as clear as it could be, which led us to call one where we meant
   the other in a few cases. This is bad due to the exception requirements
   of the PSTL.
3. There are two levels of back-end dispatching in the PSTL, which treat
   CPU backends as a special case. This was confusing and not as flexible
   as we'd like. For example, there was no straightforward way to dispatch
   all uses of `unseq` to a specific back-end from the OpenMP backend,
   or for CPU backends to fall back on each other.

This patch rewrites the backend dispatching mechanism to solve these
problems, but doesn't touch any of the actual implementation of
algorithms. Specifically, this rewrite has the following characteristics:

- All back-ends are full top-level backends defining all the basis operations
  required by the PSTL. This is made realistic for CPU backends by providing
  the CPU-based basis operations as simple helpers that can easily be reused
  when defining the PSTL basis operations.

- The default definitions for algorithms are separated from their dispatching
  logic and grouped in families instead, based on the basis operation they
  require for their default implementation.

- The front-end is thus simplified a whole lot and made very consistent
  for all algorithms, which makes it easier to audit the front-end for
  things like exception-correctness.

Fixes llvm#70718
ldionne added a commit to ldionne/llvm-project that referenced this issue May 27, 2024
The experimental PSTL's current dispatching mechanism was designed with
flexibility in mind. However, while reviewing the in-progress OpenMP
backend, I realized that the dispatching mechanism based on ADL and
default definitions in the frontend had several downsides. To name a
few:

1. The dispatching of an algorithm to the back-end and its default
  implementation is bundled together via `_LIBCPP_PSTL_CUSTOMIZATION_POINT`.
  This makes the dispatching really confusing and leads to annoyances
  such as variable shadowing and weird lambda captures in the front-end.
2. The distinction between back-end functions and front-end algorithms
  is not as clear as it could be, which led us to call one where we meant
  the other in a few cases. This is bad due to the exception requirements
  of the PSTL.
3. There are two levels of back-end dispatching in the PSTL, which treat
  CPU backends as a special case. This was confusing and not as flexible
  as we'd like. For example, there was no straightforward way to dispatch
  all uses of `unseq` to a specific back-end from the OpenMP backend,
  or for CPU backends to fall back on each other.

This patch rewrites the backend dispatching mechanism to solve these
problems, but doesn't touch any of the actual implementation of
algorithms. Specifically, this rewrite has the following characteristics:

- All back-ends are full top-level backends defining all the basis operations
required by the PSTL. This is made realistic for CPU backends by providing
the CPU-based basis operations as simple helpers that can easily be reused
when defining the PSTL basis operations.

- The default definitions for algorithms are separated from their dispatching
logic and grouped in families instead, based on the basis operation they
require for their default implementation.

- The front-end is thus simplified a whole lot and made very consistent
for all algorithms, which makes it easier to audit the front-end for
things like exception-correctness.

Fixes llvm#70718
ldionne added a commit to ldionne/llvm-project that referenced this issue May 28, 2024
The experimental PSTL's current dispatching mechanism was designed with
flexibility in mind. However, while reviewing the in-progress OpenMP
backend, I realized that the dispatching mechanism based on ADL and
default definitions in the frontend had several downsides. To name a
few:

1. The dispatching of an algorithm to the back-end and its default
  implementation is bundled together via `_LIBCPP_PSTL_CUSTOMIZATION_POINT`.
  This makes the dispatching really confusing and leads to annoyances
  such as variable shadowing and weird lambda captures in the front-end.
2. The distinction between back-end functions and front-end algorithms
  is not as clear as it could be, which led us to call one where we meant
  the other in a few cases. This is bad due to the exception requirements
  of the PSTL: calling a front-end algorithm inside the implementation of
  a back-end is incorrect for exception-safety.
3. There are two levels of back-end dispatching in the PSTL, which treat
  CPU backends as a special case. This was confusing and not as flexible
  as we'd like. For example, there was no straightforward way to dispatch
  all uses of `unseq` to a specific back-end from the OpenMP backend,
  or for CPU backends to fall back on each other.

This patch rewrites the backend dispatching mechanism to solve these
problems, but doesn't touch any of the actual implementation of
algorithms. Specifically, this rewrite has the following characteristics:

- All back-ends are full top-level backends defining all the basis operations
required by the PSTL. This is made realistic for CPU backends by providing
the CPU-based basis operations as simple helpers that can easily be reused
when defining the PSTL basis operations.

- The default definitions for algorithms are separated from their dispatching
logic and grouped in families instead, based on the basis operation they
require for their default implementation.

- The front-end is thus simplified a whole lot and made very consistent
for all algorithms, which makes it easier to audit the front-end for
things like exception-correctness, appropriate forwarding, etc.

Fixes llvm#70718
ldionne added a commit to ldionne/llvm-project that referenced this issue May 28, 2024
The experimental PSTL's current dispatching mechanism was designed with
flexibility in mind. However, while reviewing the in-progress OpenMP
backend, I realized that the dispatching mechanism based on ADL and
default definitions in the frontend had several downsides. To name a
few:

1. The dispatching of an algorithm to the back-end and its default
  implementation is bundled together via `_LIBCPP_PSTL_CUSTOMIZATION_POINT`.
  This makes the dispatching really confusing and leads to annoyances
  such as variable shadowing and weird lambda captures in the front-end.
2. The distinction between back-end functions and front-end algorithms
  is not as clear as it could be, which led us to call one where we meant
  the other in a few cases. This is bad due to the exception requirements
  of the PSTL: calling a front-end algorithm inside the implementation of
  a back-end is incorrect for exception-safety.
3. There are two levels of back-end dispatching in the PSTL, which treat
  CPU backends as a special case. This was confusing and not as flexible
  as we'd like. For example, there was no straightforward way to dispatch
  all uses of `unseq` to a specific back-end from the OpenMP backend,
  or for CPU backends to fall back on each other.

This patch rewrites the backend dispatching mechanism to solve these
problems, but doesn't touch any of the actual implementation of
algorithms. Specifically, this rewrite has the following characteristics:

- All back-ends are full top-level backends defining all the basis operations
required by the PSTL. This is made realistic for CPU backends by providing
the CPU-based basis operations as simple helpers that can easily be reused
when defining the PSTL basis operations.

- The default definitions for algorithms are separated from their dispatching
logic and grouped in families instead, based on the basis operation they
require for their default implementation.

- The front-end is thus simplified a whole lot and made very consistent
for all algorithms, which makes it easier to audit the front-end for
things like exception-correctness, appropriate forwarding, etc.

Fixes llvm#70718
ldionne added a commit to ldionne/llvm-project that referenced this issue May 28, 2024
The experimental PSTL's current dispatching mechanism was designed with
flexibility in mind. However, while reviewing the in-progress OpenMP
backend, I realized that the dispatching mechanism based on ADL and
default definitions in the frontend had several downsides. To name a
few:

1. The dispatching of an algorithm to the back-end and its default
  implementation is bundled together via `_LIBCPP_PSTL_CUSTOMIZATION_POINT`.
  This makes the dispatching really confusing and leads to annoyances
  such as variable shadowing and weird lambda captures in the front-end.
2. The distinction between back-end functions and front-end algorithms
  is not as clear as it could be, which led us to call one where we meant
  the other in a few cases. This is bad due to the exception requirements
  of the PSTL: calling a front-end algorithm inside the implementation of
  a back-end is incorrect for exception-safety.
3. There are two levels of back-end dispatching in the PSTL, which treat
  CPU backends as a special case. This was confusing and not as flexible
  as we'd like. For example, there was no straightforward way to dispatch
  all uses of `unseq` to a specific back-end from the OpenMP backend,
  or for CPU backends to fall back on each other.

This patch rewrites the backend dispatching mechanism to solve these
problems, but doesn't touch any of the actual implementation of
algorithms. Specifically, this rewrite has the following characteristics:

- All back-ends are full top-level backends defining all the basis operations
required by the PSTL. This is made realistic for CPU backends by providing
the CPU-based basis operations as simple helpers that can easily be reused
when defining the PSTL basis operations.

- The default definitions for algorithms are separated from their dispatching
logic and grouped in families instead, based on the basis operation they
require for their default implementation.

- The front-end is thus simplified a whole lot and made very consistent
for all algorithms, which makes it easier to audit the front-end for
things like exception-correctness, appropriate forwarding, etc.

Fixes llvm#70718
ldionne added a commit to ldionne/llvm-project that referenced this issue May 28, 2024
The experimental PSTL's current dispatching mechanism was designed with
flexibility in mind. However, while reviewing the in-progress OpenMP
backend, I realized that the dispatching mechanism based on ADL and
default definitions in the frontend had several downsides. To name a
few:

1. The dispatching of an algorithm to the back-end and its default
  implementation is bundled together via `_LIBCPP_PSTL_CUSTOMIZATION_POINT`.
  This makes the dispatching really confusing and leads to annoyances
  such as variable shadowing and weird lambda captures in the front-end.
2. The distinction between back-end functions and front-end algorithms
  is not as clear as it could be, which led us to call one where we meant
  the other in a few cases. This is bad due to the exception requirements
  of the PSTL: calling a front-end algorithm inside the implementation of
  a back-end is incorrect for exception-safety.
3. There are two levels of back-end dispatching in the PSTL, which treat
  CPU backends as a special case. This was confusing and not as flexible
  as we'd like. For example, there was no straightforward way to dispatch
  all uses of `unseq` to a specific back-end from the OpenMP backend,
  or for CPU backends to fall back on each other.

This patch rewrites the backend dispatching mechanism to solve these
problems, but doesn't touch any of the actual implementation of
algorithms. Specifically, this rewrite has the following characteristics:

- All back-ends are full top-level backends defining all the basis operations
required by the PSTL. This is made realistic for CPU backends by providing
the CPU-based basis operations as simple helpers that can easily be reused
when defining the PSTL basis operations.

- The default definitions for algorithms are separated from their dispatching
logic and grouped in families instead, based on the basis operation they
require for their default implementation.

- The front-end is thus simplified a whole lot and made very consistent
for all algorithms, which makes it easier to audit the front-end for
things like exception-correctness, appropriate forwarding, etc.

Fixes llvm#70718
ldionne added a commit to ldionne/llvm-project that referenced this issue May 28, 2024
The experimental PSTL's current dispatching mechanism was designed with
flexibility in mind. However, while reviewing the in-progress OpenMP
backend, I realized that the dispatching mechanism based on ADL and
default definitions in the frontend had several downsides. To name a
few:

1. The dispatching of an algorithm to the back-end and its default
  implementation is bundled together via `_LIBCPP_PSTL_CUSTOMIZATION_POINT`.
  This makes the dispatching really confusing and leads to annoyances
  such as variable shadowing and weird lambda captures in the front-end.
2. The distinction between back-end functions and front-end algorithms
  is not as clear as it could be, which led us to call one where we meant
  the other in a few cases. This is bad due to the exception requirements
  of the PSTL: calling a front-end algorithm inside the implementation of
  a back-end is incorrect for exception-safety.
3. There are two levels of back-end dispatching in the PSTL, which treat
  CPU backends as a special case. This was confusing and not as flexible
  as we'd like. For example, there was no straightforward way to dispatch
  all uses of `unseq` to a specific back-end from the OpenMP backend,
  or for CPU backends to fall back on each other.

This patch rewrites the backend dispatching mechanism to solve these
problems, but doesn't touch any of the actual implementation of
algorithms. Specifically, this rewrite has the following characteristics:

- All back-ends are full top-level backends defining all the basis operations
required by the PSTL. This is made realistic for CPU backends by providing
the CPU-based basis operations as simple helpers that can easily be reused
when defining the PSTL basis operations.

- The default definitions for algorithms are separated from their dispatching
logic and grouped in families instead, based on the basis operation they
require for their default implementation.

- The front-end is thus simplified a whole lot and made very consistent
for all algorithms, which makes it easier to audit the front-end for
things like exception-correctness, appropriate forwarding, etc.

Fixes llvm#70718
ldionne added a commit to ldionne/llvm-project that referenced this issue May 28, 2024
The experimental PSTL's current dispatching mechanism was designed with
flexibility in mind. However, while reviewing the in-progress OpenMP
backend, I realized that the dispatching mechanism based on ADL and
default definitions in the frontend had several downsides. To name a
few:

1. The dispatching of an algorithm to the back-end and its default
  implementation is bundled together via `_LIBCPP_PSTL_CUSTOMIZATION_POINT`.
  This makes the dispatching really confusing and leads to annoyances
  such as variable shadowing and weird lambda captures in the front-end.
2. The distinction between back-end functions and front-end algorithms
  is not as clear as it could be, which led us to call one where we meant
  the other in a few cases. This is bad due to the exception requirements
  of the PSTL: calling a front-end algorithm inside the implementation of
  a back-end is incorrect for exception-safety.
3. There are two levels of back-end dispatching in the PSTL, which treat
  CPU backends as a special case. This was confusing and not as flexible
  as we'd like. For example, there was no straightforward way to dispatch
  all uses of `unseq` to a specific back-end from the OpenMP backend,
  or for CPU backends to fall back on each other.

This patch rewrites the backend dispatching mechanism to solve these
problems, but doesn't touch any of the actual implementation of
algorithms. Specifically, this rewrite has the following characteristics:

- All back-ends are full top-level backends defining all the basis operations
required by the PSTL. This is made realistic for CPU backends by providing
the CPU-based basis operations as simple helpers that can easily be reused
when defining the PSTL basis operations.

- The default definitions for algorithms are separated from their dispatching
logic and grouped in families instead, based on the basis operation they
require for their default implementation.

- The front-end is thus simplified a whole lot and made very consistent
for all algorithms, which makes it easier to audit the front-end for
things like exception-correctness, appropriate forwarding, etc.

Fixes llvm#70718
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. pstl Issues related to the C++17 Parallel STL
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants