google / highway

Performance-portable, length-agnostic SIMD with runtime dispatch
Apache License 2.0
4.09k stars 308 forks source link

Question: header only version ? #2114

Open boxerab opened 4 months ago

boxerab commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

aaron-boxer commented 4 months ago

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 commented 4 months ago

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

aaron-boxer commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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

boxerab commented 4 months ago

:) 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 commented 4 months ago

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

boxerab commented 2 months ago

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

jan-wassenberg commented 2 months ago

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