From 67684ba4b5b347023306e194b2d3765280a49767 Mon Sep 17 00:00:00 2001 From: "Artem M. Chirkin" <9253178+achirkin@users.noreply.github.com> Date: Wed, 2 Oct 2024 22:01:34 +0200 Subject: [PATCH] Remove EXPLICIT_INSTANTIATE_ONLY macros (#358) `CUVS_EXPLICIT_INSTANTIATE_ONLY` macro carried over from raft; it is not very useful in the fully compiled library, such as cuVS, hence is removed. Authors: - Artem M. Chirkin (https://github.com/achirkin) Approvers: - Corey J. Nolet (https://github.com/cjnolet) URL: https://github.com/rapidsai/cuvs/pull/358 --- cpp/CMakeLists.txt | 2 - cpp/bench/ann/CMakeLists.txt | 2 - .../detail/pairwise_matrix/dispatch-ext.cuh | 4 - .../detail/pairwise_matrix/dispatch.cuh | 4 - cpp/src/distance/distance-ext.cuh | 4 - cpp/src/distance/distance.cuh | 4 - cpp/test/CMakeLists.txt | 2 - cpp/test/cluster/linkage.cu | 9 -- docs/source/developer_guide.md | 91 ------------------- 9 files changed, 122 deletions(-) diff --git a/cpp/CMakeLists.txt b/cpp/CMakeLists.txt index 063a41b1e..6dc32cafe 100644 --- a/cpp/CMakeLists.txt +++ b/cpp/CMakeLists.txt @@ -430,8 +430,6 @@ add_library( src/stats/trustworthiness_score.cu ) -target_compile_definitions(cuvs PRIVATE "CUVS_EXPLICIT_INSTANTIATE_ONLY") - target_compile_options( cuvs INTERFACE $<$:--expt-extended-lambda --expt-relaxed-constexpr> diff --git a/cpp/bench/ann/CMakeLists.txt b/cpp/bench/ann/CMakeLists.txt index 3224587e4..8cbf8c8b3 100644 --- a/cpp/bench/ann/CMakeLists.txt +++ b/cpp/bench/ann/CMakeLists.txt @@ -174,8 +174,6 @@ function(ConfigureAnnBench) ) endif() - target_compile_definitions(${BENCH_NAME} PRIVATE "CUVS_EXPLICIT_INSTANTIATE_ONLY") - target_include_directories( ${BENCH_NAME} PUBLIC "$" diff --git a/cpp/src/distance/detail/pairwise_matrix/dispatch-ext.cuh b/cpp/src/distance/detail/pairwise_matrix/dispatch-ext.cuh index 4fd194f6c..edfd7cf5f 100644 --- a/cpp/src/distance/detail/pairwise_matrix/dispatch-ext.cuh +++ b/cpp/src/distance/detail/pairwise_matrix/dispatch-ext.cuh @@ -22,8 +22,6 @@ #include // raft::identity_op #include // RAFT_EXPLICIT -#ifdef CUVS_EXPLICIT_INSTANTIATE_ONLY - namespace cuvs::distance::detail { template -#ifdef CUVS_EXPLICIT_INSTANTIATE_ONLY - namespace cuvs { namespace distance { @@ -149,8 +147,6 @@ void pairwise_distance(raft::resources const& handle, }; // namespace distance }; // namespace cuvs -#endif // CUVS_EXPLICIT_INSTANTIATE_ONLY - /* * Hierarchy of instantiations: * diff --git a/cpp/src/distance/distance.cuh b/cpp/src/distance/distance.cuh index d1bfc8212..005cb212d 100644 --- a/cpp/src/distance/distance.cuh +++ b/cpp/src/distance/distance.cuh @@ -15,8 +15,4 @@ */ #pragma once -#ifndef CUVS_EXPLICIT_INSTANTIATE_ONLY -#include "distance-inl.cuh" -#endif - #include "distance-ext.cuh" diff --git a/cpp/test/CMakeLists.txt b/cpp/test/CMakeLists.txt index b81ef6bfa..5f7b5b02b 100644 --- a/cpp/test/CMakeLists.txt +++ b/cpp/test/CMakeLists.txt @@ -71,8 +71,6 @@ function(ConfigureTest) "$<$:${CUVS_CUDA_FLAGS}>" ) - target_compile_definitions(${TEST_NAME} PRIVATE "CUVS_EXPLICIT_INSTANTIATE_ONLY") - if(_CUVS_TEST_NOCUDA) target_compile_definitions(${TEST_NAME} PRIVATE "CUVS_DISABLE_CUDA") endif() diff --git a/cpp/test/cluster/linkage.cu b/cpp/test/cluster/linkage.cu index 0f2461fa7..c9f9a50e7 100644 --- a/cpp/test/cluster/linkage.cu +++ b/cpp/test/cluster/linkage.cu @@ -14,15 +14,6 @@ * limitations under the License. */ -// XXX: We allow the instantiation of masked_l2_nn here: -// raft::linkage::FixConnectivitiesRedOp red_op(params.n_row); -// raft::linkage::cross_component_nn( -// handle, out_edges, data.data(), colors.data(), params.n_row, params.n_col, red_op); -// -// TODO: consider adding this to libraft.so or creating an instance in a -// separate translation unit for this test. -#undef CUVS_EXPLICIT_INSTANTIATE_ONLY - #include "../test_utils.cuh" #include diff --git a/docs/source/developer_guide.md b/docs/source/developer_guide.md index 516819b1c..e54336852 100644 --- a/docs/source/developer_guide.md +++ b/docs/source/developer_guide.md @@ -292,97 +292,6 @@ Sometimes, we need to temporarily change the log pattern (eg: for reporting deci 4. Before creating a new primitive, check to see if one exists already. If one exists but the API isn't flexible enough to include your use-case, consider first refactoring the existing primitive. If that is not possible without an extreme number of changes, consider how the public API could be made more flexible. If the new primitive is different enough from all existing primitives, consider whether an existing public API could invoke the new primitive as an option or argument. If the new primitive is different enough from what exists already, add a header for the new public API function to the appropriate subdirectory and namespace. -## Header organization of expensive function templates - -RAFT is a heavily templated library. Several core functions are expensive to compile and we want to prevent duplicate compilation of this functionality. To limit build time, RAFT provides a precompiled library (libraft.so) where expensive function templates are instantiated for the most commonly used template parameters. To prevent (1) accidental instantiation of these templates and (2) unnecessary dependency on the internals of these templates, we use a split header structure and define macros to control template instantiation. This section describes the macros and header structure. - -**Macros.** We define the macros `RAFT_COMPILED` and `RAFT_EXPLICIT_INSTANTIATE_ONLY`. The `RAFT_COMPILED` macro is defined by `CMake` when compiling code that (1) is part of `libraft.so` or (2) is linked with `libraft.so`. It indicates that a precompiled `libraft.so` is present at runtime. - -The `RAFT_EXPLICIT_INSTANTIATE_ONLY` macro is defined by `CMake` during compilation of `libraft.so` itself. When defined, it indicates that implicit instantiations of expensive function templates are forbidden (they result in a compiler error). In the RAFT project, we additionally define this macro during compilation of the tests and benchmarks. - -Below, we summarize which combinations of `RAFT_COMPILED` and `RAFT_EXPLICIT_INSTANTIATE_ONLY` are used in practice and what the effect of the combination is. - -| RAFT_COMPILED | RAFT_EXPLICIT_INSTANTIATE_ONLY | Which targets | -|---------------|--------------------------------|------------------------------------------------------------------------------------------------------| -| defined | defined | `raft::compiled`, RAFT tests, RAFT benchmarks | -| defined | | Downstream libraries depending on `libraft` like cuML, cuGraph. | -| | | Downstream libraries depending on `libraft-headers` like cugraph-ops. | - - -| RAFT_COMPILED | RAFT_EXPLICIT_INSTANTIATE_ONLY | Effect | -|---------------|--------------------------------|-------------------------------------------------------------------------------------------------------| -| defined | defined | Templates are precompiled. Compiler error on accidental instantiation of expensive function template. | -| defined | | Templates are precompiled. Implicit instantiation allowed. | -| | | Nothing precompiled. Implicit instantiation allowed. | -| | defined | Avoid this: nothing precompiled. Compiler error on any instantiation of expensive function template. | - - - -**Header organization.** Any header file that defines an expensive function template (say `expensive.cuh`) should be split in three parts: `expensive.cuh`, `expensive-inl.cuh`, and `expensive-ext.cuh`. The file `expensive-inl.cuh` ("inl" for "inline") contains the template definitions, i.e., the actual code. The file `expensive.cuh` includes one or both of the other two files, depending on the values of the `RAFT_COMPILED` and `RAFT_EXPLICIT_INSTANTIATE_ONLY` macros. The file `expensive-ext.cuh` contains `extern template` instantiations. In addition, if `RAFT_EXPLICIT_INSTANTIATE_ONLY` is set, it contains template definitions to ensure that a compiler error is raised in case of accidental instantiation. - -The dispatching by `expensive.cuh` is performed as follows: -``` c++ -#ifndef RAFT_EXPLICIT_INSTANTIATE_ONLY -// If implicit instantiation is allowed, include template definitions. -#include "expensive-inl.cuh" -#endif - -#ifdef RAFT_COMPILED -// Include extern template instantiations when RAFT is compiled. -#include "expensive-ext.cuh" -#endif -``` - -The file `expensive-inl.cuh` is unchanged: -``` c++ -namespace raft { -template -void expensive(T arg) { - // .. function body -} -} // namespace raft -``` - -The file `expensive-ext.cuh` contains the following: -``` c++ -#include // RAFT_EXPLICIT - -#ifdef RAFT_EXPLICIT_INSTANTIATE_ONLY -namespace raft { -// (1) define templates to raise an error in case of accidental instantiation -template void expensive(T arg) RAFT_EXPLICIT; -} // namespace raft -#endif //RAFT_EXPLICIT_INSTANTIATE_ONLY - -// (2) Provide extern template instantiations. -extern template void raft::expensive(int); -extern template void raft::expensive(float); -``` - -This header has two responsibilities: (1) define templates to raise an error in case of accidental instantiation and (2) provide `extern template` instantiations. -First, if `RAFT_EXPLICIT_INSTANTIATE_ONLY` is set, `expensive` is defined. This is done for two reasons: (1) to give a definition, because the definition in `expensive-inl.cuh` was skipped and (2) to indicate that the template should be explicitly instantiated by taging it with the `RAFT_EXPLICIT` macro. This macro defines the function body, and it ensures that an informative error message is generated when an implicit instantiation erroneously occurs. Finally, the `extern template` instantiations are listed. - -To actually generate the code for the template instances, the file `src/expensive.cu` contains the following. Note that the only difference between the extern template instantiations in `expensive-ext.cuh` and these lines are the removal of the word `extern`: - -``` c++ -#include - -template void raft::expensive(int); -template void raft::expensive(float); -``` - -**Design considerations**: - -1. In the `-ext.cuh` header, do not include implementation headers. Only include function parameter types and types that are used to instantiate the templates. If a primitive takes custom parameter types, define them in a separate header called `_types.hpp`. (see [Common Design Considerations](https://github.com/rapidsai/raft/blob/7b065aff81a0b1976e2a9e2f3de6690361a1111b/docs/source/developer_guide.md#common-design-considerations)). - -2. Keep docstrings in the `-inl.cuh` header, as it is closer to the code. Remove docstrings from template definitions in the `-ext.cuh` header. Make sure to explicitly include public APIs in the RAFT API docs. That is, add `#include ` to the docs in `docs/source/cpp_api/expensive.rst` (instead of `#include `). - -3. The order of inclusion in `expensive.cuh` is extremely important. If `RAFT_EXPLICIT_INSTANTIATE_ONLY` is not defined, but `RAFT_COMPILED` is defined, then we must include the template definitions before the `extern template` instantiations. - -4. If a header file defines multiple expensive templates, it can be that one of them is not instantiated. In this case, **do define** the template with `RAFT_EXPLICIT` in the `-ext` header. This way, when the template is instantiated, the developer gets a helpful error message instead of a confusing "function not found". - -This header structure was proposed in [issue #1416](https://github.com/rapidsai/raft/issues/1416), which contains more background on the motivation of this structure and the mechanics of C++ template instantiation. - ## Testing It's important for RAFT to maintain a high test coverage of the public APIs in order to minimize the potential for downstream projects to encounter unexpected build or runtime behavior as a result of changes.