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

Optimizations on the loop blocking #1

Closed

Conversation

iomaganaris
Copy link

@iomaganaris iomaganaris commented Oct 17, 2024

  • Read neighbors only once for the unrolled loop and then only once for the epilogue loop
  • Create as_const_sid for nanobind_adapter to be able to pass the input fields as const and allow the compiler to apply optimizations like avoiding reading vertically independent fields in every k loop iteration (based on Felix's suggestion)
  • It was too painful to reuse as_sid from as_const_sid by adding a bool template parameter to as_sid and depending on that call add_const due to the variadic parameters. Maybe try and find a way to make it better?

The idea behind as_const_sid is to be called by the nanobind bindings generated by gt4py only for the input fields. Some extra work is necessary for that.

…st and avoid reading fields that are independent of the vertical level in each iteration
Copy link
Owner

@fthaler fthaler left a comment

Choose a reason for hiding this comment

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

Once everything works and is ready, I would actually suggest to open two separate PRs on the main GT repo: one with the SID neighbor table updates and the other one with as_const_sid as both are quite independent.
For as_const_sid a test would also be appreciated.

include/gridtools/storage/adapter/nanobind_adapter.hpp Outdated Show resolved Hide resolved
include/gridtools/storage/adapter/nanobind_adapter.hpp Outdated Show resolved Hide resolved
include/gridtools/fn/sid_neighbor_table.hpp Outdated Show resolved Hide resolved
include/gridtools/fn/sid_neighbor_table.hpp Outdated Show resolved Hide resolved
include/gridtools/fn/sid_neighbor_table.hpp Outdated Show resolved Hide resolved
@fthaler
Copy link
Owner

fthaler commented Oct 23, 2024

Just checked the nanobind documentation and saw that there is a ReadOnly property in the ndarray class (https://nanobind.readthedocs.io/en/latest/api_extra.html#_CPPv4N8nanobind7ndarray8ReadOnlyE). Could it also make sense to just read this property and decide based on this if we should treat the data type as const?

iomaganaris and others added 3 commits October 23, 2024 11:33
@iomaganaris
Copy link
Author

Just checked the nanobind documentation and saw that there is a ReadOnly property in the ndarray class (https://nanobind.readthedocs.io/en/latest/api_extra.html#_CPPv4N8nanobind7ndarray8ReadOnlyE). Could it also make sense to just read this property and decide based on this if we should treat the data type as const?

Good point 👍 I guess something like the last commit together with https://github.com/GridTools/icon_structured_benchmark/compare/main...gtfn_improvements would also make sense? In the bindings I've only added const to the input fields, since the neighbors are going to be automatically turned to const later in as_neighbor_table. We could also just pass them as const from the bindings as well. What do you think?

@iomaganaris
Copy link
Author

Superseded by GridTools#1808 and GridTools#1809

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

Successfully merging this pull request may close these issues.

2 participants