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

Support implementation_deps on cuda_object and cuda_library and (transitive) rdc #164

Open
rnburn opened this issue Sep 13, 2023 · 6 comments
Labels
enhancement New feature or request

Comments

@rnburn
Copy link

rnburn commented Sep 13, 2023

implementation_deps are an option with cc_library and allow you to add deps that don't propagage
https://bazel.build/reference/be/c-cpp#cc_library.implementation_deps

@cloudhan
Copy link
Collaborator

cloudhan commented Sep 15, 2023

Great idea. And after careful thinking about cuda_objects and cuda_library with rdc=True, The currect design is flawed.

In rdc case, objects propagation for dlink follows the table, and in non-rdc case, objects does not propagate

rule dep rule dep position defines from deps objects from deps1 handling of objects from deps
co2 co deps use and propagate propagate for dlink -
co co impl3 use not propagate? -
co cl4 disallowed - - -
cl co deps use and propagate propagate for dlink use for dlink, archive
cl co impl use not propagate use for dlink, archive
cl cl deps use and propagate propagate for dlink use for dlink
cl cl impl use not propagate use for dlink

NOTE: co for cuda_objects, cl for cuda_library

Footnotes

  1. deps propagation is the same

  2. cuda_objects

  3. implementation_deps

  4. cuda_library

@rnburn
Copy link
Author

rnburn commented Sep 15, 2023

In rdc case, objects from deps should always propagate for dlink, and in non-rdc case, objects does not propagate

I didn't put together an issue yet. But I was also noticing that library dependencies set on cuda_objects don't get linked in.

For our unit testing framework catch2, I had to add the dependencies again to our cc_test target even though they're already specified on the cuda_object in order to get linking for the tests to work:
https://github.com/spaceandtimelabs/blitzar/blob/main/bazel/sxt_build_system.bzl#L61-L62

@cloudhan
Copy link
Collaborator

That is because cuda_objects only provides a CcInfo with only compilation_context, but no linking context. As the original design, cuda_objects is just compilation actions, whose actifacts are to be fed into a cuda_library for final dlink. Move it to implementation_deps also clarify the purpose here.

@cloudhan cloudhan added the enhancement New feature or request label Sep 15, 2023
@rnburn
Copy link
Author

rnburn commented Sep 15, 2023

That is because cuda_objects only provides a CcInfo with only compilation_context, but no linking context. As the original design, cuda_objects is just compilation actions, whose actifacts are to be fed into a cuda_library for final dlink.

I don't think that scales well to larger projects with external dependencies. In our case, nearly everything in our project is a cuda_object and we only combine it into a cuda_library at the end when we're linking the shared library:
https://github.com/spaceandtimelabs/blitzar/blob/main/cbindings/BUILD#L10-L21

It works ok right now, as we don't have many external dependencies. But if we did start using external library, we'd have to keep track of everything used and add them as dependencies to the final cuda_library -- that could start to become a maintenance burden.

Is there any reason why the cuda_objects can't carry linking dependencies?

@cloudhan
Copy link
Collaborator

Is there any reason why the cuda_objects can't carry linking dependencies?

It is not "can't" per se, it is just not implemented.

I don't think that scales well to larger projects with external dependencies. In our case, nearly everything in our project is a cuda_object...

I am not worried about this. The reason for cuda_objects all over around in your repo is because cuda_library does not work for shared library linking. once we fix them, you should be able to dlink multiple times. But we can impl it anyway.

@cloudhan cloudhan changed the title support implementation_deps on cuda_object and cuda_library Support implementation_deps on cuda_object and cuda_library and (transitive) rdc Sep 16, 2023
@cloudhan
Copy link
Collaborator

Another problem arised when it comes to transitive rdc (allow cuda_library to consume cuda_library and propagate the rdc objects). Previously, it was pretty simple and rdc=True was designed to mark a dlink step in the rule. Then the produced archive is a self contained and is turned back to normal library. Now we need handle the transitive device link issue.

@cloudhan cloudhan mentioned this issue Sep 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants