This is an archive of the discontinued LLVM Phabricator instance.

[pstl] Fix compile errors when PARALLEL_POLICIES is disabled
AbandonedPublic

Authored by jerryct on Dec 28 2018, 1:47 PM.

Details

Summary

Wrap all functions dispatched by a parallel policy tag in a conditional directive.

Diff Detail

Event Timeline

jerryct created this revision.Dec 28 2018, 1:47 PM
MikeDvorskiy added inline comments.
include/pstl/internal/algorithm_impl.h
62–75

What if I suggest excluding the overloaded parallel pattern (here and everywhere) by SFINAE approach, like this:

pstl::internal::enable_if_execution_policy<_ExecutionPolicy, bool>
pattern_any_of(_ExecutionPolicy&&
exec, _ForwardIterator first, _ForwardIterator last, _Pred __pred,.....

The macro __PSTL_USE_PAR_POLICIES controls parallel policies definitions and it should be enough.

akukanov added inline comments.
include/pstl/internal/algorithm_impl.h
62–75

Mikhail, I think that use of enable_if_execution_policy is inconsistent if only applied to "parallel" overloads, and less clear comparing to the macro.

If the SFINAE way is preferred, we should make it differently, e.g. introduce enable_if_parallel/enable_if_not_parallel and use those instead of is_parallel function parameter which becomes redundant.

ldionne accepted this revision.Jan 10 2019, 3:27 AM

LGTM. I can witness the problem locally with the CMake build and the fix resolves the issue. Thanks!

Committed as r350813.

This revision is now accepted and ready to land.Jan 10 2019, 3:27 AM
ldionne closed this revision.Jan 10 2019, 3:27 AM
rodgert marked an inline comment as done.Jan 15 2019, 1:00 PM
rodgert added a subscriber: rodgert.

See inline comment

include/pstl/internal/algorithm_impl.h
181

Shouldn't this definition be protected by #ifdef __PSTL_USE_PAR_POLICIES?

Sure, it will fail to compile because it calls something that is __PSTL_USE_PAR_POLICIES but what was it ldionne said about being consistent recently?

rodgert reopened this revision.Jan 15 2019, 1:01 PM
This revision is now accepted and ready to land.Jan 15 2019, 1:01 PM
rodgert requested changes to this revision.Jan 17 2019, 10:42 AM

See inline notes on __PSTL_USE_PAR_POLICIES #ifdefs

include/pstl/internal/algorithm_impl.h
2432–2439

Shouldn't this definition also be protected by #ifdef __PSTL_USE_PAR_POLICIES?

This revision now requires changes to proceed.Jan 17 2019, 10:42 AM

Added a reply note to Mihail and Alexey's inline commentary on whether __PSTL_USE_PAR_POLICIES is how we really want to be controlling this.

ldionne added inline comments.Jan 22 2019, 7:44 AM
include/pstl/internal/algorithm_impl.h
181

I don't understand your comment. What is inconsistent about pattern_walk1_n not being guarded by #ifdef __PSTL_USE_PAR_POLICIES? Sorry, just trying to understand.

FWIW, I think that removing code based on __PSTL_USE_PAR_POLICIES is a misfeature and we should find a more gracious way of handling the lack of parallel policies, for example by having a backend that does everything sequentially if that makes sense. I accepted this revision because it fixed a compilation error under a configuration that currently seems to be supported.

rodgert added inline comments.Jan 22 2019, 10:18 AM
include/pstl/internal/algorithm_impl.h
181

Sorry, I applied the inline comment to the wrong variant of pattern_walk1_n, it's the next one down, where /*is_parallel=*/std::true_type

I also agree that removing code based on __PSTL_USE_PAR_POLICIES is a misfeature. I want to propose some alternatives based on work I've started on an OpenMP based backend and discussions that came out of SG1 in San Diego about likely future directions with this part of the standard library, but I need to finish this merge and get a patch into libstdc++ before I can return to that work.

In doing the reconciliation with the libstdc++ version of this code with what was submitted upstream (which unfortunately differs far more than what I thought we had agreed for the initial commit), I came across these inconsistencies and am just reporting them (they are fixed in my local branch, and will be part of that submission when it's ready).

2506–2515

This should also be protected by #ifdef __PSTL_USE_PAR_POLICIES

jerryct added inline comments.Feb 2 2019, 1:12 AM
include/pstl/internal/algorithm_impl.h
181

I addressed you comment in this new change: https://reviews.llvm.org/D57638

jerryct reclaimed this revision.Mar 2 2019, 5:22 AM
This revision now requires changes to proceed.Mar 2 2019, 5:22 AM
jerryct abandoned this revision.Mar 2 2019, 5:22 AM