[PATCH] Integration of parallel standard algorithms for c++17

[PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

[-- Attachment #1: compressed patch --]
[-- Type: application/x-bzip2, Size: 64886 bytes --]

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

This patch has been held up by licensing issues, specifically, awaiting
upstream's plans to relicense the code from Apache 2.0 to the libc++ license -

https://llvm.org/docs/DeveloperPolicy.html#license

We expect the relicensing to conclude within the next few weeks. At that
time I will be resubmiting this patch with the new license.

-- Tom

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

[-- Attachment #1: Type: text/plain, Size: 7175 bytes --]

Update C++17 parallel algorithms to LLVM/MIT licensed upstream sources

	* include/Makefile.am:update from upstream
		remove ${pstl_srcdir}/iterators.h
		add ${pstl_srcdir}/memory_impl.h

	* include/bits/c++config:update from upstream
	* include/pstl/algorithm_fwd.h:update from upstreamgg
	* include/pstl/algorithm_impl.h:update from upstream
	* include/pstl/execution_defs.h:update from upstream
	* include/pstl/execution_impl.h:update from upstream
	* include/pstl/glue_algorithm_defs.h:update from upstream
	* include/pstl/glue_algorithm_impl.h:update from upstream
	* include/pstl/glue_execution_defs.h:update from upstream
	* include/pstl/glue_memory_defs.h:update from upstream
	* include/pstl/glue_memory_impl.h:update from upstream
	* include/pstl/glue_numeric_defs.h:update from upstream
	* include/pstl/glue_numeric_impl.h:update from upstream
	* include/pstl/memory_impl.h:update from upstream
	* include/pstl/numeric_fwd.h:update from upstream
	* include/pstl/numeric_impl.h:update from upstream
	* include/pstl/parallel_backend.h:update from upstream
	* include/pstl/parallel_backend_tbb.h:update from upstream
	* include/pstl/parallel_backend_tbb_fwd.h:update from upstream
	* include/pstl/parallel_backend_utils.h:update from upstream
	* include/pstl/parallel_impl.h:update from upstream
	* include/pstl/pstl_config.h:update from upstream
	* include/pstl/unseq_backend_simd.h:update from upstream
	* include/pstl/utils.h:update from upstream
	* include/std/algorithm:update from upstream
	* include/std/execution:update from upstream
	* include/std/memory:update from upstream
	* include/std/numeric:update from upstream
	* testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_construct.cc:update from upstream
	* testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_copy_move.cc:update from upstream
	* testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_fill_destroy.cc:update from upstream
	* testsuite/20_util/specialized_algorithms/pstl/uninitialized_construct.cc:update from upstream
	* testsuite/20_util/specialized_algorithms/pstl/uninitialized_copy_move.cc:update from upstream
	* testsuite/20_util/specialized_algorithms/pstl/uninitialized_fill_destroy.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.heap.operations/is_heap.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.merge/inplace_merge.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.merge/merge.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/alg.copy/copy_if.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/alg.partitions/is_partitioned.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/alg.partitions/partition.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/alg.partitions/partition_copy.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/alg.reverse/reverse.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/alg.reverse/reverse_copy.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/copy.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/copy_move.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/fill.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/generate.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/move.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/remove.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/remove_copy.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/replace.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/replace_copy.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/rotate.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/rotate_copy.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/swap_ranges.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/transform_binary.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/transform_unary.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/unique.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.modifying.operations/unique_copy_equal.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/adjacent_find.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/all_of.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/any_of.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/count.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/equal.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/find.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/find_end.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/find_first_of.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/find_if.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/for_each.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/mismatch.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/none_of.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/nth_element.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/search.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.nonmodifying/search_n.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.sorting/alg.heap.operations/is_heap.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.sorting/alg.lex.comparison/lexicographical_compare.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.sorting/alg.min.max/minmax_element.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.sorting/alg.set.operations/includes.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.sorting/alg.set.operations/set.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.sorting/is_sorted.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.sorting/partial_sort.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.sorting/partial_sort_copy.cc:update from upstream
	* testsuite/25_algorithms/pstl/alg.sorting/sort.cc:update from upstream
	* testsuite/26_numerics/pstl/numeric.ops/adjacent_difference.cc:update from upstream
	* testsuite/26_numerics/pstl/numeric.ops/reduce.cc:update from upstream
	* testsuite/26_numerics/pstl/numeric.ops/scan.cc:update from upstream
	* testsuite/26_numerics/pstl/numeric.ops/transform_reduce.cc:update from upstream
	* testsuite/26_numerics/pstl/numeric.ops/transform_scan.cc:update from upstream
	* testsuite/util/pstl/parallel_utils.h:update from upstream
	* testsuite/util/pstl/pstl_test_config.h:update from upstream
gg

[-- Attachment #2: 20190131-pstl-integration.patch.bz2 --]
[-- Type: application/x-bzip, Size: 73782 bytes --]

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Jakub Jelinek]

On Thu, Jan 31, 2019 at 09:08:52PM -0800, Thomas Rodgers wrote:
> Update C++17 parallel algorithms to LLVM/MIT licensed upstream sources

Just ChangeLog formatting nits below:
> 
> 	* include/Makefile.am:update from upstream
> 		remove ${pstl_srcdir}/iterators.h
> 		add ${pstl_srcdir}/memory_impl.h
> 

Generally, the format is TAB * SPACE filename optional function name etc. in
parens COLON SPACE What has changed PERIOD, where what has changed starts
with a capital letter.  Your entries miss the capital letters, spaces and
periods.  include/Makefile.am has GCC as its own upstream, so Update from
upstream. makes no sense.  You've changed something in there, so usually one
writes what has changed:
	* include/Makefile.am (std_headers): Add ${std_srcdir}/execution.
	(pstl_srcdir, pstl_builddir, pstl_headers): New variables.
...
etc.
	* include/Makefile.in: Regenerated.

> 	* include/bits/c++config:update from upstream

Similarly, c++config doesn't have some other upstream, so one needs to say
what has changed.  Try contrib/mklog to get a template and just tweak it
when it doesn't handle something right (it is written mostly for C/C++
sources, not Makefiles etc.).

> 	* include/pstl/algorithm_fwd.h:update from upstreamgg

etc., these are newly added, so one usually writes ...: New file.

	Jakub

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

I will take another stab at un-nit-ing the changlog.

Jakub Jelinek writes:

> On Thu, Jan 31, 2019 at 09:08:52PM -0800, Thomas Rodgers wrote:
>> Update C++17 parallel algorithms to LLVM/MIT licensed upstream sources
>
> Just ChangeLog formatting nits below:
>>
>> 	* include/Makefile.am:update from upstream
>> 		remove ${pstl_srcdir}/iterators.h
>> 		add ${pstl_srcdir}/memory_impl.h
>>
>
> Generally, the format is TAB * SPACE filename optional function name etc. in
> parens COLON SPACE What has changed PERIOD, where what has changed starts
> with a capital letter.  Your entries miss the capital letters, spaces and
> periods.  include/Makefile.am has GCC as its own upstream, so Update from
> upstream. makes no sense.  You've changed something in there, so usually one
> writes what has changed:
> 	* include/Makefile.am (std_headers): Add ${std_srcdir}/execution.
> 	(pstl_srcdir, pstl_builddir, pstl_headers): New variables.
> ...
> etc.
> 	* include/Makefile.in: Regenerated.
>
>> 	* include/bits/c++config:update from upstream
>
> Similarly, c++config doesn't have some other upstream, so one needs to say
> what has changed.  Try contrib/mklog to get a template and just tweak it
> when it doesn't handle something right (it is written mostly for C/C++
> sources, not Makefiles etc.).
>
>> 	* include/pstl/algorithm_fwd.h:update from upstreamgg
>
> etc., these are newly added, so one usually writes ...: New file.

They are newly added yes, but this patch relative to my first version of
this patch where they were newly added. I commented on what changed to
get to this patch.

>
> 	Jakub

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Jonathan Wakely]

