[CMake] rewrite precompiled header usage - #2222
Draft
tdavidcl wants to merge 10 commits into
Draft
Conversation
…ries SHAMROCK_USE_PCH existed but only wired shamrock_exe/shamrock_pylib/ shamrock_test to `REUSE_FROM shamrock_lib`, a target that does not exist yet (src/shamrock/CMakeLists.txt still has a TODO to rename shamlib to shamrock_lib), so enabling the option did nothing for the 20 actual compiled libraries, including the ones that dominate build time per the build-profile report (shammodels_sph, shammodels_gsph, shamalgs, shamtree, shamlib). Add cmake/ShamrockPCH.cmake with sham_pch_root()/sham_pch_reuse() helpers: shambackends precompiles the small set of headers that get re-parsed in nearly every translation unit (sycl.hpp, DeviceBuffer.hpp, nlohmann/json.hpp), and every library that depends on it directly or transitively reuses that same precompiled object instead of re-parsing the headers itself. Still off by default. Assisted-by: Claude Code
… env This is the machine config CI's build-profile job (and most other acpp-clang CI jobs) uses, so this gets the now-fixed PCH plumbing exercised by CI to see its actual effect, without changing the SHAMROCK_USE_PCH default (still Off) for any other machine config. Assisted-by: Claude Code
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
CI hit "error: is pie differs in PCH file vs. current file" repeatedly in shamrock_test: shambackends (the PCH-owning target) is a shared library and gets -fPIC from CMake automatically, but the plain executable targets (shamrock_exe, shamrock_test) got no such override and fell back to -fPIE, which Clang's PCH validator rejects as a mismatch. Force -fPIC uniformly via CMAKE_CXX_FLAGS whenever SHAMROCK_USE_PCH is on, mirroring the existing object-lib-mode workaround for the same class of problem. Verified locally: main.cpp, main_test.cpp and a real test file (tests/shammath/AABB_tests.cpp) all now compile cleanly against the reused shambackends PCH. Assisted-by: Claude Code
Precompiling shambackends/sycl.hpp standalone (-x c++-header) skips AdaptiveCpp's normal per-TU multipass compilation for the cuda/hip backends, so compiler-injected builtins the generic hiplike kernel launcher relies on (__acpp_warp_size, the kernel launch macros) end up undeclared when generating the PCH: error: use of undeclared identifier '__acpp_warp_size' error: use of undeclared identifier 'decomposition' Scope the PCH to the CPU-only backends (omp*, generic) where it works; env_tests.yml's cuda.integrated-multipass/cuda.explicit-multipass matrix entries build this same debian-generic.acpp machine config. Assisted-by: Claude Code
pybind11_add_module forces CXX_VISIBILITY_PRESET=hidden (and, when supported, -flto) specifically on the module target it creates, neither of which shambackends' PCH was built with: error: default visibility for functions and variables [-fvisibility] differs in PCH file vs. current file shamrock_exe (a plain executable, unaffected by those pybind11-module-only properties) still reuses the PCH. Assisted-by: Claude Code
The clang-tidy CI job only configures and generates version.cpp, it never runs a full ninja build, so shambackends' cmake_pch.hxx.pch is never actually built. Every compile command clang-tidy inherited from compile_commands.json still asked for it via "-Xclang -include-pch -Xclang <path>.pch", which clang-tidy then reports as a hard error: error: PCH file '.../cmake_pch.hxx.pch' not found: module file not found error: unable to read PCH file ...: 'No such file or directory' Drop that pair (and -Winvalid-pch, now pointless) in make_clang_tidy_db.py; the plain "-Xclang -include -Xclang <path>.hxx" stays, so clang-tidy still sees the same header content, just parsed normally instead of via the (missing) precompiled form. Verified locally: clang-tidy now runs clean on a file that previously hit this error. Assisted-by: Claude Code
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
Workflow reportworkflow report corresponding to commit 855e60a Pre-commit check reportPre-commit check: ✅ Test pipeline can run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.