Skip to content

Commit

Permalink
Remove EXPLICIT_INSTANTIATE_ONLY macros (#358)
Browse files Browse the repository at this point in the history
`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: #358
  • Loading branch information
achirkin authored Oct 2, 2024
1 parent 6c0fd14 commit 67684ba
Show file tree
Hide file tree
Showing 9 changed files with 0 additions and 122 deletions.
2 changes: 0 additions & 2 deletions cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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 $<$<COMPILE_LANG_AND_ID:CUDA,NVIDIA>:--expt-extended-lambda
--expt-relaxed-constexpr>
Expand Down
2 changes: 0 additions & 2 deletions cpp/bench/ann/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -174,8 +174,6 @@ function(ConfigureAnnBench)
)
endif()

target_compile_definitions(${BENCH_NAME} PRIVATE "CUVS_EXPLICIT_INSTANTIATE_ONLY")

target_include_directories(
${BENCH_NAME}
PUBLIC "$<BUILD_INTERFACE:${CUVS_SOURCE_DIR}/include>"
Expand Down
4 changes: 0 additions & 4 deletions cpp/src/distance/detail/pairwise_matrix/dispatch-ext.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,6 @@
#include <raft/core/operators.hpp> // raft::identity_op
#include <raft/util/raft_explicit.hpp> // RAFT_EXPLICIT

#ifdef CUVS_EXPLICIT_INSTANTIATE_ONLY

namespace cuvs::distance::detail {

template <typename OpT,
Expand All @@ -47,8 +45,6 @@ void pairwise_matrix_dispatch(OpT distance_op,

}; // namespace cuvs::distance::detail

#endif // CUVS_EXPLICIT_INSTANTIATE_ONLY

#define instantiate_cuvs_distance_detail_pairwise_matrix_dispatch( \
OpT, DataT, AccT, OutT, FinOpT, IdxT) \
extern template void cuvs::distance::detail:: \
Expand Down
4 changes: 0 additions & 4 deletions cpp/src/distance/detail/pairwise_matrix/dispatch.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,4 @@
*/
#pragma once

#ifndef CUVS_EXPLICIT_INSTANTIATE_ONLY
#include "dispatch-inl.cuh"
#endif

#include "dispatch-ext.cuh"
4 changes: 0 additions & 4 deletions cpp/src/distance/distance-ext.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@

#include <cuda_fp16.h>

#ifdef CUVS_EXPLICIT_INSTANTIATE_ONLY

namespace cuvs {
namespace distance {

Expand Down Expand Up @@ -149,8 +147,6 @@ void pairwise_distance(raft::resources const& handle,
}; // namespace distance
}; // namespace cuvs

#endif // CUVS_EXPLICIT_INSTANTIATE_ONLY

/*
* Hierarchy of instantiations:
*
Expand Down
4 changes: 0 additions & 4 deletions cpp/src/distance/distance.cuh
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,4 @@
*/
#pragma once

#ifndef CUVS_EXPLICIT_INSTANTIATE_ONLY
#include "distance-inl.cuh"
#endif

#include "distance-ext.cuh"
2 changes: 0 additions & 2 deletions cpp/test/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,6 @@ function(ConfigureTest)
"$<$<COMPILE_LANGUAGE:CUDA>:${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()
Expand Down
9 changes: 0 additions & 9 deletions cpp/test/cluster/linkage.cu
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,6 @@
* limitations under the License.
*/

// XXX: We allow the instantiation of masked_l2_nn here:
// raft::linkage::FixConnectivitiesRedOp<value_idx, value_t> red_op(params.n_row);
// raft::linkage::cross_component_nn<value_idx, value_t>(
// 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 <cuvs/cluster/agglomerative.hpp>
Expand Down
91 changes: 0 additions & 91 deletions docs/source/developer_guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <typename T>
void expensive(T arg) {
// .. function body
}
} // namespace raft
```
The file `expensive-ext.cuh` contains the following:
``` c++
#include <raft/util/raft_explicit.cuh> // RAFT_EXPLICIT
#ifdef RAFT_EXPLICIT_INSTANTIATE_ONLY
namespace raft {
// (1) define templates to raise an error in case of accidental instantiation
template <typename T> void expensive(T arg) RAFT_EXPLICIT;
} // namespace raft
#endif //RAFT_EXPLICIT_INSTANTIATE_ONLY
// (2) Provide extern template instantiations.
extern template void raft::expensive<int>(int);
extern template void raft::expensive<float>(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 <raft/expensive-inl.cuh>

template void raft::expensive<int>(int);
template void raft::expensive<float>(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 `<primitive_name>_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 <raft/expensive.cuh>` to the docs in `docs/source/cpp_api/expensive.rst` (instead of `#include <raft/expensive-inl.cuh>`).

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.
Expand Down

0 comments on commit 67684ba

Please sign in to comment.