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

Some other fixes around cuda examples #2501

Merged
merged 8 commits into from Mar 20, 2017
Merged

Conversation

atrantan
Copy link

@atrantan atrantan commented Feb 14, 2017

This patch tries to:

  • Solve some erroneous runtime behaviours when components are used in Cuda code
  • Revert commit 11e29e0 because of FindCuda.cmake constraints

{
return p_;
}

T* device_ptr() const HPX_NOEXCEPT
Copy link
Member

Choose a reason for hiding this comment

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

Should we add an overload for std::addressof() as well?

Copy link
Author

Choose a reason for hiding this comment

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

Nope. Even if yes, the signature of std::adressof() (return type being T* and argument being T&) does not make things easy

@@ -160,10 +160,10 @@ namespace hpx { namespace components { namespace server

/// \brief Actions to create new objects
template <typename Component>
HPX_HOST_DEVICE naming::gid_type create_component();
naming::gid_type create_component();

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this make the HPX_HOST_DEVICE conditional for NVCC only instead?

Copy link
Author

@atrantan atrantan Feb 14, 2017

Choose a reason for hiding this comment

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

@hkaiser Possible but need to do some cleaner workaround (in terms of host / device) around constructors/destructors/member functions of gid_type https://github.com/atrantan/hpx/blob/master/hpx/runtime/naming/name.hpp I may suggest to do it in a future work.

Copy link
Member

Choose a reason for hiding this comment

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

I would like to avoid breaking NVCC builds just to fix the clang builds. Let's try to keep NVCC in working state as long as possible...

Copy link
Author

Choose a reason for hiding this comment

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

It doesn't break NVCC builds that was working before. I may be wrong but only the partitioned_vector.cu was using components and for the moment it has always fail at compilation.

CMakeLists.txt Outdated
@@ -969,6 +969,7 @@ endif()
# CUDA features
################################################################################
if(HPX_WITH_CUDA AND NOT HPX_WITH_CUDA_CLANG)
hpx_add_target_compile_option(-DHPX_WITH_CUDA)
Copy link
Member

Choose a reason for hiding this comment

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

This is wrong. The actual problem lies here: https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/config/compiler_specific.hpp#L75 which should test for HPX_HAVE_CUDA instead.

Copy link
Author

Choose a reason for hiding this comment

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

See commit 80277b9 that has been merged recently on master.

Copy link
Member

Choose a reason for hiding this comment

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

I know, that's a bug on master.

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.

That LGTM now, thanks a lot!

@atrantan
Copy link
Author

Got the initial issue both with nvcc and clang because hpx_add_config_define(HPX_HAVE_CUDA) in https://github.com/STEllAR-GROUP/hpx/blob/master/CMakeLists.txt at line 248 seem to not touch flags associated to source files with .cu extension

@hkaiser
Copy link
Member

hkaiser commented Mar 19, 2017

HPX_HAVE_CUDA should end up in <hpx/config/defines.hpp>, so it should get picked up even by .cu files.

@hkaiser hkaiser merged commit 682dbeb into STEllAR-GROUP:master Mar 20, 2017
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.

None yet

2 participants