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

Generate separate objects for cu and nvrtc #119

Closed
wants to merge 1 commit into from

Conversation

abelsiqueira
Copy link
Member

Description

Generate separate objects for cu and nvrtc. Namely libcudawrappers_cu.so and libcudawrappers_nvrtc.so.

Related issues:

Instructions to review the pull request

  • Check that CHANGELOG.md has been updated if necessary

Clone and verify

cd $(mktemp -d --tmpdir cudawrappers-XXXXXX)
git clone https://github.com/nlesc-recruit/cudawrappers .
git checkout <this-branch>
cmake -S . -B build
make -C build
make -C build format # Nothing should change
make -C build lint # linting is broken until we fix the issues (see #92)
  • Look into build for the .so files.

@fdiblen fdiblen added the blocked label Mar 3, 2022
@fdiblen
Copy link
Member

fdiblen commented Mar 3, 2022

We are not sure if this is what we need as #96 is suggesting something else.

@@ -20,6 +20,8 @@ set(SOURCE_FILES src/cu.cpp src/nvrtc.cpp)
include_directories(./include/)
include_directories(${CUDAToolkit_INCLUDE_DIRS})

add_library(cudawrappers_cu SHARED src/cu.cpp)
add_library(cudawrappers_nvrtc SHARED src/nvrtc.cpp)
add_library(cudawrappers SHARED ${SOURCE_FILES})
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to build libcudawrappers.so from source, you could just link all the libcudawrappers_*.so files together?

Copy link
Member Author

Choose a reason for hiding this comment

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

From what I understood no, the .so does not have the necessary information to be merged or linked in another .so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to consider the use of CMake object libraries. You can make object libraries for cudawrappers_cu and cudawrappers_nvrt and link these into the final libcudawrappers_cu.so and libcudawrappers_nvrtc.so, as well as libcudawrappers.so.

@fdiblen
Copy link
Member

fdiblen commented Mar 7, 2022

I think we need to make this more clear before we do more work.

@abelsiqueira abelsiqueira marked this pull request as draft March 7, 2022 16:02
@fdiblen
Copy link
Member

fdiblen commented Mar 8, 2022

Ref:

@bouweandela
Copy link
Member

Superseded by #150

@bouweandela bouweandela deleted the 95-separate-objects branch April 21, 2022 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants