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

Remove dummy hooks for kernels #1700

Closed
wants to merge 4 commits into from
Closed

Conversation

MarcelKoch
Copy link
Member

This PR removes the need for dummy kernels hooks. By using if constexpr, kernels for non-build backends are just skipped, so no implementation needs to be provided.

@MarcelKoch MarcelKoch self-assigned this Oct 16, 2024
@ginkgo-bot ginkgo-bot added the mod:core This is related to the core module. label Oct 16, 2024
@MarcelKoch MarcelKoch requested a review from a team October 16, 2024 13:53
@MarcelKoch MarcelKoch added the 1:ST:ready-for-review This PR is ready for review label Oct 16, 2024
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

LGTM. it reduces the missing possibility

Comment on lines +487 to +488
} \
\
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} \
\
} \

nit

@upsj
Copy link
Member

upsj commented Oct 16, 2024

IMO this removes a rather significant feature - being able to swap out backend binaries to replace dummies by actual implementations. With a bit of clean-up of the ABI, this would allow us to ship binaries for all backends compiled by different compilers.

@MarcelKoch
Copy link
Member Author

This is not a 'feature' we are using right now, and I highly doubt we would ever use it.

With a bit of clean-up of the ABI, this would allow us to ship binaries for all backends compiled by different compilers.

I'm not sure how to understand this. We can already do, if multiple backends are enabled, which requires having the different compiler available on the same system.
So would you suggest compiling the different backend libraries on different machines, and then manually linking them together?

@upsj
Copy link
Member

upsj commented Oct 16, 2024

I was planning to start using it for Julia binary distribution in Yggdrasil, making the CUDA and ROCm support controllable.

@MarcelKoch
Copy link
Member Author

Can you explain that a bit more? Were you building only the backend libraries on different systems, then bringing them onto one system, and linking them together?

@MarcelKoch
Copy link
Member Author

MarcelKoch commented Oct 16, 2024

Also, shouldn't that still be possible? On the system where you build core and link everything together, you need to enable all backends, and then you should be able to use the libraries from the other systems.

@upsj
Copy link
Member

upsj commented Oct 16, 2024

No, if you build Ginkgo with all backends enabled, then executing a binary linking against Ginkgo requires all backend runtime libraries (for CUDA/HIP/SYCL) to be available, unless we want to start implementing our own dynamic linking, which we absolutely should not attempt.

Without the stubs, we lose the main reason why we split things up into separate libraries in the first place.

@MarcelKoch
Copy link
Member Author

@upsj I'm still struggling to understand your point, since I'm not really familiar with linking. As far as I understand it, with the current implementation you would be able to:

  1. Build Ginkgo with no backends enabled
  2. Build all backend libraries separately
  3. Ship the ginkgo library with no backends to a target system
  4. Replace the dummy libraries with the actual backend library on the target system (probably by using LD_PRELOAD)

This is different from building Ginkgo with all backends enabled, since other runtime libraries, e.g. cuda runtime, would be needed for backends which are not available on the target system.

Does that summarize the issue you mentioned?

@upsj
Copy link
Member

upsj commented Oct 17, 2024

Yes, by linking against a shared library (e.g. libginkgo_cuda.so, you pull in all dependent libraries like libcusparse.so, ...)

@MarcelKoch
Copy link
Member Author

@upsj Then I get your point. I will close this PR as I agree that this would be a nice feature to have.

@MarcelKoch MarcelKoch closed this Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-for-review This PR is ready for review mod:core This is related to the core module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants