diff --git a/specs/cpp-17-migration/spec.md b/specs/cpp-17-migration/spec.md new file mode 100644 index 0000000..41eaafe --- /dev/null +++ b/specs/cpp-17-migration/spec.md @@ -0,0 +1,299 @@ +# Migrate project to C++17 standard and reduce complexity + +GitHub Issue: https://github.com/AcademySoftwareFoundation/openqmc/issues/86 + +## Summary + +Migrate OpenQMC from C++14 to C++17 to eliminate the binary/header-only +install split and reduce build system complexity. C++17 `inline` variables +remove the need for the `OQMC_ENABLE_BINARY` mechanism entirely, allowing the +library to remain header-only without duplicating large static arrays across +compilation units. + +## Motivation + +OpenQMC includes six large blue noise lookup tables (key and rank tables for +PMJ, Sobol, and Lattice samplers) defined in `include/oqmc/bntables.h`. Each +table is 2^16 entries of `uint32_t`, totalling roughly 1.5 MB of data across +all six. + +In C++14, `constexpr` namespace-scope variables have internal linkage. When +`bntables.h` is included from multiple translation units, each unit gets its +own copy of the table data in the final binary. To avoid this, the project +offers a binary variant controlled by the `OPENQMC_ENABLE_BINARY` CMake +option. When enabled, the tables are declared `extern` in the header and +defined once in `src/bntables.cpp`, which is compiled into a static or shared +library. + +This creates user-facing complexity: + +- Three CMake options (`OPENQMC_ENABLE_BINARY`, `OPENQMC_SHARED_LIB`, + `OPENQMC_FORCE_PIC`) that exist solely for this purpose. +- Conditional `INTERFACE` vs `STATIC`/`SHARED` library target logic in + `src/CMakeLists.txt`. +- A preprocessor split (`#if defined(OQMC_ENABLE_BINARY)`) in `bntables.h`. +- Documentation explaining the trade-off between header-only and binary + installs. + +C++17 introduces `inline` variable declarations. An `inline constexpr` +namespace-scope variable has external linkage and is guaranteed to have a +single definition across all translation units. This directly solves the +duplication problem with no build system workaround required. + +## Changes — Headers + +Replace the conditional compilation in `include/oqmc/bntables.h`. The current +pattern for each sampler namespace (repeated for `pmj`, `sobol`, `lattice`): + +```cpp +#if defined(OQMC_ENABLE_BINARY) +extern const std::uint32_t keyTable[size]; +extern const std::uint32_t rankTable[size]; +#else +constexpr std::uint32_t keyTable[] = { +#include "data/pmj/keys.txt" +}; +constexpr std::uint32_t rankTable[] = { +#include "data/pmj/ranks.txt" +}; +#endif +``` + +Becomes: + +```cpp +inline constexpr std::uint32_t keyTable[] = { +#include "data/pmj/keys.txt" +}; +inline constexpr std::uint32_t rankTable[] = { +#include "data/pmj/ranks.txt" +}; +``` + +The `#if defined(OQMC_ENABLE_BINARY)` blocks and corresponding `#else` / +`#endif` are removed entirely. + +## Changes — Source code + +### `src/bntables.cpp` + +Delete this file. It only exists to provide `extern` definitions for the +binary variant. + +## Changes — Build system + +### `CMakeLists.txt` (root) + +Remove the following options: + +```cmake +option(OPENQMC_ENABLE_BINARY "Build binary to reduce memory cost.") +option(OPENQMC_SHARED_LIB "Make a shared library, in place of static.") +option(OPENQMC_FORCE_PIC "Force PIC for static libraries.") +``` + +### `src/CMakeLists.txt` + +Replace the conditional library creation: + +```cmake +if(OPENQMC_ENABLE_BINARY) + if(OPENQMC_SHARED_LIB) + add_library(${PROJECT_NAME} SHARED) + else() + add_library(${PROJECT_NAME} STATIC) + endif() + + if(OPENQMC_FORCE_PIC) + set_target_properties(${PROJECT_NAME} PROPERTIES POSITION_INDEPENDENT_CODE ON) + endif() + + target_sources(${PROJECT_NAME} PRIVATE bntables.cpp) + target_compile_options(${PROJECT_NAME} PRIVATE ${OPENQMC_CXX_FLAGS}) + target_include_directories(${PROJECT_NAME} PRIVATE ${PROJECT_SOURCE_DIR}/include) +else() + add_library(${PROJECT_NAME} INTERFACE) +endif() +``` + +With: + +```cmake +add_library(${PROJECT_NAME} INTERFACE) +``` + +Remove the `OQMC_ENABLE_BINARY` compile definition block: + +```cmake +if(OPENQMC_ENABLE_BINARY) + target_compile_definitions(${PROJECT_NAME} INTERFACE OQMC_ENABLE_BINARY) +endif() +``` + +Change the C++ standard requirement: + +```cmake +# Before +target_compile_features(${PROJECT_NAME} INTERFACE cxx_std_14) + +# After +target_compile_features(${PROJECT_NAME} INTERFACE cxx_std_17) +``` + +### `CMakePresets.json` + +The `unix` preset sets `CMAKE_CXX_STANDARD` to `"14"` (line 18). Update to +`"17"`. + +### `cmake/examples/` + +Update `cxx_std_14` to `cxx_std_17` in the manual include example. + +## Changes — Documentation + +### `README.md` + +- Change "This C++14 (CPU, GPU) header only library" to reference C++17. +- Update the requirements section: change "no dependencies other than C++14" + to C++17 and adjust VFX Reference Platform compatibility to CY2021. +- Remove or update references to the binary install variant and its CMake + options throughout the document. + +Update minimum tested compiler versions: + +- **GCC**: GCC 7 is the minimum requirement for C++17. No change needed. +- **Clang**: Clang 5 is the minimum requirement for C++17. No change needed. +- **NVCC**: Minimum version moves from NVCC 10 to NVCC 11. Update tested + compiler documentation and CI matrix accordingly. + +### `mkdocs/home.html` + +- Update "Header-only C++14 library" and "CY2018" references on line 77. + +## Migration risks + +- **Downstream breakage**: Projects using `OPENQMC_ENABLE_BINARY`, + `OPENQMC_SHARED_LIB`, or `OPENQMC_FORCE_PIC` will see CMake errors on + upgrade. This is an intentional breaking change. A CHANGELOG entry and + migration note should accompany the release. +- **VFX Reference Platform**: This migration moves the minimum from CY2018 + to CY2021. +- **Per-TU parse cost**: `OPENQMC_ENABLE_BINARY` solved two problems at once. + As well as avoiding C++14's internal-linkage duplication in the final + binary, defining the tables in a single `src/bntables.cpp` meant only that + one translation unit had to parse the ~393K lines of integer literals in + `include/oqmc/data/`. Moving to header-only `inline constexpr` fixes the + duplication but reintroduces the parse cost in *every* translation unit that + includes `bntables.h`. This is the headline trade-off of the migration and + needs to be measured before it is accepted (see Validation). + +## Validation + +The following must be carried out before the migration is accepted. They are +not yet done. + +- **NVCC device-linkage**: The blue noise tables are consumed from + `OQMC_HOST_DEVICE` code (`bntables.h` `tableValue`), so they must be usable + on the device. The current internal-linkage `constexpr` arrays produce a + per-TU copy, which is what makes them trivially available in device code + today. `inline constexpr` changes this to a single external-linkage + definition, and NVCC's handling of inline-variable device-side definitions + has historically had caveats. Confirm on the minimum supported toolchain + (NVCC 11) that the tables compile, link, and produce correct results in + device code with no duplicate-definition or missing-symbol errors. + +- **Compile-time trade-off**: A test/benchmark must be created to measure the + per-TU parse cost described under Migration risks on a representative + consumer (several translation units each including `bntables.h`). Compare + the C++14 binary variant, C++14 header-only, and the proposed C++17 + `inline constexpr` builds for total compile time and final binary size, and + record the results here so the trade-off can be accepted explicitly. + +## Candidate C++17 improvements + +Beyond the core `inline constexpr` change, the following are candidate +improvements enabled by C++17 that could be included in this migration or +follow-up work. + +### Hex float literals + +`include/oqmc/float.h` line 34: + +```cpp +// Before +constexpr auto floatOneOverTwoPower32 = 1.0f / (1ull << 32); ///< 0x1p-32 + +// After +constexpr auto floatOneOverTwoPower32 = 0x1p-32f; +``` + +Hex float literals (P3.9) are a C++17 feature. This replaces a computed +constant with an exact representation, removing the reliance on compile-time +evaluation of the division. + +### `[[maybe_unused]]` attribute + +The project defines a custom `OQMC_MAYBE_UNUSED` macro in +`include/oqmc/unused.h` (line 11) to suppress unused variable warnings: + +```cpp +#define OQMC_MAYBE_UNUSED(exp) (void)(exp) +``` + +This is used in 6 locations across the codebase: + +- `include/oqmc/stochastic.h` line 36 — `maxIndexSize` +- `include/oqmc/lattice.h` lines 51, 62 — `cache` parameter +- `include/oqmc/sobol.h` lines 51, 62 — `cache` parameter +- `src/shapes.h` lines 95, 110 — `x` and `y` parameters + +C++17 provides `[[maybe_unused]]` as a standard attribute. All uses can be +replaced, and `unused.h` can be deleted. For example: + +```cpp +// Before +OQMC_MAYBE_UNUSED(cache); + +// After (on parameter) +[[maybe_unused]] const void* cache +``` + +### Nested namespace declarations + +`include/oqmc/bntables.h` uses nested namespaces with separate declarations: + +```cpp +namespace oqmc +{ +namespace bntables +{ +namespace pmj +{ +``` + +C++17 allows compact nested namespace syntax: + +```cpp +namespace oqmc::bntables::pmj +{ +``` + +However, care should be taken to remain consistent with the rest of the +codebase. If adopted, it should apply uniformly (e.g. `bntables.h` uses 3 +levels of nesting, while most other files use 1-2). This is a low-priority +stylistic change. + +### `if constexpr` + +`include/oqmc/owen.h` line 51 uses a runtime `if` to check dimension 0: + +```cpp +if(dimension == 0) +{ + return reverseBits16(index); +} +``` + +This could become `if constexpr` if the function were templated on +`dimension`. However, this would be an API change and the compiler likely +optimises this already. Low priority.