[Openmp-commits] [PATCH] D51944: Fix affinity mask usage on big-endian CPUs

[Jonathan Peyton via Phabricator via Openmp-commits]

jlpeyton added a comment.

Almost, if `KMP_AFFINITY_ENABLE()` is reverted back and if the `KMPNativeAffinity::Mask::mask_t` type was made public (I don't see any problem doing that), then along with the rest of changes you made, I think it should look something like this:

  diff --git a/runtime/src/z_Linux_util.cpp b/runtime/src/z_Linux_util.cpp
  index 8c59b43..7506380 100644
  --- a/runtime/src/z_Linux_util.cpp
  +++ b/runtime/src/z_Linux_util.cpp
  @@ -179,7 +179,10 @@ void __kmp_affinity_determine_capable(const char *env_var) {
           KMP_INTERNAL_FREE(buf);
         }
         if (errno == EFAULT) {
  -        KMP_AFFINITY_ENABLE(gCode);
  +        int mask_size = ((gCode + sizeof(KMPNativeAffinity::Mask::mask_t) - 1) &
  +                         ~(sizeof(KMPNativeAffinity::Mask::mask_t))) /
  +                        sizeof(KMPNativeAffinity::Mask::mask_t);
  +        KMP_AFFINITY_ENABLE(mask_size);
           KA_TRACE(10, ("__kmp_affinity_determine_capable: "
                         "affinity supported (mask size %d)\n",
                         (int)__kmp_affin_mask_size));
  @@ -256,7 +259,10 @@ void __kmp_affinity_determine_capable(const char *env_var) {
           return;
         }
         if (errno == EFAULT) {
  -        KMP_AFFINITY_ENABLE(gCode);
  +        int mask_size = ((gCode + sizeof(KMPNativeAffinity::Mask::mask_t) - 1) &
  +                         ~(sizeof(KMPNativeAffinity::Mask::mask_t))) /
  +                        sizeof(KMPNativeAffinity::Mask::mask_t);
  +        KMP_AFFINITY_ENABLE(mask_size);
           KA_TRACE(10, ("__kmp_affinity_determine_capable: "
                         "affinity supported (mask size %d)\n",
                         (int)__kmp_affin_mask_size));

Sorry, I didn't explain it all the way in my previous comment.  `__kmp_affin_mask_size` should be the number of `mask_t`s to allocate (instead of just the raw size in bytes) which requires further dividing by `sizeof(mask_t)`.  So if we take the code you have in the macro and apply it to the two places in z_Linux_util.cpp (plus the extra division), then it would look like above.  This also allows us to change `mask_t` to whatever type we please without any further modifications to the code.  Also, this won't disturb the Windows/Hwloc code if we leave the `KMP_AFFINITY_ENABLE()` macro alone.

So

1. Revert `KMP_AFFINITY_ENABLE()`
2. Keep the rest of your current changes
3. Make mask_t public
4. Add the changes to z_Linux_util.cpp


Repository:
  rOMP OpenMP

https://reviews.llvm.org/D51944