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

views from Kokkos_ScratchSpace should use different alignment #1700

Closed
alanw0 opened this issue Jul 17, 2018 · 8 comments
Closed

views from Kokkos_ScratchSpace should use different alignment #1700

alanw0 opened this issue Jul 17, 2018 · 8 comments
Assignees
Labels
Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos) Enhancement Improve existing capability; will potentially require voting
Milestone

Comments

@alanw0
Copy link

alanw0 commented Jul 17, 2018

This link:
https://github.com/kokkos/kokkos/blob/master/core/src/Kokkos_ScratchSpace.hpp#L65
shows that views into Kokkos_ScratchSpace have 8-byte alignment.
We would like to request that this be changed to use KOKKOS_MEMORY_ALIGNMENT for the alignment value.

@ibaned ibaned self-assigned this Aug 1, 2018
@ibaned ibaned added this to the 2018 July milestone Aug 1, 2018
@ibaned ibaned added the Enhancement Improve existing capability; will potentially require voting label Aug 1, 2018
@crtrott
Copy link
Member

crtrott commented Aug 15, 2018

Sure, Dan is doing it ;-)

@ibaned
Copy link
Contributor

ibaned commented Aug 22, 2018

I tried to do a simple replacement of '8' with 'KOKKOS_MEMORY_ALIGNMENT', but that caused a lot more scratch memory to be allocated and crashed our tests. A more involved approach is needed, not sure when I'll have time to debug it.

@alanw0
Copy link
Author

alanw0 commented Aug 22, 2018

Just curious - did the memory increase by more than ((KOKKOS_MEMORY_ALIGNMENT) - (8)) per scratch-view ? Maybe that difference is multiplied by num-threads or something?

I guess when the user provides bytes-per-thread to the TeamPolicy, they don't account for alignment padding, nor do they say how many views they will be obtaining into that memory...

If this can't be quickly solved, perhaps nalu needs to move on to some other solution.

@crtrott
Copy link
Member

crtrott commented Sep 19, 2018

One issue is that KOKKOS_MEMORY_ALIGNMENT is probably larger than it needs to be. On GPUs I think it might 128 bytes to align with the cache line. So if you create a lot fo small things this is gonna be costly. What we should do is align by max(8,sizeof(value_type)).

@crtrott
Copy link
Member

crtrott commented Sep 19, 2018

Also we don't probably need KOKKOS_MEMORY_ALIGNMENT. I bet that the main issue is that SIMD types don't get aligned correctly and cause issues? Is that right?

@alanw0
Copy link
Author

alanw0 commented Sep 20, 2018

Yes the issue is with simd types.

@crtrott
Copy link
Member

crtrott commented Sep 24, 2018

Ok so I have a fix ready for pull request, which will align each scratch view to sizeof(value_type). That should hopefully make this work.

@crtrott crtrott added the Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos) label Sep 24, 2018
@crtrott
Copy link
Member

crtrott commented Sep 24, 2018

Marking this as a bug since it actually needs to be aligned for types which require alignment.

crtrott added a commit that referenced this issue Sep 24, 2018
@ibaned ibaned assigned crtrott and unassigned ibaned Sep 29, 2018
@crtrott crtrott closed this as completed Nov 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Broken / incorrect code; it could be Kokkos' responsibility, or others’ (e.g., Trilinos) Enhancement Improve existing capability; will potentially require voting
Projects
None yet
Development

No branches or pull requests

3 participants