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

Enforce create_mirror restrictions w.r.t. ViewCtorArgs on all variants #6304

Merged

Conversation

masterleinad
Copy link
Contributor

In reference to https://kokkosteam.slack.com/archives/G5CBLMFLP/p1689961392307099?thread_ts=1689959585.967619&cid=G5CBLMFLP. We should make sure that we impose the same restrictions on View constructor arguments even if we only return the source View in create_mirror_view. create_mirror_view_and_copy is consistent already.

@fnrizzi
Copy link
Contributor

fnrizzi commented Jul 25, 2023

can you please copy/paste the slack text? so that it is easier to access/read for anybody?

@fnrizzi
Copy link
Contributor

fnrizzi commented Jul 25, 2023

@masterleinad what do mean "even if we only return the source View" ?

@masterleinad
Copy link
Contributor Author

can you please copy/paste the slack text? so that it is easier to access/read for anybody?

We saw/discussed that

 using MemorySpace = ...;
  auto pif          = 3.14f;
  auto v = Kokkos::create_mirror_view(
      Kokkos::view_alloc("pi", MemorySpace()),
      Kokkos::View<float, Kokkos::HostSpace, Kokkos::MemoryUnmanaged>(&pif));
  ASSERT_TRUE(v.label().empty() || v.label() == "pi");

might compile and run or fail at compile-time depending on MemorySpace.

@masterleinad
Copy link
Contributor Author

@masterleinad what do mean "even if we only return the source View" ?

If create_mirror_view doesn't create a new object we still want to enforce the same restrictions.

@masterleinad masterleinad force-pushed the improve_static_asserts_create_mirror branch from 1f3402c to 3a255d1 Compare July 25, 2023 12:38
@crtrott
Copy link
Member

crtrott commented Aug 3, 2023

All of the NVCC builds failed for unrelated reasons, gonna restart this.

@crtrott
Copy link
Member

crtrott commented Aug 3, 2023

Retest this please

@crtrott
Copy link
Member

crtrott commented Aug 4, 2023

This time everything passed except the NVHPC run which failed with the OpCode thing from the team algos, and also passed last time around. Also it confirmed again that the OpCode thing seems deterministic on waffle, happened again there.

@crtrott crtrott merged commit 0ebedee into kokkos:develop Aug 4, 2023
27 of 28 checks passed
@crtrott crtrott mentioned this pull request Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants