[Openmp-commits] [PATCH] D22365: Make balanced affinity work on AArch64 (and possibly other architectures too)

[Andrey Churbanov via Openmp-commits]

AndreyChurbanov added inline comments.

================
Comment at: runtime/src/kmp_affinity.cpp:4655
@@ -4621,3 +4654,3 @@
         // Number of cores - maximum value; it does not count trail cores with 0 processors
         int ncores = address2os[ __kmp_avail_proc - 1 ].first.labels[ core_level ] + 1;
 
----------------
pawosm01 wrote:
> AndreyChurbanov wrote:
> > This is the number of cores in the last package. Other packages are skipped (no threads bound there).
> Do you refer to the effect this has on line 4679? This code crashes when __kmp_ncores is used there instead.
My comment was probably irrelevant here.  The problem was incorrect filling of the procarr array that only contained info on the last package.  So if this array is in use (I mostly tested case on line 4710), then all threads were bound to this last package.

If you fix memory overwriting at initialization, then anyway ncores will need to be multiplied by number of packages here or extra loop over packages needs to be added, I think.  Otherwise l don't see how all machine can be covered, given that loops  for( int i = 0; i < ncores; i++ ) only walk through subset of all cores now.

BTW, I finished with debugging of no-HT case. It works fine because you effectively reduced this case to single package via shifting topology one level down and making the balanced affinity to see packages as cores and cores as threads. This trick does not work for HT enabled topology.


Repository:
  rL LLVM

https://reviews.llvm.org/D22365