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

Fix allocating large Views #4907

Merged
merged 8 commits into from
Mar 30, 2022

Conversation

masterleinad
Copy link
Contributor

Fixes #4906. For Kokkos::Views with dynamic View greater or equal to 3, we use unsigned int as type for storing the dynamic rank. Hence, we need to explicitly cast to size_t in some places where we multiply the ranks and return size_t.
Still needs the test.

@fnrizzi
Copy link
Contributor

fnrizzi commented Mar 28, 2022

@masterleinad is this ready or still draft? ah i see, the test is missing

@masterleinad
Copy link
Contributor Author

@masterleinad is this ready or still draft? ah i see, the test is missing

I just need to add a test and make sure it works on all backends. Otherwise, it's ready from my side.

@mzuzek
Copy link

mzuzek commented Mar 28, 2022

Hi @masterleinad!
Do you think operator() in ViewOffset could also be affected and should have size_type casts in index multiplication ?

@masterleinad
Copy link
Contributor Author

Do you think operator() in ViewOffset could also be affected and should have size_type casts in index multiplication ?

It's probably a good idea.

@PhilMiller
Copy link
Contributor

Should this potentially get cherry picked for 3.6 as well?

@PhilMiller
Copy link
Contributor

In operator() and other spots with nested expressions, I think the casts may need to be applied pretty much ubiquitously, since otherwise subexpressions may overflow before reaching the cast

@masterleinad
Copy link
Contributor Author

In operator() and other spots with nested expressions, I think the casts may need to be applied pretty much ubiquitously, since otherwise subexpressions may overflow before reaching the cast

Thinking about it, we should discuss this. I'm pretty sure that the type for the variables was chosen here due to performance implications which might matter more for View access than calling span() or the like. In the end, this is something that can be worked around in user code by providing the indices as size_t.

@masterleinad masterleinad marked this pull request as ready for review March 28, 2022 17:08
@masterleinad
Copy link
Contributor Author

I marked this as ready for review and would consider making the discussion about the call operator separate.

KOKKOS_FUNCTION void operator()(int) const {
size_t idx = v.extent(0) - 1;
auto& lhs = v(idx, idx, idx, idx, idx, idx, idx, idx);
lhs = 0; // This is where it segfaulted
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this assign a non-zero value foo, and then assert on the host that v.access(idx, ...) == foo? As written, this is a weak smoke-test that the particular case doesn't happen to crash in our CI environments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@PhilMiller PhilMiller left a comment

Choose a reason for hiding this comment

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

Up to you whether to make the test more demanding.

@crtrott crtrott merged commit 9c3a7f9 into kokkos:develop Mar 30, 2022
@masterleinad masterleinad deleted the fix_large_view_allocations branch May 17, 2022 22:06
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.

None yet

6 participants