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

Reduce use of DataLayouts internals #1934

Merged
merged 1 commit into from
Aug 15, 2024
Merged

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Aug 14, 2024

This is basically a peel off from #1929, and extends #1932. This PR:

  • Reduces the use of parent(data) and especially size(parent(data))
  • eliminates get_n_items, and instead only integers are passed (when needed)
  • Uses UniversalSize to leverage static parameters more
  • Introduces DataLayouts.array_size and DataLayouts.farray_size, which return static sizes of the datalayouts (array_size uses a field dimension of 1, which is often helpful)

This does increase some inference failures because I've passed some additional variables through to gpu kernels using Val, which should result in statically known offsets, potentially improving performance of some kernels (likely by no more than 30%).

@charleskawczynski charleskawczynski force-pushed the ck/use_less_dl_internals branch 2 times, most recently from b582ba3 to 8628c64 Compare August 15, 2024 02:17
@charleskawczynski charleskawczynski force-pushed the ck/use_less_dl_internals branch 8 times, most recently from 8c96331 to 2051e9b Compare August 15, 2024 13:53
@charleskawczynski charleskawczynski marked this pull request as ready for review August 15, 2024 13:53
@charleskawczynski charleskawczynski force-pushed the ck/use_less_dl_internals branch 3 times, most recently from 0badf8c to a6438d5 Compare August 15, 2024 15:15
@charleskawczynski charleskawczynski changed the title Reduce use of DataLayouts internals in ClimaCore Reduce use of DataLayouts internals Aug 15, 2024
@charleskawczynski
Copy link
Member Author

I'll bump the allocation limits, but it looks like CI is otherwise passing.

Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

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

Look good to me, just a question

@charleskawczynski
Copy link
Member Author

just a question

?

@Sbozzolo
Copy link
Member

just a question

?

I thought I left a question in the review, but maybe I didn't click "submit". Upon re writing the question, I found the answer myself :)

@charleskawczynski charleskawczynski force-pushed the ck/use_less_dl_internals branch from a6438d5 to f474b57 Compare August 15, 2024 16:56
@charleskawczynski charleskawczynski merged commit 2f26e9a into main Aug 15, 2024
15 of 19 checks passed
@charleskawczynski charleskawczynski deleted the ck/use_less_dl_internals branch August 15, 2024 16:56
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.

2 participants