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

Fix/cuda shuffle changes #108

Merged
merged 3 commits into from
Aug 2, 2019
Merged

Fix/cuda shuffle changes #108

merged 3 commits into from
Aug 2, 2019

Conversation

griwodz
Copy link
Member

@griwodz griwodz commented Jul 31, 2019

Intrinsic functions __shfl_* as well as _any, _all and _ballot were replaced by more flexible _sync and _async version with CUDA 9. Those make sense with more recent CCs that allow the use of these functions even if some threads of a warp are already dead. We support older CCs as well and cannot rely on such features.

Importing a set of macros from PopSift that hide this difference between CUDA SDKs <9 and >=9.

@griwodz
Copy link
Member Author

griwodz commented Jul 31, 2019

I fetched these changes from PopSift while working on the update_cudaCub branch because I couldn't find any real errors in all the warnings. Made this separate PR because it's not really related to Cub.

@griwodz griwodz self-assigned this Jul 31, 2019
@simogasp simogasp added this to the v1.0.0 milestone Jul 31, 2019
Copy link
Member

@simogasp simogasp left a comment

Choose a reason for hiding this comment

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

Hi @griwodz ,

please also add this in ./cmake/config.hpp.in towards the end (line 30)

#ifdef CCTAG_WITH_CUDA

   #define CUB_CDP

--> to add
    #ifndef CCTAG_HAVE_SHFL_DOWN_SYNC
    #cmakedefine CCTAG_HAVE_SHFL_DOWN_SYNC
    #endif
-->
#endif

so that the definition is also up-to-date in the generated config.hpp

CMakeLists.txt Outdated Show resolved Hide resolved
@@ -171,6 +171,9 @@ endif(CCTAG_NO_COUT)
if(CCTAG_VISUAL_DEBUG)
target_compile_definitions(CCTag PRIVATE "-DCCTAG_VISUAL_DEBUG")
endif(CCTAG_VISUAL_DEBUG)
if(HAVE_SHFL_DOWN_SYNC)
target_compile_definitions(CCTag PRIVATE "-DCCTAG_HAVE_SHFL_DOWN_SYNC")
Copy link
Member

Choose a reason for hiding this comment

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

this should go only in the case if CCTAG_WITH_CUDA is ON, as it can be only active on cuda code, am I right?
If so, it should be move up at line 116

  target_compile_definitions(CCTag
                         PUBLIC -DCCTAG_WITH_CUDA -DCUB_CDP
                         PRIVATE ${TBB_DEFINITIONS})

  if(CCTAG_HAVE_SHFL_DOWN_SYNC)
    target_compile_definitions(CCTag PRIVATE "-DCCTAG_HAVE_SHFL_DOWN_SYNC")
  endif()

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. The generated cctag_config.hpp has so far been ignored in the CCTag code. I understand that it's relevant for distributing CCTag but isn't it reasonable to use the same file inside CCTag as well as soon as we start using it?

Assuming that the answer is yes, I've added an include_directories() so that it is actually visible during compilation. But of course, when we start using cctag_config, then it's redundant to have target_compile_definitions as well.

Copy link
Member

@simogasp simogasp Aug 1, 2019

Choose a reason for hiding this comment

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

yes the cctag_config.hpp is ignored in the code because everything is managed by cmake (it propagates the definitions even when the library is used as third pary like in AliceVision).
But nevertheless, it is better to have for people that may not use cmake as building system.
At least they can include that before any other cctag header.
So I agree that it is not much useful in our configuration but it's better to have in the installed version.
For this reason please remove the include_directories() because we don't need that to be visible when building with cmake. FYI, the correct way to include that would be using

target_include_directories(<target> PUBLIC             
            $<BUILD_INTERFACE:${generated_dir}>  #this will include the generated dir *only* for in-tree building
            $<INSTALL_INTERFACE:include/> #this will include the the path where config.hpp is installed for using as third party

Copy link
Member

Choose a reason for hiding this comment

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

BTW, the same holds for the version.hpp file, it is installed but never used, at least for the moment. In case we will need it in the code in the future then, again, we have to add that target_include_directories piece of code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should start using our own generated config files at some point. The CXXFLAGS can in projects using CCTags can be manipulated as shown here: cmake-remove-a-compile-flag-for-a-single-translation-unit.
I'll try to remember that no-so-simple target_include_dir statement from above.

@griwodz
Copy link
Member Author

griwodz commented Aug 1, 2019

I have a question:
cmake expects that a developer uses
#cmakedefine SOMETHING
and generates one of the following lines:
#define SOMETHING or /* #undef SOMETHING */

Is there any reason why our config.hpp.in file uses

#ifndef CCTAG_WITH_CUDA
#cmakedefine CCTAG_WITH_CUDA
#endif

?
I don't think that it's a good idea to allow users of the CCTag library to override these defines if/when this can activate branches in installed CCTag header files that are inconsistent with the actual code in the installed CCTag library. Or is it something that is required by an IDE?

@simogasp
Copy link
Member

simogasp commented Aug 1, 2019

Yes, I agree it's a little be too overdoing, I don't remember the rationale behind it. I think it was a sort of safeguard to avoid redefining the macro in the same spirit as the define safeguards of the headers. It's embarrassing as it was me that did that and I forgot why... I must have been delirious...

@griwodz
Copy link
Member Author

griwodz commented Aug 2, 2019

I undid the last 2 commits and followed your original recommended changes.

Copy link
Member

@simogasp simogasp left a comment

Choose a reason for hiding this comment

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

thanks!

@simogasp simogasp merged commit 5f3f7fa into develop Aug 2, 2019
@simogasp simogasp deleted the fix/cudaShuffleChanges branch May 30, 2020 14:42
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.

2 participants