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

Fix computeapi unit tests #2569

Merged
merged 9 commits into from Mar 30, 2017

Conversation

Projects
None yet
2 participants
@atrantan
Copy link
Contributor

commented Mar 29, 2017

This patch tries to fix some compilation errors that come from both nvcc and cuda-clang for computeapi unit tests

@atrantan atrantan changed the title Fix tests unit computeapi Fix computeapi unit tests Mar 29, 2017

@hkaiser hkaiser added this to the 1.0.0 milestone Mar 29, 2017

{
HPX_ASSERT(false);
return nullptr;
}

This comment has been minimized.

Copy link
@hkaiser

hkaiser Mar 30, 2017

Member

Please fix the indentation for this code.

This comment has been minimized.

Copy link
@atrantan

atrantan Mar 30, 2017

Author Contributor

Done in 177d1bc


#if defined(__CUDA_ARCH__)
HPX_DEVICE operator T() const
HPX_HOST_DEVICE operator T() const
{
return access_target::read(target_, p_);
}

This comment has been minimized.

Copy link
@hkaiser

hkaiser Mar 30, 2017

Member

Why should access_target::read() ever be invoked on the device?

This comment has been minimized.

Copy link
@atrantan

atrantan Mar 30, 2017

Author Contributor

Was thinking on that ... The reason was the way traits::access_target<> has been designed in https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/compute/cuda/traits/access_target.hpp

This comment has been minimized.

Copy link
@hkaiser

hkaiser Mar 30, 2017

Member

Ahh, ok. You're right. Thanks!

typedef typename compute::vector<T, Allocator>::value_type
value_type;
for(value_type const& v : vs)
for(auto v : vs)

This comment has been minimized.

Copy link
@hkaiser

hkaiser Mar 30, 2017

Member

This should be auto const& v : vs

This comment has been minimized.

Copy link
@atrantan

atrantan Mar 30, 2017

Author Contributor

Done in 1e8fec3


}}


This comment has been minimized.

Copy link
@hkaiser

hkaiser Mar 30, 2017

Member

Please put this into the same namespace as the other helper predicates.

This comment has been minimized.

Copy link
@atrantan

atrantan Mar 30, 2017

Author Contributor

Done in 177d1bc

std::advance(iter, offset);
#endif

This comment has been minimized.

Copy link
@hkaiser

hkaiser Mar 30, 2017

Member

Can't we get rid of the std::advance altogether? Your (new) implementation should do just fine, shouldn't it?

This comment has been minimized.

Copy link
@atrantan

atrantan Mar 30, 2017

Author Contributor

Not sure about which kind of optimizations vendor compilers are doing with std::advance. I suggest to let it this way to preserve the host side advance

This comment has been minimized.

Copy link
@hkaiser

namespace hpx { namespace detail
{
template<class InputIterator, class Distance>

This comment has been minimized.

Copy link
@hkaiser

hkaiser Mar 30, 2017

Member

We normally use typename for template parameter types (instead of class).

This comment has been minimized.

Copy link
@atrantan

atrantan Mar 30, 2017

Author Contributor

Done in 1e8fec3

std::bidirectional_iterator_tag)
{
if (n < 0) {
while (n++) --i;

This comment has been minimized.

Copy link
@hkaiser

hkaiser Mar 30, 2017

Member

Please fix the indentation here.

This comment has been minimized.

Copy link
@atrantan

atrantan Mar 30, 2017

Author Contributor

Done in 177d1bc

@@ -35,7 +35,7 @@ void test_for_each(executor_type& exec, target_vector& d_A)
hpx::parallel::for_each(
hpx::parallel::execution::par.on(exec),
d_A.begin(), d_A.end(),
[] HPX_DEVICE (int & i)
[] HPX_HOST_DEVICE (int & i)

This comment has been minimized.

Copy link
@hkaiser

hkaiser Mar 30, 2017

Member

Please add a comment, why this has to be __host__ __device__.

This comment has been minimized.

Copy link
@atrantan

atrantan Mar 30, 2017

Author Contributor

Done in 177d1bc

@@ -25,17 +25,24 @@ typedef hpx::compute::cuda::default_executor executor_type;
typedef hpx::compute::cuda::allocator<int> target_allocator;
typedef hpx::compute::vector<int, target_allocator> target_vector;

struct transform_test
{
template< typename T >

This comment has been minimized.

Copy link
@hkaiser

hkaiser Mar 30, 2017

Member

Please align your white-space placement to the surrounding code.

This comment has been minimized.

Copy link
@atrantan

atrantan Mar 30, 2017

Author Contributor

Done in 177d1bc

struct transform_test
{
template< typename T >
HPX_HOST_DEVICE int operator()(T A, T B)

This comment has been minimized.

Copy link
@hkaiser

hkaiser Mar 30, 2017

Member

Please don't use capital letters for something which is not-a-macro and not-a-template-parameter. Also, should the arguments be const&?

This comment has been minimized.

Copy link
@atrantan

atrantan Mar 30, 2017

Author Contributor

Done in 177d1bc

atrantan added some commits Mar 30, 2017

atrantan
atrantan

@hkaiser hkaiser merged commit 7ea2b5b into STEllAR-GROUP:master Mar 30, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@atrantan atrantan deleted the atrantan:fix_tests_unit_computeapi branch Mar 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.