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

Custom init() function for parallel_reduce with array value_type #210

Closed
etphipp opened this issue Mar 14, 2016 · 12 comments
Closed

Custom init() function for parallel_reduce with array value_type #210

etphipp opened this issue Mar 14, 2016 · 12 comments
Assignees
Labels
Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos)

Comments

@etphipp
Copy link
Contributor

etphipp commented Mar 14, 2016

I have a functor that does reduction on an array that requires a non-trivial init() function, however I am finding that my functor's init function is not getting called. Here are the relevant parts of the declaration:

struct MaxMinFunctor {
  typedef double value_type[];

   KOKKOS_INLINE_FUNCTION
   void init (value_type dst) const { ... }

By adding print and assert statements I can verify init() is not getting called. Looking at the Kokkos source code, I see this:

template< class FunctorType , class ArgTag >
struct FunctorValueTraits< FunctorType , ArgTag , true /* == exists FunctorType::value_type */ >
{
  typedef typename Impl::remove_extent< typename FunctorType::value_type >::type  value_type ;

  // If not an array then what is the sizeof(value_type)
  enum { StaticValueSize = Impl::is_array< typename FunctorType::value_type >::value ? 0 : sizeof(value_type) };

  typedef value_type                 * pointer_type ;

  // The reference_type for an array is 'value_type *'
  // The reference_type for a single value is 'value_type &'

  typedef typename Impl::if_c< ! StaticValueSize , value_type *
                                                 , value_type & >::type  reference_type ;

and this

// Function signatures for FunctorType::init function without a tag and is an array
template< class FunctorType >
struct FunctorValueInitFunction< FunctorType , void , true > {

  typedef typename FunctorValueTraits<FunctorType,void>::reference_type value_type ;

  KOKKOS_INLINE_FUNCTION static void enable_if( void (FunctorType::*)( value_type * ) const );

Shouldn't that be:

KOKKOS_INLINE_FUNCTION static void enable_if( void (FunctorType::*)( value_type ) const );

given that reference_type for an array is value_type *?

If I add an empty overload of "void init(double**) const {}" to my functor it works and the correct init function is called. This seems like a bug, or am I just doing something stupid?

@crtrott
Copy link
Member

crtrott commented Mar 14, 2016

Can you try init(value_type& dat), I believe we expect a reference to the value_type.

@hcedwar
Copy link
Contributor

hcedwar commented Mar 14, 2016

The value_type is a dynamic length array, so appears to be a bug in the 'is there an init function' introspection.

@hcedwar hcedwar added the Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos) label Mar 14, 2016
@etphipp
Copy link
Contributor Author

etphipp commented Mar 14, 2016

Changing it to a reference doesn't work, and in fact introduces a compiler error:

/home/etphipp/Trilinos/Trilinos/packages/trilinoscouplings/examples/fenl/SampleGrouping.hpp:260:33: error: parameter �dst� includes reference to array of unknown bound �Kokkos::Example::FENL::MaxAnisotropyGrouping<Scalar, MeshType, CoeffFunctionType>::MaxMinFunctor::value_type�
void init (value_type& dst) const

@etphipp
Copy link
Contributor Author

etphipp commented Mar 14, 2016

My belief is the introspection is wrong and it should be

KOKKOS_INLINE_FUNCTION static void enable_if( void (FunctorType::*)( value_type ) const );

instead of

KOKKOS_INLINE_FUNCTION static void enable_if( void (FunctorType::*)( value_type* ) const );

since value_type is already a pointer.

@crtrott
Copy link
Member

crtrott commented Mar 14, 2016

Not so sure. It looks like we need to test for both. If your value_type is a "double" then the signature is
value_type& not value_type for the init thing. Probably we need to do some clever thing about testing whether the value_type is a actual scalar type or an array ....

@crtrott
Copy link
Member

crtrott commented Mar 14, 2016

Carter can you do this?

@etphipp
Copy link
Contributor Author

etphipp commented Mar 14, 2016

Btw, changing it to a static-length array doesn't make it work either.

@crtrott
Copy link
Member

crtrott commented Mar 14, 2016

Yeah I wouldn't expect it too.

@hcedwar hcedwar self-assigned this Mar 14, 2016
@etphipp
Copy link
Contributor Author

etphipp commented Mar 14, 2016

You actually do all of that stuff already. My point was the test for when value_type is an array is wrong. The separate test for when value_type is not an array is correct.

@crtrott
Copy link
Member

crtrott commented Mar 14, 2016

Oh ok. So it should be easy to fix 👍

@etphipp
Copy link
Contributor Author

etphipp commented Mar 14, 2016

Yes, I think I told you how to fix it 😃

@crtrott
Copy link
Member

crtrott commented Mar 16, 2016

Pushed to master, closing the issue

@crtrott crtrott closed this as completed Mar 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos)
Projects
None yet
Development

No branches or pull requests

3 participants