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

Prepare for deprecation of ViewAllocateWithoutInitializing #3264

Merged
merged 9 commits into from
Aug 19, 2020

Conversation

dalg24
Copy link
Member

@dalg24 dalg24 commented Aug 8, 2020

Per comment in the code, ViewAllocateWithoutInitializing was there only for backward compatibility. Moving away from it allows us to get rid of two View constructor overloads*.
Admittedly the changes I propose would break code that relied on Kokkos::ViewAllocateWithoutInitializing being an actual type but I think it is ok.

I considered adding an implicit conversion from ViewAllocateWithoutInitializing to Impl::ViewCtorProp<std::string,Impl::WithoutInitializing_t> because that solution would be safer for code downstream. In fact, it was originally my plan but it turned out to be harder than I thought. I managed to implement it with the help from @nliber but it forced me to change all the overloads that took Impl::ViewCtorProp<P...> and I ended dropping the idea because the complexity added was not worth it.

You will notice I haven't actually yet deprecated the use of ViewAllocateWithoutInitializing. My intent is to make sure we remove it from all examples and tutorials and perform the deprecation later. Hopefully by then we will be able to use the deprecated attribute from C++14.

TLDR Make ViewAllocateWithoutInitializing a function that generates a Impl::ViewCtorProp rather a concrete type an alias in preparation for deprecation/removal.

*Don't worry we still have 14 besides besides the special members!

@dalg24 dalg24 added this to the Tentative 3.3 Release milestone Aug 8, 2020
@dalg24
Copy link
Member Author

dalg24 commented Aug 8, 2020

I had a peek at Trilinos and this PR would break

packages/muelu/test/scaling/MMKernelDriver.cpp:282:    using no_init_view=Kokkos::ViewAllocateWithoutInitializing;
packages/muelu/test/scaling/MMKernelDriver.cpp:442:    using no_init_view=Kokkos::ViewAllocateWithoutInitializing;
packages/muelu/test/scaling/MMKernelDriver.cpp:689:        using no_init_view=Kokkos::ViewAllocateWithoutInitializing;
packages/muelu/test/scaling/MMKernelDriver.cpp:842:    using no_init_view=Kokkos::ViewAllocateWithoutInitializing;
packages/shylu/shylu_node/tacho/src/impl/Tacho_Util.hpp:531:  using do_not_initialize_tag = Kokkos::ViewAllocateWithoutInitializing;

one possible fix would be Kokkos::ViewAllocateWithoutInitializing => decltype(Kokkos::view_alloc(Kokkos::WithoutInitializing, "whatever")) actually that would not work, but a lambda would [](std::string label){return Kokkos::view_alloc(Kokkos::WithoutInitializing, label);}.

@dalg24
Copy link
Member Author

dalg24 commented Aug 11, 2020

@crtrott encouraged me to come up with a solution that does not break code. So here it is.

Make sure to drop 730adc2 before we merge. It is only there to demonstrate that the solution I propose works.

Copy link
Contributor

@DavidPoliakoff DavidPoliakoff left a comment

Choose a reason for hiding this comment

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

Looks good to me, though like Damien says we should drop the named commit when we merge

@dalg24
Copy link
Member Author

dalg24 commented Aug 11, 2020

Build before dropping the commit log is here

I added a unit test anyway.

@nliber
Copy link
Contributor

nliber commented Aug 19, 2020

Thoroughly looked through the changes. Looks good to me.

@dalg24 dalg24 merged commit 3ab63b3 into kokkos:develop Aug 19, 2020
@dalg24 dalg24 deleted the view_alloc_without_initializing branch August 19, 2020 21:48
ndellingwood added a commit to ndellingwood/Trilinos that referenced this pull request Oct 22, 2020
Beginning with kokkos/3.3, ViewAllocateWithoutInitializing becomes an
alias to a specialization of ViewCtorProp taking a string as ctor
argument, kokkos/kokkos#3264. These changes result in removal of the
"label" member of the former ViewAllocateWithoutInitializing struct

stokhos/src/sacado/kokkos/pce/KokkosExp_View_UQ_PCE_Contiguous.hpp
contains a make_view overload that required the label member from the
ViewAllocateWithoutInitializing struct; this PR makes a change
for updated compatibility
ndellingwood added a commit to ndellingwood/Trilinos that referenced this pull request Nov 24, 2020
Beginning with kokkos/3.3, ViewAllocateWithoutInitializing becomes an
alias to a specialization of ViewCtorProp taking a string as ctor
argument, kokkos/kokkos#3264. These changes result in removal of the
"label" member of the former ViewAllocateWithoutInitializing struct

stokhos/src/sacado/kokkos/pce/KokkosExp_View_UQ_PCE_Contiguous.hpp
contains a make_view overload that required the label member from the
ViewAllocateWithoutInitializing struct; this PR makes a change
for updated compatibility
ndellingwood added a commit to trilinos/Trilinos that referenced this pull request Dec 3, 2020
Beginning with kokkos/3.3, ViewAllocateWithoutInitializing becomes an
alias to a specialization of ViewCtorProp taking a string as ctor
argument, kokkos/kokkos#3264. These changes result in removal of the
"label" member of the former ViewAllocateWithoutInitializing struct

stokhos/src/sacado/kokkos/pce/KokkosExp_View_UQ_PCE_Contiguous.hpp
contains a make_view overload that required the label member from the
ViewAllocateWithoutInitializing struct; this PR makes a change
for updated compatibility
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

3 participants