Skip to content

Moved most tpps out of hpps#1686

Merged
lsawade merged 2 commits intoremove-assembly-super-includesfrom
remove-tpps-from-hpps
Mar 6, 2026
Merged

Moved most tpps out of hpps#1686
lsawade merged 2 commits intoremove-assembly-super-includesfrom
remove-tpps-from-hpps

Conversation

@lsawade
Copy link
Copy Markdown
Collaborator

@lsawade lsawade commented Mar 3, 2026

Description

Moving definitions out of hpp's to tpp's and instantiating some things where possible in cpp's. Incremental building is already much better.

Issue Number

Adds to #1651

Checklist

Please make sure to check developer documentation on specfem docs.

  • I ran the code through pre-commit to check style
  • THE DOCUMENTATION BUILDS WITHOUT WARNINGS/ERRORS
  • I have added labels to the PR (see right hand side of the PR page)
  • My code passes all the integration tests
  • I have added sufficient unittests to test my changes
  • I have added/updated documentation for the changes I am proposing
  • I have updated CMakeLists to ensure my code builds
  • My code builds across all platforms

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 template declarations in several headers and move the corresponding template definitions out of those headers.
  • Add explicit template instantiation definitions for selected templates in .cpp translation units (e.g., program header printing, assembly info, locate_sources).
  • Convert specfem::attenuation from a header-only INTERFACE target to a compiled (STATIC) library, and update unit tests to include needed .tpp files.

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.

Comment on lines +1 to 3
file(GLOB_RECURSE ATTENUATION_SOURCES "*.cpp")
add_library(specfem_attenuation STATIC ${ATTENUATION_SOURCES})

Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment on lines +47 to +50
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);
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Suggested change
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);

Copilot uses AI. Check for mistakes.
Comment on lines +125 to +131
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);
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +98
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);
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
@lsawade lsawade merged commit 65944ee into remove-assembly-super-includes Mar 6, 2026
5 checks passed
@lsawade lsawade deleted the remove-tpps-from-hpps branch March 6, 2026 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants