This is an archive of the discontinued LLVM Phabricator instance.

[pstl] Do not inject execution policies into namespace ::pstl in the tests
AbandonedPublic

Authored by ldionne on Apr 10 2019, 1:18 PM.

Details

Summary

Doing so makes the test suite test non-conforming behavior that neither
libc++ nor libstdc++ needs.

Event Timeline

ldionne created this revision.Apr 10 2019, 1:18 PM
MikeDvorskiy added a comment.EditedApr 11 2019, 2:12 AM

Guys,
I understand that given pstl code is becoming a part of C++ standard library and , of course should be std::execution::unsequenced_policy and so on..

But, there is PSTL standalone version, which works with the other compilers and environment , MSVC for example, where there is another implementation of Parallel STL.
In that case we have to differ a namespace where the execution policies are defined, otherwise a user code (and the tests) will use MSVC implementation of parallel algorithms instead of our...

I have two alternative ideas here:

  1. Using a special macro like

__PSTL_NAMESPACE_FOR_EXECUTION

On LLVM side it will be defined as

#define __PSTL_NAMESPACE_FOR_EXECUTION std

On our side it will be defined as

#define __PSTL_NAMESPACE_FOR_EXECUTION __pstl
  1. Or just use

using namespace std;

or (depends on a macro like PSTL_POLICY_USAGE)

using namespace __pstl;

in test\support\pstl_test_config.h or test\support\utils.h

ldionne added a comment.EditedApr 11 2019, 3:24 PM

Guys,
I understand that given pstl code is becoming a part of C++ standard library and , of course should be std::execution::unsequenced_policy and so on..

But, there is PSTL standalone version, which works with the other compilers and environment , MSVC for example, where there is another implementation of Parallel STL.
In that case we have to differ a namespace where the execution policies are defined, otherwise a user code (and the tests) will use MSVC implementation of parallel algorithms instead of our...

I have two alternative ideas here:
[...]

Of course, we should still support the use case you have in mind. My goal is not to make the PSTL *only* useful for standard libraries -- the PSTL shouldn't inject its declarations into namespace std, and this was a follow-up fix I planned on making. Let me address your concern first and I think this patch will then make more sense to you.

Basically, I was thinking (like you) that we should have a way to customize the namespace in which pstl injects the algorithms. Then standard libraries can use something like std::__1, and you can use something like pstl::execution or whatever you want. I think we're in agreement here -- let me put this mechanism in place before removing this namespace injection.

we should have a way to customize the namespace in which pstl injects the algorithms

Actually, pstl the algorithms are defined in "std" namespace.
pstl injects just execution policy into "std".

I would like to pay your attention for changing like this in the tests

std::for_each_n(std::execution::seq, expected_first, n, Flip<T>(1));

Where a passed execution specifies an overloaded(by a policy type) implementation of std::for_each

and now the code is not correct in general I guess, because the test engine passes __pstl::execution::seg etc policies.

So, just for the test I suggest

std::for_each_n(__PSTL_POLICY_NAMESPACE::execution::seq, expected_first, n, Flip<T>(1));

to specify a proper overload in case of standalone PSTL version and there is a standard PSTL from a standard library delivered with a compiler.

Continuing the discussion - the code snippet of the test engine - pay your attention to using namespace __pstl::execution

invoke_on_all_policies(Op op, T&&... rest)
{
    using namespace __pstl::execution;

    // Try static execution policies
    invoke_on_all_iterator_types()(seq, op, std::forward<T>(rest)...);
    invoke_on_all_iterator_types()(unseq, op, std::forward<T>(rest)...);
#if _PSTL_USE_PAR_POLICIES
    invoke_on_all_iterator_types()(par, op, std::forward<T>(rest)...);
    invoke_on_all_iterator_types()(par_unseq, op, std::forward<T>(rest)...);
#endif
}
MikeDvorskiy requested changes to this revision.Apr 18 2019, 1:56 AM

Louis,

I have an issue regarding the dummy stdlib headers.
Proposed location is valid only for the test. (/test/support/stdlib)

The tests is not necessary part of standalone PSTL because is not a part of implementation of PSTL.
For example, If i have an application which use standalone pstl include "pstl/test/support/stdlib" would look strange at least.

So, I suggest moving the dummy stdlib headers into ..pstl\internal\stdlib or into ..pstl\stdlib

