Repository navigation
Conversation
|
Please rebase |
| header | ||
| ) | ||
| target_include_directories( | ||
| ${SVS_LIB} BEFORE INTERFACE $<BUILD_INTERFACE:${generated_include_dir}> |
There was a problem hiding this comment.
Blocker: the header only exists in the build tree, and the install rule in CMakeLists.txt copies ${PROJECT_SOURCE_DIR}/include/svs only. After cmake --install, euclidean.h / inner_product.h / cosine.h include a dispatch_surface.h that isn't there, so any find_package(svs) consumer (and the conda package) fails to compile. On a fresh configure of this branch, build/cmake_install.cmake has no rule for it; CI doesn't catch it because everything builds in-tree.
Suggest something like:
install(
FILES "${header}"
DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}/svs/core/distance"
)| endforeach() | ||
| if(undeclared) | ||
| string(REPLACE ";" ", " undeclared_display "${undeclared}") | ||
| message(STATUS " not in the surface: ${undeclared_display}") |
There was a problem hiding this comment.
nit: on a default configure this always prints not in the surface: NONE plus the two-line explanation. Since NONE is excluded by design (the validator strips it), I'd drop it here too so the message only fires when a real level is missing.
| endforeach() | ||
|
|
||
| # mandatory levels | ||
| foreach(svs_mandatory_level AVX2 AVX512) |
There was a problem hiding this comment.
question: hard-coding AVX2 and AVX512 as mandatory sits a bit oddly next to a declaration file that's meant to be overridable. Is this a permanent constraint from distance_core.h, or temporary until a later slice? If permanent, a one-line pointer to the code that requires it would help.
| #define DISTANCE_L2_INSTANTIATE_TEMPLATE(N, AVX) \ | ||
| DISTANCE_L2_TEMPLATE_HELPER(template, N, AVX); | ||
| // Alias macro for each ISA level to support targeted combinations per ISA | ||
| #define SVS_TYPE_PAIRS_NONE SVS_FOR_EACH_TYPE_PAIR |
There was a problem hiding this comment.
Is this SVS_TYPE_PAIRS_NONE for a subsequent PR? Looks unused here. Can leave it if there will be usage later on.
| // Important: If a project uses SVS and calls compute kernels on datatype | ||
| // combinations not in this list, they will be compiled with their project | ||
| // and won't support multi-arch dispatching | ||
| #define SVS_FOR_EACH_TYPE_PAIR(M, ...) \ |
There was a problem hiding this comment.
Looks like DISTANCE_{L2,IP,CS}_{EXTERN,INSTANTIATE}_TEMPLATE macros are removed outright. They're in an installed header, so any downstream user breaks.
| target_compile_options(${lib_name} INTERFACE -march=${arch} -mtune=${arch}) | ||
| set(obj_name ${arch}_obj) | ||
|
|
||
| set(obj_name ${infix}_obj) |
There was a problem hiding this comment.
This would result in renaming haswell_obj/cascadelake_obj to avx2_obj/avx512_obj - is it intentional?
This is the first PR in a series that will re-introduce what was there in #372.
I'm cherry-picking the changes and try to limit the scope of each PR.
This PR only removes the explicit dim macros in the source files and lets cmake generate the necessary bits.