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

Question: header only version ? #2114

Open
boxerab opened this issue Apr 27, 2024 · 13 comments
Open

Question: header only version ? #2114

boxerab opened this issue Apr 27, 2024 · 13 comments

Comments

@boxerab
Copy link
Contributor

boxerab commented Apr 27, 2024

Not sure if this is feasible, but as the ops are already inline header only,
would it be possible for the rest of the library to be header only as well ?
Pros are ease of integration into other projects, cons are time needed to refactor,
and code bloat, also perhaps maintainability.

@jan-wassenberg
Copy link
Member

We're indeed close to this for ops in static-dispatch mode. There are a few source files (targets, print, abort, timer) which would be reasonably straightforward to bundle into a header. contrib also has image and vqsort*.

I think a larger obstacle would be per_target.cc, which uses foreach_target. It seems difficult to reason about how that would compose with a header-only library, at the very least it would impose requirements on the ordering of highway.h vs any other Highway-using headers.

Would anyone be interested in looking into this?

@johnplatts
Copy link
Contributor

johnplatts commented Apr 29, 2024

Here is an example of a project that allows Google Highway to be used in a header-only manner if compiling for a single HWY_TARGET:
https://github.com/johnplatts/simdhwyhash

The CMakeLists.txt file in simdhwyhash checks to see if Google Highway can be used in a header-only manner in simdhwyhash using check_cxx_source_compiles.

Including the check to see if Google Highway can be used in a header-only manner in simdhwyhash also speeds up the build time in the case where the system-provided Google Highway library is not used and libhwy does not have to be built.

@jan-wassenberg
Copy link
Member

Interesting, great that that works, thanks for sharing :)

@aaron-boxer
Copy link

aaron-boxer commented May 1, 2024

We're indeed close to this for ops in static-dispatch mode. There are a few source files (targets, print, abort, timer) which would be reasonably straightforward to bundle into a header. contrib also has image and vqsort*.

I think a larger obstacle would be per_target.cc, which uses foreach_target. It seems difficult to reason about how that would compose with a header-only library, at the very least it would impose requirements on the ordering of highway.h vs any other Highway-using headers.

Perhaps all of the low hanging fruit can be converted into header only, while leaving per_target.cc for now, and then we could figure out a design for removing per_target.cc. I don't think ordering requirement would be that difficult for clients to handle.

@jan-wassenberg
Copy link
Member

Possibly; some users might not even require per_target. What did you have in mind with "converted into header only"?

@aaron-boxer
Copy link

aaron-boxer commented May 1, 2024

What did you have in mind with "converted into header only"?

Just move implementations into header files, is what I had in mind. And delete the .cc files

@jan-wassenberg
Copy link
Member

hm, another idea would be to add #include of their .cc files, gated by an #if. Seems like that would allow experimentation with no impact on current usages?

@boxerab
Copy link
Contributor Author

boxerab commented May 1, 2024

hm, another idea would be to add #include of their .cc files, gated by an #if. Seems like that would allow experimentation with no impact on current usages?

great idea

@jan-wassenberg
Copy link
Member

:) Would you, or anyone else, like to experiment with that?

@boxerab
Copy link
Contributor Author

boxerab commented May 1, 2024

:) Would you, or anyone else, like to experiment with that?

presumably we would have a HWY_HEADER_ONLY flag and accompanying #define , and the .cc files would not be included in the CMake files if the flag was set. If you can set this up, I can experiment with including the .cc files as you suggested

@jan-wassenberg
Copy link
Member

Sure, can do. Let's start with only the main library for now.

copybara-service bot pushed a commit that referenced this issue May 2, 2024
copybara-service bot pushed a commit that referenced this issue May 2, 2024
copybara-service bot pushed a commit that referenced this issue May 2, 2024
@boxerab
Copy link
Contributor Author

boxerab commented Jun 20, 2024

@jan-wassenberg I'm afraid I won't have time to look at this for a while - should we close ?

@jan-wassenberg
Copy link
Member

No worries. I think we can leave this open for now in case anyone else is interested?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants