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

Make alignment consistent #809

Closed
hcedwar opened this issue May 17, 2017 · 2 comments
Closed

Make alignment consistent #809

hcedwar opened this issue May 17, 2017 · 2 comments
Assignees
Labels
Enhancement Improve existing capability; will potentially require voting
Milestone

Comments

@hcedwar
Copy link
Contributor

hcedwar commented May 17, 2017

In Kokkos_Macros.hpp we have KOKKOS_ALIGN_SIZE with inconsistent values.
In Kokkos_MemoryTraits.hpp we have KOKKOS_MEMORY_ALIGNMENT with different value.
and scratch memory space Views are 8 byte aligned.

Need to have consistent alignment of global memory throughout; or clear comment/documentation for different alignments.
OK that level 0 scratch is (smaller) that global memory alignment for CUDA.

@hcedwar hcedwar added the Enhancement Improve existing capability; will potentially require voting label May 17, 2017
@hcedwar hcedwar added this to the Backlog milestone May 17, 2017
@etphipp
Copy link
Contributor

etphipp commented Jul 5, 2017

In addition, the user needs greater control over the alignment/padding decisions. For example, by my reading of the calculation of the padding size in View_Mapping.hpp (starting around line 1700), a 2-D layout right, double precision view would only be padded if it had >= 64 columns, even though the performance benefits can be observed with a much smaller number of columns. For example, in a tensor project I am working on, the dominant cost is a kernel that processes a tall, skinny, layout-right matrix. With the current alignment/padding logic, I get about 30% less performance for smaller numbers of columns if I force padding versus not (e.g., by setting Kokkos::MEMORY_ALIGNMENT_THRESHOLD = 0 in Kokkos_MemoryTraits.hpp, instead of 4).

At the very minimum, the user should be able to change Kokkos::MEMORY_ALIGNMENT_THRESHOLD at compile time, similar to how it is done with Kokkos::Kokkos::MEMORY_ALIGNMENT. However I think an even better approach would be to allow the user to specify these constants on a per-view basis, e.g., by passing them in as arguments to the AllowPadding ctor property.

Also, the user needs control over the alignment of scratch memory space views. For example, for KNL, I want scratch views 64 byte aligned for aligned vector load/stores.

@etphipp
Copy link
Contributor

etphipp commented Jul 5, 2017

To clarify, I meant 30% less performance is the views are not padded (which is what happens now).

@ibaned ibaned modified the milestones: 2017-August (middle), Backlog Jul 5, 2017
hcedwar added a commit to hcedwar/kokkos that referenced this issue Sep 20, 2017
For kokkos#1092: Move SpaceAccessibility meta-function to Kokkos namespace.

For kokkos#809: Always have KOKKOS_MEMORY_ALIGNMENT macro defined,
replace KOKKOS_ALIGN_SIZE with KOKKOS_MEMORY_ALIGNMENT, and
static_assert that it is a power of two.
Rename KOKKOS_ALIGN_PTR macro as KOKKOS_IMPL_ALIGN_PTR.
Remove KOKKOS_ALIGN macro and just use __attribute__((aligned(size))),
this is used in only one soon-to-be-deprecated location.
crtrott added a commit that referenced this issue Sep 21, 2017
@crtrott crtrott closed this as completed Oct 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve existing capability; will potentially require voting
Projects
None yet
Development

No branches or pull requests

4 participants