From adbdfee8c5ab0d6293b6dabdf8b879aa0a512f0a Mon Sep 17 00:00:00 2001 From: Josh Bainbridge Date: Sun, 12 Jul 2026 02:15:01 +0100 Subject: [PATCH 1/2] Migrate project to C++17 and remove binary variant In C++14, constexpr namespace-scope variables have internal linkage, so header-only installs duplicate the ~1.5 MB of blue noise tables in every translation unit that includes them. The binary install variant existed only to work around this, at the cost of three CMake options, conditional target logic, and a preprocessor split in bntables.h. C++17 inline variables have external linkage with a single definition across translation units, removing the need for the workaround. A benchmark recorded in the spec shows the C++17 header-only build matches the binary variant's size exactly, with identical output. Declare the blue noise tables as inline constexpr in bntables.h and delete src/bntables.cpp. Remove the OPENQMC_ENABLE_BINARY, OPENQMC_SHARED_LIB and OPENQMC_FORCE_PIC options, making the library target unconditionally INTERFACE with cxx_std_17. Raise the standard in the unix preset and the manual include example. Update the README, mkdocs home page and CHANGELOG for C++17, VFX Reference Platform CY2021 and NVCC 11. Record validation results in the migration spec, and use auto in trace.cpp for a modernize-use-auto finding that the standard bump newly triggers. Resolves #86 Signed-off-by: Josh Bainbridge --- CHANGELOG.md | 4 ++ CMakeLists.txt | 3 -- CMakePresets.json | 2 +- README.md | 30 ++++----------- cmake/examples/include/CMakeLists.txt | 4 +- include/oqmc/bntables.h | 48 +++--------------------- mkdocs/home.html | 2 +- specs/cpp-17-migration/spec.md | 51 ++++++++++++++++---------- src/CMakeLists.txt | 24 +----------- src/bntables.cpp | 53 --------------------------- src/tools/lib/CMakeLists.txt | 2 +- src/tools/lib/trace.cpp | 2 +- 12 files changed, 58 insertions(+), 167 deletions(-) delete mode 100644 src/bntables.cpp diff --git a/CHANGELOG.md b/CHANGELOG.md index e08f23b..81eeffd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,10 +10,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added ### Changed +- Migrated the project from C++14 to C++17, moving the minimum supported VFX Reference Platform from CY2018 to CY2021, and the minimum NVCC version from 10 to 11. Blue noise tables are now `inline constexpr`, so header-only installs no longer duplicate table data across compilation units. - Sped up scalar paths of `oqmc::sobolReversedIndex` using the construction method from Ahmed 2024. ### Deprecated ### Removed + +- Removed the binary library variant and its `OPENQMC_ENABLE_BINARY`, `OPENQMC_SHARED_LIB` and `OPENQMC_FORCE_PIC` CMake options. C++17 `inline` variables make the header-only install equivalent, so downstream projects using these options should simply remove them. + ### Fixed - Improve quality of uniform float distribution in `oqmc::uintToFloat`. diff --git a/CMakeLists.txt b/CMakeLists.txt index 2a4eafe..f1cc22f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -39,9 +39,6 @@ endif() option(OPENQMC_BUILD_TOOLS "Build the command line tools.") option(OPENQMC_BUILD_TESTING "Build the unit tests.") option(OPENQMC_FORCE_DOWNLOAD "Ignore installed dependencies.") -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.") # Enable testing diff --git a/CMakePresets.json b/CMakePresets.json index f377918..4b10049 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -15,7 +15,7 @@ "inherits": "base", "cacheVariables": { "CMAKE_BUILD_TYPE": "Debug", - "CMAKE_CXX_STANDARD": "14", + "CMAKE_CXX_STANDARD": "17", "CMAKE_CXX_STANDARD_REQUIRED": "ON", "CMAKE_EXPORT_COMPILE_COMMANDS": "ON", "OPENQMC_CXX_FLAGS": "-Werror;-Wall;-Wextra;-Wpedantic;-Wno-unknown-pragmas", diff --git a/README.md b/README.md index 9ccd62a..80fba7e 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ and is actively used in VFX production. ## Description -This C++14 (CPU, GPU) header only library provides an API to deliver high +This C++17 (CPU, GPU) header only library provides an API to deliver high quality QMC sample points. The API aims to be compatible with a variety of common use cases found in production codebases. The library also provides multiple state of the art back-end implementations to minimise noise. @@ -52,7 +52,7 @@ Project features are: - Clear and extendable code base. - Unit and statistical testing. - Modern [CMake](https://cmake.org/) based build system. -- Header only and binary configurations. +- Header only implementation. - No library or STL dependencies. - Includes tools, docs and examples. @@ -125,8 +125,8 @@ more advanced solutions. ## Requirements -The library itself has no dependencies other than C++14. Although not tested -with older versions of GCC, this should make it compatible with CY2018 and +The library itself has no dependencies other than C++17. Although not tested +with older versions of GCC, this should make it compatible with CY2021 and newer versions of the [VFX Reference Platform](https://vfxplatform.com). Supported operating systems: @@ -146,7 +146,7 @@ Tested compilers: - Clang 6 - Clang 21 - GCC 7.5 -- NVCC 10 +- NVCC 11 ## Installation @@ -188,8 +188,6 @@ fastest and simplest approach. ```cmake # Optionally set options (they are OFF by default) set(OPENQMC_ARCH_TYPE AVX CACHE STRING "" FORCE) -set(OPENQMC_ENABLE_BINARY ON CACHE BOOL "" FORCE) -set(OPENQMC_SHARED_LIB ON CACHE BOOL "" FORCE) # Load external dependencies add_subdirectory(path/to/submodule EXCLUDE_FROM_ALL) @@ -199,9 +197,6 @@ mark_as_advanced(FORCE OPENQMC_ARCH_TYPE) mark_as_advanced(FORCE OPENQMC_BUILD_TOOLS) mark_as_advanced(FORCE OPENQMC_BUILD_TESTING) mark_as_advanced(FORCE OPENQMC_FORCE_DOWNLOAD) -mark_as_advanced(FORCE OPENQMC_ENABLE_BINARY) -mark_as_advanced(FORCE OPENQMC_SHARED_LIB) -mark_as_advanced(FORCE OPENQMC_FORCE_PIC) # Add dependencies target_link_libraries(${PROJECT_NAME} PRIVATE OpenQMC::OpenQMC) @@ -219,7 +214,7 @@ installation process will follow an idiomatic approach that provides you the opportunity to set library build options. An example: ```bash -cmake -B build -D CMAKE_INSTALL_PREFIX=/install/path -D OPENQMC_ENABLE_BINARY=ON +cmake -B build -D CMAKE_INSTALL_PREFIX=/install/path cmake --build build --target install ``` @@ -245,8 +240,8 @@ method isn't recommended, as it can be error-prone and prevents using build options otherwise available via CMake. ```cmake -# Enable C++14 support -target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_14) +# Enable C++17 support +target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_17) # Add dependencies target_include_directories(${PROJECT_NAME} PRIVATE path/to/library/include) @@ -260,15 +255,6 @@ which apply to downstream projects. The options are: - `OPENQMC_ARCH_TYPE`: Sets the architecture to optimise for on the CPU, or to target NVIDIA GPUs. Option values can be `Scalar`, `SSE`, `AVX`, `ARM` or `GPU`. Default value is `Scalar`. -- `OPENQMC_ENABLE_BINARY`: You can reduce binary size of downstream projects by - opting for a binary variant of the library. Option values can be `ON` or - `OFF`. Default value is `OFF`. -- `OPENQMC_SHARED_LIB`: You can request a shared library instead of the default - static. This also automatically enables PIC. Option values can be `ON` or - `OFF`. Default value is `OFF`. -- `OPENQMC_FORCE_PIC`: When compiling a static library and the downstream - project is a shared library, you can force enable PIC. Option values can be - `ON` or `OFF`. Default value is `OFF`. ## Versioning diff --git a/cmake/examples/include/CMakeLists.txt b/cmake/examples/include/CMakeLists.txt index b195d3e..f558ca4 100644 --- a/cmake/examples/include/CMakeLists.txt +++ b/cmake/examples/include/CMakeLists.txt @@ -11,9 +11,9 @@ project(example-include VERSION 0.0.0 LANGUAGES CXX) add_executable(${PROJECT_NAME} main.cpp) -# Enable C++14 support +# Enable C++17 support -target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_14) +target_compile_features(${PROJECT_NAME} PRIVATE cxx_std_17) # Add dependencies diff --git a/include/oqmc/bntables.h b/include/oqmc/bntables.h index 0829ef5..aa499fb 100644 --- a/include/oqmc/bntables.h +++ b/include/oqmc/bntables.h @@ -79,82 +79,46 @@ static_assert(xBits == yBits, namespace pmj { -#if defined(OQMC_ENABLE_BINARY) - -/// Optimised blue noise key table for pmj. -extern const std::uint32_t keyTable[size]; - -/// Optimised blue noise rank table for pmj. -extern const std::uint32_t rankTable[size]; - -#else - /// Optimised blue noise key table for pmj. -constexpr std::uint32_t keyTable[] = { +inline constexpr std::uint32_t keyTable[] = { #include "data/pmj/keys.txt" }; /// Optimised blue noise rank table for pmj. -constexpr std::uint32_t rankTable[] = { +inline constexpr std::uint32_t rankTable[] = { #include "data/pmj/ranks.txt" }; -#endif - } // namespace pmj namespace sobol { -#if defined(OQMC_ENABLE_BINARY) - /// Optimised blue noise key table for sobol. -extern const std::uint32_t keyTable[size]; - -/// Optimised blue noise rank table for sobol. -extern const std::uint32_t rankTable[size]; - -#else - -/// Optimised blue noise key table for sobol. -constexpr std::uint32_t keyTable[] = { +inline constexpr std::uint32_t keyTable[] = { #include "data/sobol/keys.txt" }; /// Optimised blue noise rank table for sobol. -constexpr std::uint32_t rankTable[] = { +inline constexpr std::uint32_t rankTable[] = { #include "data/sobol/ranks.txt" }; -#endif - } // namespace sobol namespace lattice { -#if defined(OQMC_ENABLE_BINARY) - -/// Optimised blue noise key table for lattice. -extern const std::uint32_t keyTable[size]; - -/// Optimised blue noise rank table for lattice. -extern const std::uint32_t rankTable[size]; - -#else - /// Optimised blue noise key table for lattice. -constexpr std::uint32_t keyTable[] = { +inline constexpr std::uint32_t keyTable[] = { #include "data/lattice/keys.txt" }; /// Optimised blue noise rank table for lattice. -constexpr std::uint32_t rankTable[] = { +inline constexpr std::uint32_t rankTable[] = { #include "data/lattice/ranks.txt" }; -#endif - } // namespace lattice } // namespace bntables diff --git a/mkdocs/home.html b/mkdocs/home.html index df9da9d..be14bdc 100644 --- a/mkdocs/home.html +++ b/mkdocs/home.html @@ -74,7 +74,7 @@

High Performance

🔧

Easy Integration

-

Header-only C++14 library with no dependencies. Compatible with VFX Reference Platform CY2018 and newer. Drop in and start sampling.

+

Header-only C++17 library with no dependencies. Compatible with VFX Reference Platform CY2021 and newer. Drop in and start sampling.

diff --git a/specs/cpp-17-migration/spec.md b/specs/cpp-17-migration/spec.md index 41eaafe..4f7db34 100644 --- a/specs/cpp-17-migration/spec.md +++ b/specs/cpp-17-migration/spec.md @@ -189,25 +189,38 @@ Update minimum tested compiler versions: ## 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. +- **NVCC device-linkage** (outstanding): 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. Note that the tables are only read from host code + in practice (each blue noise sampler copies them into its cache in + `initialiseCache`, which is host-only), but this must still be verified on + CUDA hardware before release. + +- **Compile-time trade-off** (measured): A benchmark harness compiled 8 + representative consumer translation units (each using all three blue noise + sampler variants, so all six tables are referenced) plus a main, with + `-O2`, best of 3 runs. Apple Silicon M-series, Clang 21.1.1: + + | Configuration | Build time | Binary size | + | --------------------------- | ---------- | ----------- | + | C++14 header-only | 3.83 s | 12.73 MB | + | C++14 binary variant | 2.30 s | 1.64 MB | + | C++17 `inline constexpr` | 3.83 s | 1.64 MB | + + The C++17 build matches the binary variant's final size exactly (the + linker deduplicates the single external-linkage definition) while staying + header-only, and all three builds produce bit-identical sample output. The + per-TU parse cost relative to the binary variant is ~170 ms per table + consuming TU on this machine, the same as the C++14 header-only build. In + real projects only TUs that include a blue noise sampler header pay this + cost. This trade-off is accepted. ## Candidate C++17 improvements diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index b397463..938fa86 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -3,23 +3,7 @@ # Create project target -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() +add_library(${PROJECT_NAME} INTERFACE) if(${OPENQMC_ARCH_TYPE} STREQUAL Scalar) target_compile_definitions(${PROJECT_NAME} INTERFACE OQMC_FORCE_SCALAR) @@ -29,11 +13,7 @@ if(${OPENQMC_ARCH_TYPE} STREQUAL GPU) target_compile_definitions(${PROJECT_NAME} INTERFACE OQMC_FORCE_SCALAR) endif() -if(OPENQMC_ENABLE_BINARY) - target_compile_definitions(${PROJECT_NAME} INTERFACE OQMC_ENABLE_BINARY) -endif() - -target_compile_features(${PROJECT_NAME} INTERFACE cxx_std_14) +target_compile_features(${PROJECT_NAME} INTERFACE cxx_std_17) # Add include directories diff --git a/src/bntables.cpp b/src/bntables.cpp deleted file mode 100644 index 9c59cb2..0000000 --- a/src/bntables.cpp +++ /dev/null @@ -1,53 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -// Copyright Contributors to the OpenQMC Project. - -#include - -namespace oqmc -{ - -namespace bntables -{ - -namespace pmj -{ - -extern const std::uint32_t keyTable[] = { -#include -}; - -extern const std::uint32_t rankTable[] = { -#include -}; - -} // namespace pmj - -namespace sobol -{ - -extern const std::uint32_t keyTable[] = { -#include -}; - -extern const std::uint32_t rankTable[] = { -#include -}; - -} // namespace sobol - -namespace lattice -{ - -extern const std::uint32_t keyTable[] = { -#include -}; - -extern const std::uint32_t rankTable[] = { -#include -}; - -} // namespace lattice - -} // namespace bntables - -} // namespace oqmc diff --git a/src/tools/lib/CMakeLists.txt b/src/tools/lib/CMakeLists.txt index a3b3e0d..40dd6b2 100644 --- a/src/tools/lib/CMakeLists.txt +++ b/src/tools/lib/CMakeLists.txt @@ -71,7 +71,7 @@ function(target_architecture_gpu TARGET) get_target_property(TARGET_SOURCES ${TARGET} SOURCES) set_source_files_properties(${TARGET_SOURCES} PROPERTIES LANGUAGE CUDA) target_compile_options(${TARGET} PRIVATE --extended-lambda) - set_target_properties(${TARGET} PROPERTIES CUDA_STANDARD 14) + set_target_properties(${TARGET} PROPERTIES CUDA_STANDARD 17) set_target_properties(${TARGET} PROPERTIES CUDA_STANDARD_REQUIRED ON) endfunction() diff --git a/src/tools/lib/trace.cpp b/src/tools/lib/trace.cpp index c046049..7ec96f2 100644 --- a/src/tools/lib/trace.cpp +++ b/src/tools/lib/trace.cpp @@ -888,7 +888,7 @@ OQMC_HOST_DEVICE glm::vec3 trace(const Session& session, Method method, int maxOpacity, Ray ray, Sampler traceDomain) { bool computeEmission = true; - glm::vec3 throughput = glm::vec3(1); + auto throughput = glm::vec3(1); glm::vec3 radiance = glm::vec3(); for(int depth = 0; depth <= maxDepth; ++depth) From ffdc9b0824bf2cd1c5756f44a60b9acc511e7205 Mon Sep 17 00:00:00 2001 From: Josh Bainbridge Date: Sun, 12 Jul 2026 03:17:22 +0100 Subject: [PATCH 2/2] Add GPU CI support --- .github/workflows/ci-pipeline.yml | 56 +++++++++++++++++++++++++++++++ CHANGELOG.md | 3 ++ 2 files changed, 59 insertions(+) diff --git a/.github/workflows/ci-pipeline.yml b/.github/workflows/ci-pipeline.yml index 1e94bc3..ae926d5 100644 --- a/.github/workflows/ci-pipeline.yml +++ b/.github/workflows/ci-pipeline.yml @@ -127,11 +127,41 @@ jobs: - run: tar -czvf build.tar.gz build if: ${{ matrix.save }} + # Run trace with each blue noise sampler to validate table linkage + - run: ./build/src/tools/cli/trace pmjbn box + - run: ./build/src/tools/cli/trace sobolbn box + - run: ./build/src/tools/cli/trace latticebn box + # Upload tools build for test-generate - uses: actions/upload-artifact@v4 with: {name: "${{ matrix.build }}-${{ matrix.arch }}", path: build.tar.gz} if: ${{ matrix.save }} + build-tools-gpu: + name: Build tools library GPU + needs: test-unit + strategy: + matrix: + build: [Release, Debug] + runs-on: ubuntu-20.04-gpu-t4-4c-16g-176h + container: + image: aswf/ci-base:2023 + options: --gpus all + steps: + - uses: actions/checkout@v4 + - run: cmake --preset unix -D OPENQMC_ARCH_TYPE=GPU -D CMAKE_CUDA_ARCHITECTURES=75 -D CMAKE_BUILD_TYPE=${{ matrix.build }} + - run: cmake --build --preset unix + - run: tar -czvf build.tar.gz build + + # Run trace with each blue noise sampler to validate table linkage + - run: ./build/src/tools/cli/trace pmjbn box + - run: ./build/src/tools/cli/trace sobolbn box + - run: ./build/src/tools/cli/trace latticebn box + + # Upload tools build for test-generate-gpu + - uses: actions/upload-artifact@v4 + with: {name: "${{ matrix.build }}-GPU", path: build.tar.gz} + pre-test-generate: name: Pre generate reference data needs: build-tools @@ -178,3 +208,29 @@ jobs: - run: tar -xzvf build.tar.gz - run: ./build/src/tools/cli/generate ${{ matrix.sampler }} > output - run: diff output data + + test-generate-gpu: + name: Run generate GPU + needs: + - pre-test-generate + - build-tools-gpu + strategy: + matrix: + build: [Release, Debug] + sampler: [pmj, sobol, lattice] + runs-on: ubuntu-20.04-gpu-t4-4c-16g-176h + container: + image: aswf/ci-base:2023 + options: --gpus all + steps: + # Download tools build + - uses: actions/download-artifact@v4 + with: {name: "${{ matrix.build }}-GPU"} + + # Download sample data + - uses: actions/download-artifact@v4 + with: {name: "${{ matrix.sampler }}"} + + - run: tar -xzvf build.tar.gz + - run: ./build/src/tools/cli/generate ${{ matrix.sampler }} > output + - run: diff output data diff --git a/CHANGELOG.md b/CHANGELOG.md index 81eeffd..8730433 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Added + +- GPU coverage on the CI pipeline is restored using ASWF hosted T4 runners, building the tools as CUDA and validating device results against CPU reference data. + ### Changed - Migrated the project from C++14 to C++17, moving the minimum supported VFX Reference Platform from CY2018 to CY2021, and the minimum NVCC version from 10 to 11. Blue noise tables are now `inline constexpr`, so header-only installs no longer duplicate table data across compilation units.