On 31/01/19 21:08 -0800, Thomas Rodgers wrote:
>Update C++17 parallel algorithms to LLVM/MIT licensed upstream sources

Some lines in bits/c++config.h need to be split before 80 columns
(with a backslash if the split is in the middle of a preprocessor
condition obviously).

There are loads of very long lines in the actual PSTL headers too, but
I think we need to keep them similar to the upstream headers to aid
merging, so can't gratuitously reformat them. I'm assuming that
doesn't apply to <bits/c++config.h> since that's our header and any
changes will need to be manually applied anyway, right?

We'll need to add a copy of the LICENSE.TXT file to our sources, since
it's referred to by the comments at the top of each PSTL file.

Typo in include/Makefile.am: ${pstl_srcdir}/memoy_impl.h
(Which will require regenerating include/Makefile.in).

The __cpp_lib_parallel_algorithm macro needs to be (conditionally)
defined in <version> as well as <algorithm> and <numeric>.

The namespaces par_backend and unseq_backend need to be uglified.

ALL the names in the __pstl::internal namespace need to be uglified:
brick_any_of
pattern_any_of
for_each_n_it_serial
brick_walk1
pattern_walk1
etc.

I see quite a few calls to functions that should probably be qualified
to prevent ADL, but that can be fixed later (and upstream first):

   return brick_count(...
   return except_handler([&]() { ...


The DejaGnu directives in the pstl tests need to be in this order:

// { dg-options "-std=gnu++17 -ltbb" }
// { dg-do run { target c++17 } }
// { dg-require-effective-target tbb-backend }

Currently the dg-require-effective-target comes before the dg-do
line, so doesn't work. DejaGnu processes the lines in order. When it
sees the dg-do for target c++17 it overrides the effect of any earlier
dg-require-effective-target.

I fixed that locally like so:
find testsuite/20_util/specialized_algorithms/pstl/ testsuite/25_algorithms/pstl/ testsuite/26_numerics/pstl/ -name \*.cc | xargs sed -i '/dg-require-eff/{h;d};/dg-do/{p;x}'
And now I don't get FAIL results on a system with no TBB installed
(which is great). I also get no FAIL results on a system with TBB
installed (also great).


There are two copies (with slight differences) of each of the
uninitialized_xxx tests:

 libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_construct.cc    |  128 +++++++
 libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_copy_move.cc    |  143 ++++++++
 libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_fill_destroy.cc |  103 ++++++
 libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_construct.cc                           |  132 ++++++++
 libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_copy_move.cc                           |  154 +++++++++
 libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_fill_destroy.cc                        |  104 ++++++

That's all for now, but I'll keep making passes over it, going into
more detail ...

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

> We'll need to add a copy of the LICENSE.TXT file to our sources, since
> it's referred to by the comments at the top of each PSTL file.
>

Right, we were going to discuss where the "right place" under
libstdc++-v3/ would be for that.

As for the rest, noted, I'll add them to the list for the next go.

Jonathan Wakely writes:

> On 31/01/19 21:08 -0800, Thomas Rodgers wrote:
>>Update C++17 parallel algorithms to LLVM/MIT licensed upstream sources
>
> Some lines in bits/c++config.h need to be split before 80 columns
> (with a backslash if the split is in the middle of a preprocessor
> condition obviously).
>
> There are loads of very long lines in the actual PSTL headers too, but
> I think we need to keep them similar to the upstream headers to aid
> merging, so can't gratuitously reformat them. I'm assuming that
> doesn't apply to <bits/c++config.h> since that's our header and any
> changes will need to be manually applied anyway, right?
>
> We'll need to add a copy of the LICENSE.TXT file to our sources, since
> it's referred to by the comments at the top of each PSTL file.
>
> Typo in include/Makefile.am: ${pstl_srcdir}/memoy_impl.h
> (Which will require regenerating include/Makefile.in).
>
> The __cpp_lib_parallel_algorithm macro needs to be (conditionally)
> defined in <version> as well as <algorithm> and <numeric>.
>
> The namespaces par_backend and unseq_backend need to be uglified.
>
> ALL the names in the __pstl::internal namespace need to be uglified:
> brick_any_of
> pattern_any_of
> for_each_n_it_serial
> brick_walk1
> pattern_walk1
> etc.
>
> I see quite a few calls to functions that should probably be qualified
> to prevent ADL, but that can be fixed later (and upstream first):
>
>   return brick_count(...
>   return except_handler([&]() { ...
>
>
> The DejaGnu directives in the pstl tests need to be in this order:
>
> // { dg-options "-std=gnu++17 -ltbb" }
> // { dg-do run { target c++17 } }
> // { dg-require-effective-target tbb-backend }
>
> Currently the dg-require-effective-target comes before the dg-do
> line, so doesn't work. DejaGnu processes the lines in order. When it
> sees the dg-do for target c++17 it overrides the effect of any earlier
> dg-require-effective-target.
>
> I fixed that locally like so:
> find testsuite/20_util/specialized_algorithms/pstl/ testsuite/25_algorithms/pstl/ testsuite/26_numerics/pstl/ -name \*.cc | xargs sed -i '/dg-require-eff/{h;d};/dg-do/{p;x}'
> And now I don't get FAIL results on a system with no TBB installed
> (which is great). I also get no FAIL results on a system with TBB
> installed (also great).
>
>
> There are two copies (with slight differences) of each of the
> uninitialized_xxx tests:
>
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_construct.cc    |  128 +++++++
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_copy_move.cc    |  143 ++++++++
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_fill_destroy.cc |  103 ++++++
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_construct.cc                           |  132 ++++++++
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_copy_move.cc                           |  154 +++++++++
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_fill_destroy.cc                        |  104 ++++++
>
> That's all for now, but I'll keep making passes over it, going into
> more detail ...

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

[-- Attachment #1: Type: text/plain, Size: 24 bytes --]

Let's try this patch -


[-- Attachment #2: pstl-integration-patch --]
[-- Type: application/x-bzip2, Size: 76454 bytes --]

[-- Attachment #3: Type: text/plain, Size: 3266 bytes --]


Jonathan Wakely writes:

> On 31/01/19 21:08 -0800, Thomas Rodgers wrote:
>>Update C++17 parallel algorithms to LLVM/MIT licensed upstream sources
>
> Some lines in bits/c++config.h need to be split before 80 columns
> (with a backslash if the split is in the middle of a preprocessor
> condition obviously).
>
> There are loads of very long lines in the actual PSTL headers too, but
> I think we need to keep them similar to the upstream headers to aid
> merging, so can't gratuitously reformat them. I'm assuming that
> doesn't apply to <bits/c++config.h> since that's our header and any
> changes will need to be manually applied anyway, right?
>
> We'll need to add a copy of the LICENSE.TXT file to our sources, since
> it's referred to by the comments at the top of each PSTL file.
>
> Typo in include/Makefile.am: ${pstl_srcdir}/memoy_impl.h
> (Which will require regenerating include/Makefile.in).
>
> The __cpp_lib_parallel_algorithm macro needs to be (conditionally)
> defined in <version> as well as <algorithm> and <numeric>.
>
> The namespaces par_backend and unseq_backend need to be uglified.
>
> ALL the names in the __pstl::internal namespace need to be uglified:
> brick_any_of
> pattern_any_of
> for_each_n_it_serial
> brick_walk1
> pattern_walk1
> etc.
>
> I see quite a few calls to functions that should probably be qualified
> to prevent ADL, but that can be fixed later (and upstream first):
>
>   return brick_count(...
>   return except_handler([&]() { ...
>
>
> The DejaGnu directives in the pstl tests need to be in this order:
>
> // { dg-options "-std=gnu++17 -ltbb" }
> // { dg-do run { target c++17 } }
> // { dg-require-effective-target tbb-backend }
>
> Currently the dg-require-effective-target comes before the dg-do
> line, so doesn't work. DejaGnu processes the lines in order. When it
> sees the dg-do for target c++17 it overrides the effect of any earlier
> dg-require-effective-target.
>
> I fixed that locally like so:
> find testsuite/20_util/specialized_algorithms/pstl/ testsuite/25_algorithms/pstl/ testsuite/26_numerics/pstl/ -name \*.cc | xargs sed -i '/dg-require-eff/{h;d};/dg-do/{p;x}'
> And now I don't get FAIL results on a system with no TBB installed
> (which is great). I also get no FAIL results on a system with TBB
> installed (also great).
>
>
> There are two copies (with slight differences) of each of the
> uninitialized_xxx tests:
>
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_construct.cc    |  128 +++++++
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_copy_move.cc    |  143 ++++++++
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/specialized.algorithms/uninitialized_fill_destroy.cc |  103 ++++++
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_construct.cc                           |  132 ++++++++
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_copy_move.cc                           |  154 +++++++++
> libstdc++-v3/testsuite/20_util/specialized_algorithms/pstl/uninitialized_fill_destroy.cc                        |  104 ++++++
>
> That's all for now, but I'll keep making passes over it, going into
> more detail ...

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Jonathan Wakely]

On 11/03/19 21:24 -0700, Thomas Rodgers wrote:
>Let's try this patch -
>


The feature test macro should be 201603L (in <execution> and
<version>):

+// Feature test macro for parallel algorithms
+# define __cpp_lib_parallel_algorithm 201703L

***

The new files have copyright dates of 2018, but it's taken so long to
get the licensing changes done and for me to review it that they need
to say "2018-2019" now:

+++ b/libstdc++-v3/include/std/execution
@@ -0,0 +1,58 @@
+// <execution> -*- C++ -*-
+
+// Copyright (C) 2018 Free Software Foundation, Inc.

***

The <execution> header warns if included pre-C++17 but it should just
not define anything:

+#if __cplusplus < 201703L
+# include <bits/c++0x_warning.h>
+#else

We only give that warning for C++11 headers, but for anything newer it
should be just:

+#if __cplusplus >= 201703L

***

There are still a couple of un-uglified names I noticed:
parallel_set_union_op, is_heap_until_local

***

The copyright notices at the top of each file seem a bit out of place
in the GCC tree:

+//===-- execution_defs.h --------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//

I wonder if we should put another comment before that, saying GCC uses
the PSTL code from the LLVM upstream, or something like that. That can
wait though.

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

Jonathan Wakely writes:

> On 11/03/19 21:24 -0700, Thomas Rodgers wrote:
>>Let's try this patch -
>>
>
>
> The feature test macro should be 201603L (in <execution> and
> <version>):
>
> +// Feature test macro for parallel algorithms
> +# define __cpp_lib_parallel_algorithm 201703L
>
> ***
>
> The new files have copyright dates of 2018, but it's taken so long to
> get the licensing changes done and for me to review it that they need
> to say "2018-2019" now:
>
> +++ b/libstdc++-v3/include/std/execution
> @@ -0,0 +1,58 @@
> +// <execution> -*- C++ -*-
> +
> +// Copyright (C) 2018 Free Software Foundation, Inc.
>
> ***
>
> The <execution> header warns if included pre-C++17 but it should just
> not define anything:
>
> +#if __cplusplus < 201703L
> +# include <bits/c++0x_warning.h>
> +#else
>
> We only give that warning for C++11 headers, but for anything newer it
> should be just:
>
> +#if __cplusplus >= 201703L

Did you mean

+#if __cplusplus >= 201603L

?

>
> ***
>
> There are still a couple of un-uglified names I noticed:
> parallel_set_union_op, is_heap_until_local
>
> ***
>
> The copyright notices at the top of each file seem a bit out of place
> in the GCC tree:
>
> +//===-- execution_defs.h --------------------------------------------------===//
> +//
> +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
> +// See https://llvm.org/LICENSE.txt for license information.
> +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
> +//
> +//===----------------------------------------------------------------------===//
>
> I wonder if we should put another comment before that, saying GCC uses
> the PSTL code from the LLVM upstream, or something like that. That can
> wait though.

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Ville Voutilainen]

On Wed, 20 Mar 2019 at 22:05, Thomas Rodgers <trodgers@redhat.com> wrote:
> > +#if __cplusplus < 201703L
> > +# include <bits/c++0x_warning.h>
> > +#else
> >
> > We only give that warning for C++11 headers, but for anything newer it
> > should be just:
> >
> > +#if __cplusplus >= 201703L
>
> Did you mean
>
> +#if __cplusplus >= 201603L
>
> ?

No. Such a value does not exist. He means what he wrote; these
parallel algos should be available starting with C++17 and in newer
versions as well, and they should just not-exist in earlier versions,
without a header warning.

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Jakub Jelinek]

On Wed, Mar 20, 2019 at 01:05:10PM -0700, Thomas Rodgers wrote:
> > We only give that warning for C++11 headers, but for anything newer it
> > should be just:
> >
> > +#if __cplusplus >= 201703L
> 
> Did you mean
> 
> +#if __cplusplus >= 201603L

I guess so:
http://eel.is/c++draft/support.limits.general
as well as
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0941r2.html
says:
__cpp_lib_parallel_algorithm 	201603L 	<algorithm> <numeric> 

	Jakub

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Jakub Jelinek]

On Wed, Mar 20, 2019 at 09:13:42PM +0100, Jakub Jelinek wrote:
> On Wed, Mar 20, 2019 at 01:05:10PM -0700, Thomas Rodgers wrote:
> > > We only give that warning for C++11 headers, but for anything newer it
> > > should be just:
> > >
> > > +#if __cplusplus >= 201703L
> > 
> > Did you mean
> > 
> > +#if __cplusplus >= 201603L
> 
> I guess so:
> http://eel.is/c++draft/support.limits.general
> as well as
> http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0941r2.html
> says:
> __cpp_lib_parallel_algorithm 	201603L 	<algorithm> <numeric> 

Sorry, ignore my mail.

	Jakub

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Ville Voutilainen]

On Wed, 20 Mar 2019 at 22:15, Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Mar 20, 2019 at 09:13:42PM +0100, Jakub Jelinek wrote:
> > On Wed, Mar 20, 2019 at 01:05:10PM -0700, Thomas Rodgers wrote:
> > > > We only give that warning for C++11 headers, but for anything newer it
> > > > should be just:
> > > >
> > > > +#if __cplusplus >= 201703L
> > >
> > > Did you mean
> > >
> > > +#if __cplusplus >= 201603L
> >
> > I guess so:
> > http://eel.is/c++draft/support.limits.general
> > as well as
> > http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0941r2.html
> > says:
> > __cpp_lib_parallel_algorithm  201603L         <algorithm> <numeric>
>
> Sorry, ignore my mail.

That's fine - that's the value of that particular feature-testing
macro, not the value of __cplusplus. :)

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

Ignore

Thomas Rodgers writes:

> Jonathan Wakely writes:
>
>> On 11/03/19 21:24 -0700, Thomas Rodgers wrote:
>>>Let's try this patch -
>>>
>>
>>
>> The feature test macro should be 201603L (in <execution> and
>> <version>):
>>
>> +// Feature test macro for parallel algorithms
>> +# define __cpp_lib_parallel_algorithm 201703L
>>
>> ***
>>
>> The new files have copyright dates of 2018, but it's taken so long to
>> get the licensing changes done and for me to review it that they need
>> to say "2018-2019" now:
>>
>> +++ b/libstdc++-v3/include/std/execution
>> @@ -0,0 +1,58 @@
>> +// <execution> -*- C++ -*-
>> +
>> +// Copyright (C) 2018 Free Software Foundation, Inc.
>>
>> ***
>>
>> The <execution> header warns if included pre-C++17 but it should just
>> not define anything:
>>
>> +#if __cplusplus < 201703L
>> +# include <bits/c++0x_warning.h>
>> +#else
>>
>> We only give that warning for C++11 headers, but for anything newer it
>> should be just:
>>
>> +#if __cplusplus >= 201703L
>
> Did you mean
>
> +#if __cplusplus >= 201603L
>
> ?
>
>>
>> ***
>>
>> There are still a couple of un-uglified names I noticed:
>> parallel_set_union_op, is_heap_until_local
>>
>> ***
>>
>> The copyright notices at the top of each file seem a bit out of place
>> in the GCC tree:
>>
>> +//===-- execution_defs.h --------------------------------------------------===//
>> +//
>> +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
>> +// See https://llvm.org/LICENSE.txt for license information.
>> +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
>> +//
>> +//===----------------------------------------------------------------------===//
>>
>> I wonder if we should put another comment before that, saying GCC uses
>> the PSTL code from the LLVM upstream, or something like that. That can
>> wait though.

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

[-- Attachment #1: Type: text/plain, Size: 14 bytes --]

See attached.

[-- Attachment #2: revised pstl integration patch --]
[-- Type: application/x-bzip2, Size: 76400 bytes --]

[-- Attachment #3: Type: text/plain, Size: 1695 bytes --]


Jonathan Wakely writes:

> On 11/03/19 21:24 -0700, Thomas Rodgers wrote:
>>Let's try this patch -
>>
>
>
> The feature test macro should be 201603L (in <execution> and
> <version>):
>
> +// Feature test macro for parallel algorithms
> +# define __cpp_lib_parallel_algorithm 201703L
>
> ***
>
> The new files have copyright dates of 2018, but it's taken so long to
> get the licensing changes done and for me to review it that they need
> to say "2018-2019" now:
>
> +++ b/libstdc++-v3/include/std/execution
> @@ -0,0 +1,58 @@
> +// <execution> -*- C++ -*-
> +
> +// Copyright (C) 2018 Free Software Foundation, Inc.
>
> ***
>
> The <execution> header warns if included pre-C++17 but it should just
> not define anything:
>
> +#if __cplusplus < 201703L
> +# include <bits/c++0x_warning.h>
> +#else
>
> We only give that warning for C++11 headers, but for anything newer it
> should be just:
>
> +#if __cplusplus >= 201703L
>
> ***
>
> There are still a couple of un-uglified names I noticed:
> parallel_set_union_op, is_heap_until_local
>
> ***
>
> The copyright notices at the top of each file seem a bit out of place
> in the GCC tree:
>
> +//===-- execution_defs.h --------------------------------------------------===//
> +//
> +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
> +// See https://llvm.org/LICENSE.txt for license information.
> +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
> +//
> +//===----------------------------------------------------------------------===//
>
> I wonder if we should put another comment before that, saying GCC uses
> the PSTL code from the LLVM upstream, or something like that. That can
> wait though.

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

[-- Attachment #1: Revised pstl integration patch --]
[-- Type: application/x-bzip2, Size: 76406 bytes --]

[-- Attachment #2: Type: text/plain, Size: 1872 bytes --]


This time with the changelog reflecting the updated files in include/std

Thomas Rodgers writes:

> See attached.
>
> Jonathan Wakely writes:
>
>> On 11/03/19 21:24 -0700, Thomas Rodgers wrote:
>>>Let's try this patch -
>>>
>>
>>
>> The feature test macro should be 201603L (in <execution> and
>> <version>):
>>
>> +// Feature test macro for parallel algorithms
>> +# define __cpp_lib_parallel_algorithm 201703L
>>
>> ***
>>
>> The new files have copyright dates of 2018, but it's taken so long to
>> get the licensing changes done and for me to review it that they need
>> to say "2018-2019" now:
>>
>> +++ b/libstdc++-v3/include/std/execution
>> @@ -0,0 +1,58 @@
>> +// <execution> -*- C++ -*-
>> +
>> +// Copyright (C) 2018 Free Software Foundation, Inc.
>>
>> ***
>>
>> The <execution> header warns if included pre-C++17 but it should just
>> not define anything:
>>
>> +#if __cplusplus < 201703L
>> +# include <bits/c++0x_warning.h>
>> +#else
>>
>> We only give that warning for C++11 headers, but for anything newer it
>> should be just:
>>
>> +#if __cplusplus >= 201703L
>>
>> ***
>>
>> There are still a couple of un-uglified names I noticed:
>> parallel_set_union_op, is_heap_until_local
>>
>> ***
>>
>> The copyright notices at the top of each file seem a bit out of place
>> in the GCC tree:
>>
>> +//===-- execution_defs.h --------------------------------------------------===//
>> +//
>> +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
>> +// See https://llvm.org/LICENSE.txt for license information.
>> +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
>> +//
>> +//===----------------------------------------------------------------------===//
>>
>> I wonder if we should put another comment before that, saying GCC uses
>> the PSTL code from the LLVM upstream, or something like that. That can
>> wait though.

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

[-- Attachment #1: Revised pstl integration patch --]
[-- Type: application/x-bzip2, Size: 76431 bytes --]

[-- Attachment #2: Type: text/plain, Size: 1987 bytes --]


Fixed a failing test.

Thomas Rodgers writes:

> This time with the changelog reflecting the updated files in include/std
>
> Thomas Rodgers writes:
>
>> See attached.
>>
>> Jonathan Wakely writes:
>>
>>> On 11/03/19 21:24 -0700, Thomas Rodgers wrote:
>>>>Let's try this patch -
>>>>
>>>
>>>
>>> The feature test macro should be 201603L (in <execution> and
>>> <version>):
>>>
>>> +// Feature test macro for parallel algorithms
>>> +# define __cpp_lib_parallel_algorithm 201703L
>>>
>>> ***
>>>
>>> The new files have copyright dates of 2018, but it's taken so long to
>>> get the licensing changes done and for me to review it that they need
>>> to say "2018-2019" now:
>>>
>>> +++ b/libstdc++-v3/include/std/execution
>>> @@ -0,0 +1,58 @@
>>> +// <execution> -*- C++ -*-
>>> +
>>> +// Copyright (C) 2018 Free Software Foundation, Inc.
>>>
>>> ***
>>>
>>> The <execution> header warns if included pre-C++17 but it should just
>>> not define anything:
>>>
>>> +#if __cplusplus < 201703L
>>> +# include <bits/c++0x_warning.h>
>>> +#else
>>>
>>> We only give that warning for C++11 headers, but for anything newer it
>>> should be just:
>>>
>>> +#if __cplusplus >= 201703L
>>>
>>> ***
>>>
>>> There are still a couple of un-uglified names I noticed:
>>> parallel_set_union_op, is_heap_until_local
>>>
>>> ***
>>>
>>> The copyright notices at the top of each file seem a bit out of place
>>> in the GCC tree:
>>>
>>> +//===-- execution_defs.h --------------------------------------------------===//
>>> +//
>>> +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
>>> +// See https://llvm.org/LICENSE.txt for license information.
>>> +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
>>> +//
>>> +//===----------------------------------------------------------------------===//
>>>
>>> I wonder if we should put another comment before that, saying GCC uses
>>> the PSTL code from the LLVM upstream, or something like that. That can
>>> wait though.

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Jonathan Wakely]

On 20/03/19 14:05 -0700, Thomas Rodgers wrote:

>
>Fixed a failing test.

Thanks. Apart from the changelog issue I mentioned on IRC, the only
other required change I see is in include/Makefile.am:

@@ -1480,7 +1512,9 @@ install-headers:
 	$(mkinstalldirs) $(DESTDIR)${host_installdir}/../ext
 	for file in ${ext_host_headers}; do \
 	  $(INSTALL_DATA) $${file} $(DESTDIR)${host_installdir}/../ext; done
-
+	$(mkinstalldirs) ($DESTDIR)${gxx_include_dir}/${pstl_builddir}

This needs to be $(DESTDIR) not ($DESTDIR).

***

I also see a couple of things that we probably can't fix for now, but
I'll mention them anyway for later consideration.

In <bits/glue_execution_defs.h> there's a local include using ""
instead of <> (see PR libstdc++/88066 for a similar case I changed
recently):

#include "execution_defs.h"

We could fix that one, but then *all* the relative includes in
include/pstl/*  use "" too. Do you think upstream would change to use
#include <pstl/blah.h> instead of #include "blah.h" ? It looks like
upstream uses <pstl/include/...> instead so maybe that's something to
change in your script to import the upstream code.

Even if we fix it, TBB headers use "" instead of <>.  And it might not
be a problem for any compiler except GCC, because only we support the
silly -I- option that makes this matter.


I also noticed that simply including <execution> (even if you don't
use anything from it) will cause a link-time dependency on TBB:

/usr/bin/ld: /tmp/ccd7w5Oo.o: in function `tbb::interface7::task_arena::current_thread_index()':
/usr/include/tbb/task_arena.h:358: undefined reference to `tbb::interface7::internal::task_arena_base::internal_current_slot()'
collect2: error: ld returned 1 exit status

This dependency comes from <tbb/partioner.h> which is included by
<tbb/parallel_scan.h> which is included by <bits/execution_defs.h>.

I think that's OK for now, as no existing code using GCC includes the
<execution> header (because it doesn't exist yet). It would be good if
we can find a way to solve that eventually though. Maybe the eventual
solution will just be a non-TBB backend using OpenMP.

Exporting something like this from libstdc++.so would work, but is
disgusting (and presumably "tbb::interface7" changes from release to
release?"):

extern "C" [[__gnu__::__weak__]]
int
_ZN3tbb10interface78internal15task_arena_base21internal_current_slotEv()
{ return -1; }

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Jonathan Wakely]

On 21/03/19 11:37 +0000, Jonathan Wakely wrote:
>Exporting something like this from libstdc++.so would work, but is
>disgusting (and presumably "tbb::interface7" changes from release to
>release?"):
>
>extern "C" [[__gnu__::__weak__]]
>int
>_ZN3tbb10interface78internal15task_arena_base21internal_current_slotEv()
>{ return -1; }

Actually looking at TBB, -2 would make sense (as that's the value of
task_arena::not_initialized), but I don't think we want to do this
anyway.

>

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

[-- Attachment #1: pstl integration patch --]
[-- Type: application/x-bzip2, Size: 76440 bytes --]

[-- Attachment #2: Type: text/plain, Size: 2528 bytes --]


Jonathan Wakely writes:

> On 20/03/19 14:05 -0700, Thomas Rodgers wrote:
>
>>
>>Fixed a failing test.
>
> Thanks. Apart from the changelog issue I mentioned on IRC, the only
> other required change I see is in include/Makefile.am:
>
> @@ -1480,7 +1512,9 @@ install-headers:
> 	$(mkinstalldirs) $(DESTDIR)${host_installdir}/../ext
> 	for file in ${ext_host_headers}; do \
> 	  $(INSTALL_DATA) $${file} $(DESTDIR)${host_installdir}/../ext; done
> -
> +	$(mkinstalldirs) ($DESTDIR)${gxx_include_dir}/${pstl_builddir}
>
> This needs to be $(DESTDIR) not ($DESTDIR).
>
> ***
>
> I also see a couple of things that we probably can't fix for now, but
> I'll mention them anyway for later consideration.
>
> In <bits/glue_execution_defs.h> there's a local include using ""
> instead of <> (see PR libstdc++/88066 for a similar case I changed
> recently):
>
> #include "execution_defs.h"
>
> We could fix that one, but then *all* the relative includes in
> include/pstl/*  use "" too. Do you think upstream would change to use
> #include <pstl/blah.h> instead of #include "blah.h" ? It looks like
> upstream uses <pstl/include/...> instead so maybe that's something to
> change in your script to import the upstream code.
>
> Even if we fix it, TBB headers use "" instead of <>.  And it might not
> be a problem for any compiler except GCC, because only we support the
> silly -I- option that makes this matter.
>
>
> I also noticed that simply including <execution> (even if you don't
> use anything from it) will cause a link-time dependency on TBB:
>
> /usr/bin/ld: /tmp/ccd7w5Oo.o: in function `tbb::interface7::task_arena::current_thread_index()':
> /usr/include/tbb/task_arena.h:358: undefined reference to `tbb::interface7::internal::task_arena_base::internal_current_slot()'
> collect2: error: ld returned 1 exit status
>
> This dependency comes from <tbb/partioner.h> which is included by
> <tbb/parallel_scan.h> which is included by <bits/execution_defs.h>.
>
> I think that's OK for now, as no existing code using GCC includes the
> <execution> header (because it doesn't exist yet). It would be good if
> we can find a way to solve that eventually though. Maybe the eventual
> solution will just be a non-TBB backend using OpenMP.
>
> Exporting something like this from libstdc++.so would work, but is
> disgusting (and presumably "tbb::interface7" changes from release to
> release?"):
>
> extern "C" [[__gnu__::__weak__]]
> int
> _ZN3tbb10interface78internal15task_arena_base21internal_current_slotEv()
> { return -1; }

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

[-- Attachment #1: Type: text/plain, Size: 22 bytes --]


Fixed up change log.

[-- Attachment #2: pstl integration patch --]
[-- Type: application/x-bzip2, Size: 76426 bytes --]

[-- Attachment #3: Type: text/plain, Size: 2617 bytes --]


Thomas Rodgers writes:

> Jonathan Wakely writes:
>
>> On 20/03/19 14:05 -0700, Thomas Rodgers wrote:
>>
>>>
>>>Fixed a failing test.
>>
>> Thanks. Apart from the changelog issue I mentioned on IRC, the only
>> other required change I see is in include/Makefile.am:
>>
>> @@ -1480,7 +1512,9 @@ install-headers:
>> 	$(mkinstalldirs) $(DESTDIR)${host_installdir}/../ext
>> 	for file in ${ext_host_headers}; do \
>> 	  $(INSTALL_DATA) $${file} $(DESTDIR)${host_installdir}/../ext; done
>> -
>> +	$(mkinstalldirs) ($DESTDIR)${gxx_include_dir}/${pstl_builddir}
>>
>> This needs to be $(DESTDIR) not ($DESTDIR).
>>
>> ***
>>
>> I also see a couple of things that we probably can't fix for now, but
>> I'll mention them anyway for later consideration.
>>
>> In <bits/glue_execution_defs.h> there's a local include using ""
>> instead of <> (see PR libstdc++/88066 for a similar case I changed
>> recently):
>>
>> #include "execution_defs.h"
>>
>> We could fix that one, but then *all* the relative includes in
>> include/pstl/*  use "" too. Do you think upstream would change to use
>> #include <pstl/blah.h> instead of #include "blah.h" ? It looks like
>> upstream uses <pstl/include/...> instead so maybe that's something to
>> change in your script to import the upstream code.
>>
>> Even if we fix it, TBB headers use "" instead of <>.  And it might not
>> be a problem for any compiler except GCC, because only we support the
>> silly -I- option that makes this matter.
>>
>>
>> I also noticed that simply including <execution> (even if you don't
>> use anything from it) will cause a link-time dependency on TBB:
>>
>> /usr/bin/ld: /tmp/ccd7w5Oo.o: in function `tbb::interface7::task_arena::current_thread_index()':
>> /usr/include/tbb/task_arena.h:358: undefined reference to `tbb::interface7::internal::task_arena_base::internal_current_slot()'
>> collect2: error: ld returned 1 exit status
>>
>> This dependency comes from <tbb/partioner.h> which is included by
>> <tbb/parallel_scan.h> which is included by <bits/execution_defs.h>.
>>
>> I think that's OK for now, as no existing code using GCC includes the
>> <execution> header (because it doesn't exist yet). It would be good if
>> we can find a way to solve that eventually though. Maybe the eventual
>> solution will just be a non-TBB backend using OpenMP.
>>
>> Exporting something like this from libstdc++.so would work, but is
>> disgusting (and presumably "tbb::interface7" changes from release to
>> release?"):
>>
>> extern "C" [[__gnu__::__weak__]]
>> int
>> _ZN3tbb10interface78internal15task_arena_base21internal_current_slotEv()
>> { return -1; }

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Jonathan Wakely]

On 21/03/19 15:19 -0700, Thomas Rodgers wrote:
>
>Fixed up change log.

Thanks, this version seems to have addressed everything.

As this was one of our big ticket features for stage 1 (but was
delayed due to the necessary legal steps that Intel took to kindly
transfer this to the LLVM project) we still want to go ahead and
commit this, even though it's late in stage 4.

There's some risk in adding this much code this late, but the
additions to existing headers are all guarded with
#if __cplusplus > 201402L
so it only affects C++17 mode (not the C++14 defaults) and will be a
no-op for most users.

So I'm going to approve this for trunk (and Thomas and I are committed
to dealing with any problems that might show up as soon as they
happen).

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Jonathan Wakely]

On 21/03/19 22:32 +0000, Jonathan Wakely wrote:
>On 21/03/19 15:19 -0700, Thomas Rodgers wrote:
>>
>>Fixed up change log.
>
>Thanks, this version seems to have addressed everything.
>
>As this was one of our big ticket features for stage 1 (but was
>delayed due to the necessary legal steps that Intel took to kindly
>transfer this to the LLVM project) we still want to go ahead and
>commit this, even though it's late in stage 4.
>
>There's some risk in adding this much code this late, but the
>additions to existing headers are all guarded with
>#if __cplusplus > 201402L
>so it only affects C++17 mode (not the C++14 defaults) and will be a
>no-op for most users.
>
>So I'm going to approve this for trunk (and Thomas and I are committed
>to dealing with any problems that might show up as soon as they
>happen).
>

This has now been committed to trunk (by me, because of some git-svn
weirdness at Thomas's end and at mine, so despite our best efforts it
ended up using my login credentials).

Thanks Thomas!

And thanks Alexey and Mikhail and everyone else at Intel for making
this code available in the first place. I'm very pleased to have this
in libstdc++ in time for the next GCC release.

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Jonathan Wakely]

On 21/03/19 23:56 +0000, Jonathan Wakely wrote:
>On 21/03/19 22:32 +0000, Jonathan Wakely wrote:
>>On 21/03/19 15:19 -0700, Thomas Rodgers wrote:
>>>
>>>Fixed up change log.
>>
>>Thanks, this version seems to have addressed everything.
>>
>>As this was one of our big ticket features for stage 1 (but was
>>delayed due to the necessary legal steps that Intel took to kindly
>>transfer this to the LLVM project) we still want to go ahead and
>>commit this, even though it's late in stage 4.
>>
>>There's some risk in adding this much code this late, but the
>>additions to existing headers are all guarded with
>>#if __cplusplus > 201402L
>>so it only affects C++17 mode (not the C++14 defaults) and will be a
>>no-op for most users.
>>
>>So I'm going to approve this for trunk (and Thomas and I are committed
>>to dealing with any problems that might show up as soon as they
>>happen).
>>
>
>This has now been committed to trunk (by me, because of some git-svn
>weirdness at Thomas's end and at mine, so despite our best efforts it
>ended up using my login credentials).
>
>Thanks Thomas!
>
>And thanks Alexey and Mikhail and everyone else at Intel for making
>this code available in the first place. I'm very pleased to have this
>in libstdc++ in time for the next GCC release.

I keep forgetting to add that docs for this stuff will be coming some
time next week, describing the TBB dependency that's needed to use
these parallel algos.

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Schwinge]

Hi!

If that's of any help to document the version dependencies:

On Fri, 22 Mar 2019 00:04:30 +0000, Jonathan Wakely <jwakely@redhat.com> wrote:
> I keep forgetting to add that docs for this stuff will be coming some
> time next week, describing the TBB dependency that's needed to use
> these parallel algos.

On an Ubuntu 12.10 x86_64 GNU/Linux system (yes, a bit old by now), I saw
all these test UNSUPPORTED.

I then installed 'libtbb-dev' (and the implied 'libtbb2'; both version
4.0+r233-1), and then saw all? tests FAIL because of:

    [...]/libstdc++-v3/include/pstl/parallel_backend_tbb.h:25: fatal error: tbb/task_arena.h: No such file or directory

I then manually installed the Ubuntu trusty (14.04LTS) version
4.2~20130725-1.1ubuntu1 packages, and then saw all? tests FAIL because
of:

    [...]/libstdc++-v3/include/pstl/parallel_backend_tbb.h:29: error: #error Intel(R) Threading Building Blocks 2018 is required; older versions are not supported.

So I suppose I need the most recent Ubuntu disco version 2018~U6-4
packages.  Because of:

    dpkg-deb: error: archive 'libtbb-dev_2018~U6-4_amd64.deb' contains not understood data member control.tar.xz, giving up

..., I manually had to convert to '*.gz' files the'.xz' files inside of
these '*.deb' archives, which yet still failed to install:

    dpkg: dependency problems prevent configuration of libtbb2:amd64:
     libtbb2:amd64 depends on libstdc++6 (>= 7); however:
      Version of libstdc++6:amd64 on system is 4.7.2-2ubuntu1.

..., which isn't really a problem as I'll obviously be testing against a
new-enough GCC/libstdc++ ;-) -- so, installed these packages with
'--force-depends-version'.

With that, the tests then all PASS for the default multilib, but all?
FAIL for '-m32' testing:

    [...]/ld: cannot find -ltbb

There is no 32-bit 'libtbb' available.  I suppose the problem is that
'check_effective_target_tbb-backend' just does a 'preprocess' test
checking for existence of '<tbb/tbb.h>', but doesn't actually try to
link, whereas all? the test cases specify '-ltbb' via 'dg-options'.

That said, instead of specifying:

    // { dg-options "-std=gnu++17 -ltbb" }
    // { dg-do run { target c++17 } }
    // { dg-require-effective-target tbb-backend }

... in all? these test case files, isn't there some DejaGnu directive
that checks whether support is available (else UNSUPPORTED), while also
adding the necessary compiler options (here: '-ltbb')?


Also, shouldn't 'check_effective_target_tbb-backend' (and also some of
the other checks in 'libstdc++-v3/testsuite/lib/libstdc++.exp'?) be using
'check_v3_target_prop_cached' to avoid checking the same things again and
again?


Grüße
 Thomas

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Jonathan Wakely]

On 27/03/19 15:51 +0100, Thomas Schwinge wrote:
>Hi!
>
>If that's of any help to document the version dependencies:

Thanks for t his.

>On Fri, 22 Mar 2019 00:04:30 +0000, Jonathan Wakely <jwakely@redhat.com> wrote:
>> I keep forgetting to add that docs for this stuff will be coming some
>> time next week, describing the TBB dependency that's needed to use
>> these parallel algos.
>
>On an Ubuntu 12.10 x86_64 GNU/Linux system (yes, a bit old by now), I saw
>all these test UNSUPPORTED.
>
>I then installed 'libtbb-dev' (and the implied 'libtbb2'; both version
>4.0+r233-1), and then saw all? tests FAIL because of:
>
>    [...]/libstdc++-v3/include/pstl/parallel_backend_tbb.h:25: fatal error: tbb/task_arena.h: No such file or directory
>
>I then manually installed the Ubuntu trusty (14.04LTS) version
>4.2~20130725-1.1ubuntu1 packages, and then saw all? tests FAIL because
>of:
>
>    [...]/libstdc++-v3/include/pstl/parallel_backend_tbb.h:29: error: #error Intel(R) Threading Building Blocks 2018 is required; older versions are not supported.

Yes, that's a known dependency.

Tom, I think check_effective_target_tbb-backend should probably check
the TBB_VERSION_MAJOR macro  in <tbb/tbb_stddef.h> and fail if an
older version is detected.

That will only help when running our tests though, not when users try
to use the algos, so we probably need to fail more gracefully if the
user has an odler TBB installed. If there's a header which is present
in TBB-2018 but not older versions then <bits/c++config.h> could check
for that instead of (or as well as) <tbb/tbb.h>.


>So I suppose I need the most recent Ubuntu disco version 2018~U6-4
>packages.  Because of:
>
>    dpkg-deb: error: archive 'libtbb-dev_2018~U6-4_amd64.deb' contains not understood data member control.tar.xz, giving up
>
>..., I manually had to convert to '*.gz' files the'.xz' files inside of
>these '*.deb' archives, which yet still failed to install:
>
>    dpkg: dependency problems prevent configuration of libtbb2:amd64:
>     libtbb2:amd64 depends on libstdc++6 (>= 7); however:
>      Version of libstdc++6:amd64 on system is 4.7.2-2ubuntu1.
>
>..., which isn't really a problem as I'll obviously be testing against a
>new-enough GCC/libstdc++ ;-) -- so, installed these packages with
>'--force-depends-version'.
>
>With that, the tests then all PASS for the default multilib, but all?
>FAIL for '-m32' testing:
>
>    [...]/ld: cannot find -ltbb
>
>There is no 32-bit 'libtbb' available.  I suppose the problem is that
>'check_effective_target_tbb-backend' just does a 'preprocess' test
>checking for existence of '<tbb/tbb.h>', but doesn't actually try to
>link, whereas all? the test cases specify '-ltbb' via 'dg-options'.

Right. Jakub noticed the same issue.

>That said, instead of specifying:
>
>    // { dg-options "-std=gnu++17 -ltbb" }
>    // { dg-do run { target c++17 } }
>    // { dg-require-effective-target tbb-backend }
>
>... in all? these test case files, isn't there some DejaGnu directive
>that checks whether support is available (else UNSUPPORTED), while also
>adding the necessary compiler options (here: '-ltbb')?

I don't think we can do that in one go, can we?

We can add something so { dg-add-options tbb } adds the required
options, but I don't think it will make it UNSUPPORTED when necessary.
Maybe I'm missing something.

>Also, shouldn't 'check_effective_target_tbb-backend' (and also some of
>the other checks in 'libstdc++-v3/testsuite/lib/libstdc++.exp'?) be using
>'check_v3_target_prop_cached' to avoid checking the same things again and
>again?

Yes. I think it was probably written before that caching helper was
added.

Re: [PATCH] Integration of parallel standard algorithms for c++17

[Thomas Rodgers]

Jonathan Wakely writes:

> On 27/03/19 15:51 +0100, Thomas Schwinge wrote:
>>Hi!
>>
>>If that's of any help to document the version dependencies:
>
> Thanks for t his.
>
>>On Fri, 22 Mar 2019 00:04:30 +0000, Jonathan Wakely <jwakely@redhat.com> wrote:
>>> I keep forgetting to add that docs for this stuff will be coming some
>>> time next week, describing the TBB dependency that's needed to use
>>> these parallel algos.
>>
>>On an Ubuntu 12.10 x86_64 GNU/Linux system (yes, a bit old by now), I saw
>>all these test UNSUPPORTED.
>>
>>I then installed 'libtbb-dev' (and the implied 'libtbb2'; both version
>>4.0+r233-1), and then saw all? tests FAIL because of:
>>
>>    [...]/libstdc++-v3/include/pstl/parallel_backend_tbb.h:25: fatal error: tbb/task_arena.h: No such file or directory
>>
>>I then manually installed the Ubuntu trusty (14.04LTS) version
>>4.2~20130725-1.1ubuntu1 packages, and then saw all? tests FAIL because
>>of:
>>
>>    [...]/libstdc++-v3/include/pstl/parallel_backend_tbb.h:29: error: #error Intel(R) Threading Building Blocks 2018 is required; older versions are not supported.
>
> Yes, that's a known dependency.
>
> Tom, I think check_effective_target_tbb-backend should probably check
> the TBB_VERSION_MAJOR macro  in <tbb/tbb_stddef.h> and fail if an
> older version is detected.
>

Ok, I'll look at adding that check.

> That will only help when running our tests though, not when users try
> to use the algos, so we probably need to fail more gracefully if the
> user has an odler TBB installed. If there's a header which is present
> in TBB-2018 but not older versions then <bits/c++config.h> could check
> for that instead of (or as well as) <tbb/tbb.h>.
>

Yes the TBB-2018 is a hard dependency in the Intel backend, but I can
likely make this more graceful. There is also a serial 'fallback' backend
currently in the works in the upstream, which would at least give
something to fallback to here.

>
>>So I suppose I need the most recent Ubuntu disco version 2018~U6-4
>>packages.  Because of:
>>
>>    dpkg-deb: error: archive 'libtbb-dev_2018~U6-4_amd64.deb' contains not understood data member control.tar.xz, giving up
>>
>>..., I manually had to convert to '*.gz' files the'.xz' files inside of
>>these '*.deb' archives, which yet still failed to install:
>>
>>    dpkg: dependency problems prevent configuration of libtbb2:amd64:
>>     libtbb2:amd64 depends on libstdc++6 (>= 7); however:
>>      Version of libstdc++6:amd64 on system is 4.7.2-2ubuntu1.
>>
>>..., which isn't really a problem as I'll obviously be testing against a
>>new-enough GCC/libstdc++ ;-) -- so, installed these packages with
>>'--force-depends-version'.
>>
>>With that, the tests then all PASS for the default multilib, but all?
>>FAIL for '-m32' testing:
>>
>>    [...]/ld: cannot find -ltbb
>>
>>There is no 32-bit 'libtbb' available.  I suppose the problem is that
>>'check_effective_target_tbb-backend' just does a 'preprocess' test
>>checking for existence of '<tbb/tbb.h>', but doesn't actually try to
>>link, whereas all? the test cases specify '-ltbb' via 'dg-options'.
>
> Right. Jakub noticed the same issue.
>
>>That said, instead of specifying:
>>
>>    // { dg-options "-std=gnu++17 -ltbb" }
>>    // { dg-do run { target c++17 } }
>>    // { dg-require-effective-target tbb-backend }
>>
>>... in all? these test case files, isn't there some DejaGnu directive
>>that checks whether support is available (else UNSUPPORTED), while also
>>adding the necessary compiler options (here: '-ltbb')?
>
> I don't think we can do that in one go, can we?
>
> We can add something so { dg-add-options tbb } adds the required
> options, but I don't think it will make it UNSUPPORTED when necessary.
> Maybe I'm missing something.
>
>>Also, shouldn't 'check_effective_target_tbb-backend' (and also some of
>>the other checks in 'libstdc++-v3/testsuite/lib/libstdc++.exp'?) be using
>>'check_v3_target_prop_cached' to avoid checking the same things again and
>>again?
>
> Yes. I think it was probably written before that caching helper was
> added.