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.

Sign up for GitHub

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

Jump to bottom

Avoid instantiating default constructor of value type when WithoutInitializing is given #5307

Merged
merged 6 commits into from
Aug 17, 2022
Merged

Avoid instantiating default constructor of value type when WithoutInitializing is given #5307

merged 6 commits into from
Aug 17, 2022

Conversation

masterleinad
Copy link
Contributor

Trying #2024 on top of #5277. Fixes #1873.

@masterleinad masterleinad force-pushed the avoid_default_ctor_withoutinitializing branch from 5700bca to 37af24c Compare August 3, 2022 17:42
@PhilMiller PhilMiller linked an issue Aug 3, 2022 that may be closed by this pull request
Support creation of managed views of classes without a device default constructor when asking for no-initialization #1873
Closed
@masterleinad masterleinad force-pushed the avoid_default_ctor_withoutinitializing branch from e79d08c to f44063b Compare August 4, 2022 15:10
@masterleinad masterleinad force-pushed the avoid_default_ctor_withoutinitializing branch from a48dcc8 to 7515804 Compare August 4, 2022 16:35
masterleinad
masterleinad commented Aug 4, 2022
View reviewed changes
core/src/impl/Kokkos_ViewMapping.hpp
Comment on lines +3475 to +3479
if (false) {
// Make sure the destroy functor gets instantiated.
// This avoids "cudaErrorInvalidDeviceFunction"-type errors.
functor.destroy_shared_allocation();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the critical change that we missed in the previous attempts.

@masterleinad masterleinad marked this pull request as ready for review August 4, 2022 16:37
PhilMiller
PhilMiller reviewed Aug 4, 2022
View reviewed changes
core/src/impl/Kokkos_ViewMapping.hpp
if (false) {
// Make sure the destroy functor gets instantiated.
// This avoids "cudaErrorInvalidDeviceFunction"-type errors.
functor.destroy_shared_allocation();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not going to generate warnings about unreachable code from one compiler or another? How about (void)&functor_type::destroy_shared_allocation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would try to deal with it when we run into problems. At least our CI seems to be fine with it.

dalg24
dalg24 reviewed Aug 5, 2022
View reviewed changes
Copy link
Member

@dalg24 dalg24 left a comment

Choose a reason for hiding this comment

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

I am glad you figured that one out

core/unit_test/TestWithoutInitializing.hpp Outdated Show resolved Hide resolved
core/unit_test/TestWithoutInitializing.hpp Outdated Show resolved Hide resolved
core/src/impl/Kokkos_ViewMapping.hpp Show resolved Hide resolved
@masterleinad @dalg24
Apply suggestions from code review
2a9ef31 
Co-authored-by: Damien L-G <dalg24+github@gmail.com>
@masterleinad masterleinad force-pushed the avoid_default_ctor_withoutinitializing branch from 5c2cf3f to 2a9ef31 Compare August 5, 2022 14:06
@masterleinad
Copy link
Contributor Author

Retest this please.

crtrott
crtrott requested changes Aug 9, 2022
View reviewed changes
Copy link
Member

@crtrott crtrott left a comment

Choose a reason for hiding this comment

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

Just guard that thing in the ViewArray version too then I approve. The clang stuff we should look at independently.

core/src/impl/Kokkos_ViewMapping.hpp Outdated

KOKKOS_INLINE_FUNCTION void operator()(DestroyTag const&,
const size_t i) const {
// KOKKOS_IMPL_CUDA_CLANG_WORKAROUND this line causes ptax error
Copy link
Member

Choose a reason for hiding this comment

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

does this comment still has any meaning? Not holding this up because of it but would be good to figure out. I also see that one of hte tests is commented out for HIP too ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can see, the test is also enabled for Cuda+Clang so I removed the comment.

@masterleinad
Copy link
Contributor Author

Just guard that thing in the ViewArray version too then I approve. The clang stuff we should look at independently.

See 3f3bbc2.

@masterleinad masterleinad force-pushed the avoid_default_ctor_withoutinitializing branch from b6da0a9 to 8268c40 Compare August 10, 2022 12:51
@masterleinad
Copy link
Contributor Author

Retest this please.

@masterleinad
Copy link
Contributor Author

Only KokkosCore_UnitTest_CudaTimingBased is failing in CUDA-11.6-NVHPC.

@crtrott crtrott merged commit b51b5e7 into kokkos:develop Aug 17, 2022
@masterleinad masterleinad mentioned this pull request Sep 7, 2022
Closed
@ldh4 ldh4 mentioned this pull request Oct 28, 2022
Merged
@masterleinad masterleinad deleted the avoid_default_ctor_withoutinitializing branch January 18, 2023 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhilMiller PhilMiller PhilMiller left review comments

@dalg24 dalg24 dalg24 approved these changes

@crtrott crtrott crtrott approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@masterleinad @PhilMiller @dalg24 @crtrott