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

Preserve KOKKOS_INVALID_INDEX in ViewDimension and ArrayLayout construction #5188

Merged
merged 7 commits into from
Jul 11, 2022

Conversation

janciesko
Copy link
Contributor

Comment on lines +496 to +503
return array_layout((VORank > 0 ? m_dim.N0 : KOKKOS_INVALID_INDEX),
(VORank > 1 ? m_dim.N1 : KOKKOS_INVALID_INDEX),
(VORank > 2 ? m_dim.N2 : KOKKOS_INVALID_INDEX),
(VORank > 3 ? m_dim.N3 : KOKKOS_INVALID_INDEX),
(VORank > 4 ? m_dim.N4 : KOKKOS_INVALID_INDEX),
(VORank > 5 ? m_dim.N5 : KOKKOS_INVALID_INDEX),
(VORank > 6 ? m_dim.N6 : KOKKOS_INVALID_INDEX),
(VORank > 7 ? m_dim.N7 : KOKKOS_INVALID_INDEX));
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this also need to be the dynamic rank?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From looking at TestViewLayoutTiled and the layout implementation it looks like we do only support static ranks. I think we should be good with that for now.

@masterleinad masterleinad added this to the Tentative 3.7 Release milestone Jul 8, 2022
@masterleinad masterleinad added this to In progress in Kokkos Release 3.7 -- 2022 Target Date via automation Jul 8, 2022
@masterleinad masterleinad added the Blocks Promotion Overview issue for release-blocking bugs label Jul 8, 2022
@crtrott
Copy link
Member

crtrott commented Jul 8, 2022

Note, this does change the behavior to what mdspan does. I wouldn't necessarily document that yet, but I think it is the correct thing to do: i.e. you can either pass dynamik_rank or rank arguments to the View. Note the whole layout() thing is still a bit messy. For example this would still allow this I think:

View<int*> a("A",N);
View(int*[4]) b("B",a.layout());

But lets fix that later.

Copy link
Contributor

@masterleinad masterleinad left a comment

Choose a reason for hiding this comment

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

I think this is good enough as a fix. We should also check for matching static extents in a follow-up.

@PhilMiller
Copy link
Contributor

I expect this will help with the issues I was trying to address in DynRankView. I'll need to look at the code, and maybe try to update my PRs related to that to be sure. Probably not a reason to hold this up if it addresses the more pressing issue(s) for core View.

@janciesko
Copy link
Contributor Author

Note, this does change the behavior to what mdspan does. I wouldn't necessarily document that yet, but I think it is the correct thing to do: i.e. you can either pass dynamik_rank or rank arguments to the View. Note the whole layout() thing is still a bit messy. For example this would still allow this I think:

View<int*> a("A",N);
View(int*[4]) b("B",a.layout());

But lets fix that later.

Yes, that's fixable.

@janciesko
Copy link
Contributor Author

Retest this please

@crtrott
Copy link
Member

crtrott commented Jul 8, 2022

I wonder whether that timeout is real? I.e. we screw somewhere up and don't get all the information over and a loop nest now is infinite long or so?

@dalg24
Copy link
Member

dalg24 commented Jul 11, 2022

I wonder whether that timeout is real? I.e. we screw somewhere up and don't get all the information over and a loop nest now is infinite long or so?

I've seen this failure (OpenMP timeout) in other PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Blocks Promotion Overview issue for release-blocking bugs
Development

Successfully merging this pull request may close these issues.

None yet

6 participants