This revision now requires changes to proceed.Apr 18 2019, 1:56 AM

Louis,

I have an issue regarding the dummy stdlib headers.
Proposed location is valid only for the test. (/test/support/stdlib)

The tests is not necessary part of standalone PSTL because is not a part of implementation of PSTL.
For example, If i have an application which use standalone pstl include "pstl/test/support/stdlib" would look strange at least.

So, I suggest moving the dummy stdlib headers into ..pstl\internal\stdlib or into ..pstl\stdlib

I believe what's wrong here is the expectation that the PSTL is something that can be shipped as-is by implementations. The idea is that the PSTL provides the functionality but not necessarily the packaging for the parallel algorithms. Hence, Standard libraries have a little bit of boilerplate to include this functionality, and a "standalone" version of the PSTL should also wrap the internal headers to package the functionality however they want to.

In other words, I think the correct thing to do is for you guys to define <algorithm> & friends headers that #include <pstl/internal/XXX.h> while making sure that it works with the underlying standard library. I don't think the pstl should include headers that are useless to all but one specific shipping vehicle of the PSTL (the standalone version). That being said, if we realize that there's something useful to factor out for several shipping vehicles, of course we should do it. WDYT?

I don't think the pstl should include headers that are useless to all but one specific shipping vehicle of the PSTL (the standalone version)

Why cannot it be lie onto high "include\pstl" (or even "pstl") level of "LLVM upstream" as a special folder, which will not be taken into libcxx and libcstd++ libraries? (BTW, and as well the other extra functionality which is not going to be released with the next libcxx/libcstd release?, for e[ample alternative back-ends, experimental features, etc)
Just ignore it "extra" folders when the code are integrating into libcxx and libcstd repositories.
What do you think here?

I don't think the pstl should include headers that are useless to all but one specific shipping vehicle of the PSTL (the standalone version)

Why cannot it be lie onto high "include\pstl" (or even "pstl") level of "LLVM upstream" as a special folder, which will not be taken into libcxx and libcstd++ libraries? (BTW, and as well the other extra functionality which is not going to be released with the next libcxx/libcstd release?, for e[ample alternative back-ends, experimental features, etc)
Just ignore it "extra" folders when the code are integrating into libcxx and libcstd repositories.
What do you think here?

I don't think it should be necessary to subset the library when shipping it, and in fact I didn't plan on needing that for libc++. However, if all we're arguing about is the path for these headers, then we can put them into include/pstl/internal/test_stdlib or something similar. But let's avoid any name/path that suggests these headers are part of the public interface of the library, because they are not (that specific bit is the point I'm trying to make).

MikeDvorskiy added a comment.EditedApr 22 2019, 1:54 AM

But let's avoid any name/path that suggests these headers are part of the public interface of the library, because they are not

I'm absolutely agree.
And I suggest put these "dummy" standard headers on the rPSTL repo root level, where there are other folder which not be taken for the library, such as "doc", "cmake" and "test".
So, I suggest adding "stdlib\pstl" where "dummy" standard headers are placed, and "experimental" folder for future C++ features:
cmake
doc
include
stdlib\pstl
stdlib\experimental or just experimental
test

Arguments for "stdlib\pstl" folder:

Many people have already use standalone PSTL and a usage model is following:


Add the <install_dir>/include folder to the compiler include paths. You can do this by calling the pstlvars script.

Add #include "pstl/execution" to your code. Then add a subset of the following set of lines, depending on the algorithms you intend to use:

#include "pstl/algorithm"
#include "pstl/numeric"
#include "pstl/memory"


Arguments for "experimental" folder:
We would prefer keeping One main repository for PSTL further development (on LLVM site). And we are going to develop just features which already have had "Technical Specification" and to be going to added into one of the next C++ standard. Otherwise we have to support the second active development in the our own repo and "sync-up with LLVM repo" overheads will be very big in that case.

MikeDvorskiy added a comment.EditedApr 24 2019, 12:48 AM

Louis, Rodgers,

What do you think about my last proposal regarding

stdlib\pstl

and

stdlib\experimental

BTW, I've already had a patch which does that modification (moving to <rPSTL>\stdlib\pstl)

ldionne abandoned this revision.Jun 20 2019, 11:26 AM