Improve neighbor loads in horizontal_shift of gtfn
#1779
Conversation
iomaganaris
commented
Jun 4, 2024
iomaganaris
commented
- Define separately neighbor variable to avoid extra memory loads in horizontal_shift()
|
Hi there, this is jenkins continuous integration... |
|
launch jenkins |
|
launch perftests |
iomaganaris
force-pushed
the
neighbor_read_perf_improvement
branch
from
ca10767 to
d302b0b
Compare
| @@ -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)); | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Right, I guess we should go for the compile time has_skip_value check to achieve this and possibly even better performance?
There was a problem hiding this comment.
Yes, I can try it out and launch the perftests CI to check
|
Another thing I noticed: the data layout of the ‘default’ neighbor tables (that is, arrays of tuples) is actually pretty bad: it does not allow for coalesced loads in a reasonable way and does not guarantee any alignment. |
|
launch perftests |
|
launch jenkins |
Credits to @lukasm91 for hinting to `__builtin_assume` in `deref`. On more recent compilers (than 11.2 what we have in CI on daint), will improve codegen to the level of #1779, but is save. --------- Co-authored-by: Felix Thaler <[email protected]>
|
Superseded by #1785 since this PR is problematic |
Credits to @lukasm91 for hinting to `__builtin_assume` in `deref`. On more recent compilers (than 11.2 what we have in CI on daint), will improve codegen to the level of #1779, but is save. --------- Co-authored-by: Felix Thaler <[email protected]>
