Page MenuHomePhabricator

[libc++][pstl] Implement tag dispatching
Needs ReviewPublic

Authored by rarutyun on Jun 17 2021, 3:53 PM.

Details

Summary

This is the initial revision of the patch.

Let's agree on the approach so we can iteratively apply necessary changes to the rest of the code.

The idea is to update the current review with every required changes after ratifying the approach.

Please note that many traits are supposed to be removed after necessary changes would be applied for the whole code base.

Diff Detail

Unit TestsFailed

TimeTest
70 msx64 debian > LLVM.Examples/OrcV2Examples::lljit-with-remote-debugging.test
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/LLJITWithRemoteDebugging /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Examples/OrcV2Examples/Inputs/argc_sub1_elf.ll | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck --check-prefix=CHECK1 /var/lib/buildkite-agent/builds/llvm-project/llvm/test/Examples/OrcV2Examples/lljit-with-remote-debugging.test

Event Timeline

rarutyun requested review of this revision.Jun 17 2021, 3:53 PM
rarutyun created this revision.
Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 17 2021, 3:53 PM
rarutyun added inline comments.Jun 17 2021, 3:57 PM
pstl/include/pstl/internal/parallel_backend_serial.h
57

Note that this overload supposed to go away when all the changes are made. Currently there is a code that depends on that.

Quuxplusone added inline comments.
pstl/include/pstl/internal/algorithm_fwd.h
19

I believe you need to add pstl/include to
https://github.com/google/llvm-premerge-checks/blob/main/scripts/clang-tidy.ignore
(Although why this file is in a Google repo instead of the llvm-project repo, I don't know!)

269

Throughout: On function declarations, I think it's unhelpful to have parameter names like _Tag __tag or (below) _ExecutionPolicy&& __exec. I would remove those parameter names. The names __first, __last, __pred are arguably more helpful; but for consistency I'd probably remove them too. (Or rather, don't add them.)

pstl/include/pstl/internal/execution_impl.h
30–32

decltype(T{}) is just T.

38–41

Could this be:

template <typename... _IteratorTypes>
using __are_random_access_iterators = decltype(__internal::__is_iterator_of<
    std::random_access_iterator_tag, _IteratorTypes...>(0));

(Notice the ADL-proofing on __is_iterator_of.)

IIUC, the intent is that __are_random_access_iterators<int*, char*> is true_type but __are_random_access_iterators<int*, std::list<int>::iterator> is false_type, right?

Or if C++17 is permitted, then

template <typename... _IteratorTypes>
using __are_random_access_iterators = std::bool_constant<
    (std::is_base_of_v<std::random_access_iterator_tag,
                       typename std::iterator_traits<_IteratorTypes>::iterator_category> && ...)
>;
120

Rather than split the declaration across #ifs, it would be easier to read (for both humans and computers) if this were just

#if defined(_PSTL_PAR_BACKEND_TBB)
 using __par_backend_tag = __tbb_backend;
#elif _PSTAL_PAR_BACKEND_SERIAL
 using __par_backend_tag = __serial_backend;
#else
 #error "A parallel backend must be specified"
#endif

(also note the probably-accidental inconsistent use of defined versus non-zero, and the bogus semicolon at the end of line 126)

129–142

IIUC, __is_vector is always true_type or false_type?
Would it be simpler to define...

template<bool _IsParallel, bool _IsVector>
struct __bikeshed_tag {
    using __is_vector = std::bool_constant<_IsVector>;
    using __is_parallel = std::bool_constant<_IsParallel>;
    using __backend_tag = std::conditional_t<_IsParallel, __par_backend_tag, void>;
};

template<bool _IsVector>
using __parallel_tag = __bikeshed_tag<true, _IsVector>;

template<bool _IsVector>
using __serial_tag = __bikeshed_tag<false, _IsVector>;

And then if you ever found any need to pattern-match on __bikeshed_tag<X, true>, it wouldn't be so awkward.

zoecarver added inline comments.Jun 28 2021, 8:44 AM
pstl/include/pstl/internal/algorithm_fwd.h
19

Additionally, it looks like these files may be nested one level too deep. I think this should be pstl/include/algorithm_fwd.h not pstl/include/pstl/include/algorithm_fwd.h.

Quuxplusone added inline comments.Jun 28 2021, 8:55 AM
pstl/include/pstl/internal/algorithm_fwd.h
19

@zoecarver: No, I bet the nesting is right. Notice that it's

    pstl/include/pstl/internal/algorithm_fwd.h
not                   ^^^^^^^^
    pstl/include/pstl/include/algorithm_fwd.h

The pattern of "foo/include/foo/" ends up being pretty common in codebases I've seen, because there'll be a "foo/src" and a "foo/include", and then in order to make #include <foo/bar.h> to work after -I foo/include, you need to put your actual header file in "foo/include/foo/bar.h".

rodgert accepted this revision.Jul 2 2021, 10:04 AM
rodgert added inline comments.
pstl/include/pstl/internal/execution_impl.h
38–41

At least as it relates to libstdc++'s consumption of pstl, C++17 is the baseline expectation.