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.

Sign up for GitHub

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

Improve neighbor loads in horizontal_shift of gtfn #1779

Closed
wants to merge 4 commits into from
Closed

Improve neighbor loads in horizontal_shift of gtfn #1779

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a486d3
Bug: storage/gpu.h functions within __CUDA_ARCH__
havogt May 30, 2024
1e6b074
within GT_CUDACC
havogt May 30, 2024
3faab94
Define separately neighbor variable to avoid extra memory loads in ho…
iomaganaris May 28, 2024
d302b0b
Add my name on AUTHORS
iomaganaris Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,4 @@ Mikael Simberg (msimberg), ETH Zurich (CSCS)
Till Ehrengruber (tehrengruber), ETH Zurich (CSCS)
Péter Kardos (petiaccja), ETH Zurich (EXCLAIM)
Stefano Ubbiali (stubbiali), ETH Zurich
Ioannis Magkanaris (iomaganaris), ETH Zurich (CSCS)
3 changes: 2 additions & 1 deletion include/gridtools/fn/unstructured.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ namespace gridtools::fn {
template <class Tag, class Ptr, class Strides, class Domain, class Conn, class Offset>
GT_FUNCTION constexpr auto horizontal_shift(iterator<Tag, Ptr, Strides, Domain> const &it, Conn, Offset) {
auto const &table = host_device::at_key<Conn>(it.m_domain.m_tables);
auto new_index = it.m_index == -1 ? -1 : get<Offset::value>(neighbor_table::neighbors(table, it.m_index));
const auto neighbor = get<Offset::value>(neighbor_table::neighbors(table, it.m_index));
Copy link
Contributor

Choose a reason for hiding this comment

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

This gives an out-of-bounds access if it.m_index is indeed -1, doesn’t it? (Which is a functional change to the previous code, so it makes sense that the compiler can apply different optimizations, e.g., load the neighbor table independently of the value of it.m_index).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that's true so it makes sense. Is there any case that the m_index check can be avoided? I'm wondering what is special with the nabla kernels that they were the only ones that benefited from this change. Also if the total runtime of the nabla kernels in the production case is small it's probably not worth to try and improve this part.
@havogt since this change alters the behavior and is possibly problematic should there be a test in the CI for a breaking case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, I guess we should go for the compile time has_skip_value check to achieve this and possibly even better performance?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I can try it out and launch the perftests CI to check

const auto new_index = it.m_index == -1 ? -1 : neighbor;
auto shifted = it;
shifted.m_index = new_index;
return shifted;
Expand Down
Loading