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

Redefine pitches #2093

Merged
merged 3 commits into from
Sep 1, 2023
Merged

Conversation

bernhardmgruber
Copy link
Member

@bernhardmgruber bernhardmgruber commented Aug 25, 2023

This PR shifts the values returned from getPitchesInBytes to be consistent with std::mdspan (except in bytes).

Example: the pitch vector for the extent {42, 10, 2} changes:

Before: {4, 3360, 80, 8}
After: {80, 8, 4}

The new meaning is that the pitch value is the number of bytes to jump from one element to the next in the given dimension.

Fixes: #2083

Waiting on #2092 to be merged before rebase.

This is a silent breaking change. However, to a function that was only introduced a few days ago with #2092.

Copy link
Contributor

@fwyzard fwyzard left a comment

Choose a reason for hiding this comment

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

why the change from

struct Action {
  static auto action(...) { ... }
};

to

struct Action {
  auto operator()(...) const { ... }
};

?

include/alpaka/mem/buf/BufUniformCudaHipRt.hpp Outdated Show resolved Hide resolved
include/alpaka/mem/buf/BufUniformCudaHipRt.hpp Outdated Show resolved Hide resolved
include/alpaka/mem/buf/cpu/Set.hpp Outdated Show resolved Hide resolved
include/alpaka/mem/view/ViewSubView.hpp Outdated Show resolved Hide resolved
@bernhardmgruber bernhardmgruber force-pushed the pitch_redefine branch 3 times, most recently from a4bb1f9 to 0bb73bc Compare August 29, 2023 13:28
@bernhardmgruber
Copy link
Member Author

why the change from

struct Action {
  static auto action(...) { ... }
};

to

struct Action {
  auto operator()(...) const { ... }
};

?

I tried hard to find the corresponding issue, but probably it was just a long discussion on a PR somewhere. I argued for using operator() consistently to avoid repreating the name of the functionality in the customization point struct name (trait) and the member function. I think we concluded to move to operator() in the future. All math customizations are already using this scheme btw. But the current situation in alpaka is inconsistent.

I don't care too much what we do. operator() is what I would prefer, but I won't lose any sleep over something else. What do other people think?

@bernhardmgruber bernhardmgruber marked this pull request as ready for review August 29, 2023 14:21
@bernhardmgruber bernhardmgruber force-pushed the pitch_redefine branch 4 times, most recently from bf207e8 to 777baed Compare August 29, 2023 16:31
@fwyzard
Copy link
Contributor

fwyzard commented Aug 29, 2023

I don't care too much what we do. operator() is what I would prefer, but I won't lose any sleep over something else. What do other people think?

If the reason is to avoid duplicating the name you could use something like static auto apply() for each class.

But I don't really mind, I was only curious about the change.

@bernhardmgruber bernhardmgruber force-pushed the pitch_redefine branch 2 times, most recently from 26df5ae to ce28ec3 Compare August 29, 2023 17:23
Comment on lines 67 to 68
TExtent const& extent,
std::size_t pitchBytes)
std::size_t pitchBytes,
TExtent const& extent)
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be swapped back.

@bernhardmgruber
Copy link
Member Author

I really appreciate how a MD-subscript operation into a buffer is now the dot product between index and pitch vector <3. This is also consistent with some MD array talk I have seen.

psychocoderHPC
psychocoderHPC previously approved these changes Aug 30, 2023
Copy link
Member

@psychocoderHPC psychocoderHPC left a comment

Choose a reason for hiding this comment

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

only one question but nothing against the PR at all.

test/integ/matMul/src/matMul.cpp Outdated Show resolved Hide resolved
This PR shifts the values returned from getPitchesInBytes to be
consistent with std::mdspan (except in bytes).

Example: the pitch vector for the extent {42, 10, 2} changes:

Before: {4, 3360, 80, 8}
After: {80, 8, 4}

The new meaning is that the pitch value is the number of bytes to jump
from one element to the next in the given dimension.

Fixes: alpaka-group#2083
@bernhardmgruber
Copy link
Member Author

Hmm. Something got stuck with the readthedocs build. The error saids: "This build was terminated due to inactivity. If you continue to encounter this error, file a support request with and reference this build id (21768482)."

@psychocoderHPC psychocoderHPC merged commit 7e99f63 into alpaka-group:develop Sep 1, 2023
21 checks passed
@bernhardmgruber bernhardmgruber deleted the pitch_redefine branch September 1, 2023 09:24
bernhardmgruber added a commit to bernhardmgruber/alpaka that referenced this pull request Sep 4, 2023
These are changes missed as part of alpaka-group#2093.

Fixes: alpaka-group#2124
bernhardmgruber added a commit to bernhardmgruber/alpaka that referenced this pull request Sep 4, 2023
These are changes missed as part of alpaka-group#2093.

Fixes: alpaka-group#2124
fwyzard pushed a commit that referenced this pull request Sep 4, 2023
These are changes missed as part of #2093.

Fixes: #2124
psychocoderHPC added a commit to psychocoderHPC/cupla that referenced this pull request Jan 25, 2024
With alpaka-group#245 the latest alpaka version was introduced in cupla.
The alpaka pitch definition has changed with
alpaka-group/alpaka#2093.

We forget to change the usage in alpaka-group#245.

This PR handels the pitch correctly.
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.

Rework pitches
3 participants