Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add more inbounds and inlining #1189

Closed
wants to merge 2 commits into from
Closed

Add more inbounds and inlining #1189

wants to merge 2 commits into from

Conversation

charleskawczynski
Copy link
Member

This PR applies more @inbounds and inlining to

  • some of the spectral element operators (written for the cpu)
  • some of the dss kernels

@charleskawczynski
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Apr 14, 2023
@charleskawczynski
Copy link
Member Author

Interestingly, this doesn’t seem to have much of an impact. Any differences could easily be variation.

@charleskawczynski charleskawczynski marked this pull request as draft April 17, 2023 19:42
@charleskawczynski
Copy link
Member Author

The CPU vector dss is the slowest of the spectral kernels, and using IntrospectionTools along with julia --check-bounds=no:

using Revise; using ClimaCore
include(joinpath(pkgdir(ClimaCore), "test", "Operators", "spectralelement", "benchmark_utils.jl"))
include(joinpath(pkgdir(ClimaCore), "test", "Operators", "spectralelement", "benchmark_kernels.jl"))
args = setup_kernel_args(["--device", "CPU"]);
kernel_vector_dss!(args)
using IntrospectionTools
@code_summary kernel_vector_dss!(args)

shows that we're currently generating the same native code as if we used julia --check-bounds=no, so these changes are not needed.

@charleskawczynski charleskawczynski deleted the ck/dss_opt branch April 25, 2023 21:20
@charleskawczynski charleskawczynski restored the ck/dss_opt branch May 6, 2023 05:04
@charleskawczynski
Copy link
Member Author

Actually, the allocation job shows that this fixes some allocation sites. So perhaps we should reconsider.

@charleskawczynski
Copy link
Member Author

Superseded by #1338

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants