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

Make internal algorithms functions const #4808

Conversation

msimberg
Copy link
Contributor

@msimberg msimberg commented Jul 7, 2020

HPX parallel algorithms wrap element-wise functions into bigger functions that operate on chunks. These functions are non-const. In the Kokkos executors I invoke those wrapping functions inside a Kokkos parallel region, and Kokkos expects element-wise functions to be const. This is a workaround that marks HPX functions like operator() for for_loop_iteration as const, and its members as mutable. As far as I can tell we can't make them properly const because of requirements in HPX itself, but I may be wrong here. Also as far as I can tell we don't really violate anything fundamental in Kokkos by doing this. Kokkos mostly requires functions to be const to discourage modifying e.g. members in a parallel region since that can be difficult to get right. Of course, this does feel very wrong, so I'm opening this up to see if someone would have suggestions on how to avoid this.

Note: I've only done this for for_loop, for_each, and transform as those are the only parallel algorithms that currently work with the Kokkos executors. Others will need specialization instead of going through our parallel algorithm machinery.

@msimberg msimberg added this to the 1.5.0 milestone Jul 7, 2020
Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to try and not make the members mutable (if you haven't tried that) to see what actually breaks. I think the main reason for not making those const in the first place was that the user supplied function objects might be mutable themselves.

@msimberg
Copy link
Contributor Author

msimberg commented Jul 7, 2020

I would like to try and not make the members mutable (if you haven't tried that) to see what actually breaks. I think the main reason for not making those const in the first place was that the user supplied function objects might be mutable themselves.

Yep, I'll have a closer look at what exactly needs to be non-const. The user-supplied functions will be a problem in the end though. I'd imagine we don't actually want to require them to be const?

@hkaiser
Copy link
Member

hkaiser commented Jul 7, 2020

The user-supplied functions will be a problem in the end though. I'd imagine we don't actually want to require them to be const?

Those are specifically allowed to be non-const, think of a function object that counts odd numbers using a for_each for instance...

@msimberg
Copy link
Contributor Author

msimberg commented Jul 7, 2020

The user-supplied functions will be a problem in the end though. I'd imagine we don't actually want to require them to be const?

Those are specifically allowed to be non-const, think of a function object that counts odd numbers using a for_each for instance...

Yep, that's that I thought.

Kokkos expects functions in parallel regions to be const. This satisfies
Kokkos' requirements when calling HPX's algorithm functions in Kokkos parallel
regions.
@msimberg msimberg force-pushed the parallel-algorithms-kokkos-const branch from eb54e1b to 21ee2dd Compare July 10, 2020 08:44
@msimberg
Copy link
Contributor Author

@hkaiser here's a log of building transform_test as an example: https://gist.github.com/msimberg/172506305a9a3df6aa777cef34800ab3. This is with the current revision on this branch (i.e. 21ee2dd). Making the operator()s here:

struct add_one
{
template <typename T>
T operator()(T const& v)
{
return v + 1;
}
};
struct throw_always
{
template <typename T>
T operator()(T)
{
throw std::runtime_error("test");
}
};
const makes at least that particular test compile. Given that the user supplied are allowed to be non-const there are a few options of which only the two last ones seem reasonable:

  • add some mutable
  • I've missed some other place in the algorithms that should be changed that would make this compile
  • specialize algorithms for the Kokkos executors (where we would require user-supplied functions to be const)
    Given our discussions outside of here, the last one seems like the best option in the end. It'll anyway be required for most algorithms.

@G-071 this branch is what you need right now as a workaround to have the Kokkos executors work with parallel algorithms.

@msimberg msimberg closed this Jul 10, 2020
@hkaiser
Copy link
Member

hkaiser commented Jul 10, 2020

@msimberg let's specialize the algorithms for Kokkos, I think this will result in the cleanest solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants