Moved most tpps out of hpps#1686
Conversation
There was a problem hiding this comment.
Pull request overview
This PR continues the compile-time improvement effort (#1651) by moving template definitions out of public headers into .tpp files and adding explicit template instantiations in .cpp files where possible, reducing recompilation blast radius and improving incremental builds.
Changes:
- Add
extern templatedeclarations in several headers and move the corresponding template definitions out of those headers. - Add explicit template instantiation definitions for selected templates in
.cpptranslation units (e.g., program header printing, assembly info, locate_sources). - Convert
specfem::attenuationfrom a header-only INTERFACE target to a compiled (STATIC) library, and update unit tests to include needed.tppfiles.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit-tests/attenuation/compute_tau_sigma_tests.cpp | Includes compute_tau_sigma.tpp directly after template definitions were moved out of headers. |
| tests/unit-tests/attenuation/compute_tau_eps_tests.cpp | Includes compute_tau_eps.tpp/compute_tau_sigma.tpp to make template definitions visible for test-only instantiations. |
| tests/unit-tests/attenuation/compute_factors_tests.cpp | Includes compute_factors.tpp to access template definitions in tests. |
| tests/unit-tests/assembly/sources/sources.cpp | Adds missing include for compute_source_array.hpp after header dependency changes. |
| core/specfem/program/program.cpp | Adds program.tpp include and explicit instantiations for print_header (dim2/dim3). |
| core/specfem/program/CMakeLists.txt | Links specfem::program against specfem::attenuation. |
| core/specfem/program.hpp | Adds extern template declarations for print_header and removes direct .tpp inclusion. |
| core/specfem/attenuation/compute_tau_sigma.hpp | Replaces .tpp inclusion with an extern template declaration. |
| core/specfem/attenuation/compute_tau_eps.hpp | Replaces .tpp inclusion with an extern template declaration. |
| core/specfem/attenuation/compute_factors.hpp | Replaces .tpp inclusion with extern template declarations. |
| core/specfem/attenuation/CMakeLists.txt | Switches attenuation target from INTERFACE to STATIC. |
| core/specfem/assembly/sources/impl/locate_sources.hpp | Replaces .tpp inclusion with extern template declarations (dim2/dim3). |
| core/specfem/assembly/sources/dim2/sources.cpp | Includes locate_sources.tpp and explicitly instantiates locate_sources<dim2>. |
| core/specfem/assembly/sources/dim3/sources.cpp | Includes locate_sources.tpp and explicitly instantiates locate_sources<dim3>. |
| core/specfem/assembly/info.hpp | Replaces .tpp inclusion with extern template declarations for constructors (dim2/dim3). |
| core/specfem/assembly/info.cpp | Includes info.tpp and explicitly instantiates Info constructors (dim2/dim3). |
| core/specfem/assembly/assembly/dim3/assembly.hpp | Removes an unnecessary include (compute_source_array.hpp). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| file(GLOB_RECURSE ATTENUATION_SOURCES "*.cpp") | ||
| add_library(specfem_attenuation STATIC ${ATTENUATION_SOURCES}) | ||
|
|
There was a problem hiding this comment.
specfem_attenuation is now created as a STATIC library from *.cpp sources, but this directory currently contains no .cpp files. With an empty ATTENUATION_SOURCES list, CMake typically errors out with “No SOURCES given to target …”, and even if it configures, the library would be an empty archive and cannot host the explicit template instantiations implied by the new extern template declarations. Add at least one .cpp (e.g., an instantiation/translation unit) to this directory and list it here, or keep the target as an INTERFACE library until there are compiled sources.
| extern template Kokkos::View<type_real[specfem::constants::N_SLS], | ||
| Kokkos::LayoutRight, Kokkos::HostSpace> | ||
| specfem::attenuation::compute_tau_sigma<specfem::constants::N_SLS>( | ||
| const type_real, const type_real); |
There was a problem hiding this comment.
This extern template declaration suppresses implicit instantiation of compute_tau_sigma<specfem::constants::N_SLS> in every TU including this header. I couldn’t find any corresponding explicit instantiation definition (i.e., a .cpp that includes compute_tau_sigma.tpp and has a template ... compute_tau_sigma<specfem::constants::N_SLS>(...) line), so calls such as in core/specfem/program/qplots.cpp will end up with an undefined reference at link time. Add a single explicit instantiation definition in the specfem_attenuation library (or remove the extern template until that exists).
| extern template Kokkos::View<type_real[specfem::constants::N_SLS], | |
| Kokkos::LayoutRight, Kokkos::HostSpace> | |
| specfem::attenuation::compute_tau_sigma<specfem::constants::N_SLS>( | |
| const type_real, const type_real); |
| extern template Kokkos::View<type_real[specfem::constants::N_SLS], | ||
| Kokkos::LayoutRight, Kokkos::HostSpace> | ||
| specfem::attenuation::compute_tau_eps<specfem::constants::N_SLS>( | ||
| type_real, | ||
| Kokkos::View<type_real[specfem::constants::N_SLS], Kokkos::LayoutRight, | ||
| Kokkos::HostSpace>, | ||
| type_real, type_real); |
There was a problem hiding this comment.
Same issue as compute_tau_sigma.hpp: this extern template declaration requires a matching explicit instantiation definition in exactly one compiled translation unit. A repository-wide search didn’t find any .cpp explicitly instantiating compute_tau_eps<specfem::constants::N_SLS>, so any TU calling it (e.g., qplots.cpp) will compile but fail to link. Add the explicit instantiation definition in the attenuation library (or drop the extern template here).
| extern template specfem::attenuation::AttenuationPropertyValues< | ||
| specfem::constants::N_SLS> | ||
| specfem::attenuation::get_attenuation_property_values< | ||
| specfem::constants::N_SLS>( | ||
| Kokkos::View<type_real[specfem::constants::N_SLS], Kokkos::LayoutRight, | ||
| Kokkos::HostSpace>, | ||
| Kokkos::View<type_real[specfem::constants::N_SLS], Kokkos::LayoutRight, | ||
| Kokkos::HostSpace>); | ||
|
|
||
| extern template type_real specfem::attenuation::get_attenuation_scale_factor< | ||
| specfem::constants::N_SLS>( | ||
| type_real, | ||
| Kokkos::View<type_real[specfem::constants::N_SLS], Kokkos::LayoutRight, | ||
| Kokkos::HostSpace>, | ||
| Kokkos::View<type_real[specfem::constants::N_SLS], Kokkos::LayoutRight, | ||
| Kokkos::HostSpace>, | ||
| type_real, type_real); |
There was a problem hiding this comment.
These extern template declarations suppress implicit instantiation for the specfem::constants::N_SLS specializations, but there is no corresponding explicit instantiation definition in any .cpp (searching the repo found none). That will produce unresolved externals anywhere these functions are used without including the .tpp. Add explicit instantiation definitions for both functions in a single .cpp compiled into specfem_attenuation, or remove these extern template declarations.
Description
Moving definitions out of
hpp's totpp's and instantiating some things where possible incpp's. Incremental building is already much better.Issue Number
Adds to #1651
Checklist
Please make sure to check developer documentation on specfem docs.