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

Leverage CTAD with [MD]RangePolicy #1141

Merged
merged 2 commits into from
Oct 1, 2024

Conversation

aprokop
Copy link
Contributor

@aprokop aprokop commented Sep 4, 2024

No description provided.

@aprokop aprokop added the refactoring Code reorganization label Sep 4, 2024
@dalg24 dalg24 changed the title Remove execution space template from {MD}RangePolicy Remove execution space template from [MD]RangePolicy Sep 4, 2024
@dalg24 dalg24 changed the title Remove execution space template from [MD]RangePolicy Leverage CTAD with [MD]RangePolicy Sep 4, 2024
examples/molecular_dynamics/example_molecular_dynamics.cpp Outdated Show resolved Hide resolved
examples/raytracing/example_raytracing.cpp Outdated Show resolved Hide resolved
examples/raytracing/example_raytracing.cpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsMinimumSpanningTree.hpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsTreeTraversal.hpp Outdated Show resolved Hide resolved
src/details/ArborX_DetailsTreeTraversal.hpp Outdated Show resolved Hide resolved
@aprokop
Copy link
Contributor Author

aprokop commented Sep 4, 2024

I may indeed misunderstand some things. I do wonder why it compiled everywhere CPU, though, and not universally failed.

@dalg24
Copy link
Contributor

dalg24 commented Sep 4, 2024

I may indeed misunderstand some things. I do wonder why it compiled everywhere CPU, though, and not universally failed.

Because only one execution space was enabled and these types were equivalent/compatible.

test/tstDetailsDistributor.cpp Outdated Show resolved Hide resolved
test/tstDetailsDistributor.cpp Outdated Show resolved Hide resolved
test/tstDetailsTreeConstruction.cpp Outdated Show resolved Hide resolved
@dalg24
Copy link
Contributor

dalg24 commented Sep 4, 2024

I stopped and didn't actually go through the rest of the changes. I expect you will find other occurrences if any

@aprokop
Copy link
Contributor Author

aprokop commented Sep 4, 2024

I stopped and didn't actually go through the rest of the changes. I expect you will find other occurrences if any

Thank you. I used regex and should have been much more careful about the changes. I'm sorry.

@aprokop
Copy link
Contributor Author

aprokop commented Sep 5, 2024

CUDA-11.1.1-NVCC build failed. Per usual, CTAD in CUDA versions pre 11.4 is really spotty. As mentioned in #974, it would make sense for us to update the CUDA-11.1 build, as it has been holding us back. Given that we have a CUDA-11.5.2-NVCC-CUDA-AWARE-MPI, I would propose switching 11.1 build to either 11.7 or 12.x.

SYCL build did not run.

Update: this reproducer fails on CUDA-11.1 with Kokkos 4.3:

#include <Kokkos_Core.hpp>

template <typename ExecutionSpace, typename ViewType>
void iota(ExecutionSpace const &space, ViewType const &v,
          typename ViewType::value_type value = 0)
{
  Kokkos::parallel_for(
      "ArborX::Algorithms::iota", Kokkos::RangePolicy(space, 0, v.extent(0)),
      KOKKOS_LAMBDA(int i) { v(i) = value + i; });
}

int main(int argc, char *argv[])
{
  Kokkos::ScopeGuard guard(argc, argv);

  Kokkos::View<int*> v("view", 10);
  iota(Kokkos::DefaultExecutionSpace{}, v);

  return EXIT_SUCCESS;
}

The failure:

<snip>/arborx/benchmarks/develop/develop.cpp: In instantiation of 'void iota(const ExecutionSpace&, const ViewType&, typename ViewType::value_type) [with ExecutionSpace = Kokkos::Cuda; ViewType = Kokkos::View<int*>; typename ViewType::value_type = int]':
<snip>/arborx/benchmarks/develop/develop.cpp:29:40:   required from here
<snip>/arborx/benchmarks/develop/develop.cpp:19:61: error: 'RangePolicy' was not declared in this scope; did you mean 'Kokkos::RangePolicy'?
   19 |   Kokkos::parallel_for(
      |                                                             ^
      |                                                  Kokkos::RangePolicy
/home/users/aprokop/local/opt/saturn/kokkos-4.3.1-cuda11.1/include/Kokkos_ExecPolicy.hpp:68:7: note: 'Kokkos::RangePolicy' declared here
   68 | class RangePolicy : public Impl::PolicyTraits<Properties...> {

It passes with added <ExecutionSpace> template to RangePolicy. It also passes with CUDA 11.5. So, some of the CTAD is definitely broken on CUDA 11.1 and probably others.

@aprokop aprokop marked this pull request as draft September 25, 2024 15:20
@aprokop
Copy link
Contributor Author

aprokop commented Oct 1, 2024

Rebased.

@aprokop aprokop marked this pull request as ready for review October 1, 2024 18:55
@aprokop aprokop merged commit 22f06fd into arborx:master Oct 1, 2024
2 checks passed
@aprokop aprokop deleted the kokkos_4.3_rangepolicy branch October 1, 2024 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code reorganization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants