[Openmp-commits] [PATCH] D56804: Fix thread_limits to work properly for teams construct

[Andrey Churbanov via Phabricator via Openmp-commits]

AndreyChurbanov added inline comments.


================
Comment at: runtime/src/kmp.h:1851
                 thread) */
+  int thread_limit; /* internal control for thread-limit-var */
   int max_active_levels; /* internal control for max_active_levels */
----------------
protze.joachim wrote:
> Is this entry necessary? From my understanding of the spec, one copy per cg or thread should be sufficient.
Maybe you are correct, but the specification says it is per-task ICV.  So this looks a safer way to go. E.g. in future the setter routine can be added.


================
Comment at: runtime/src/kmp_runtime.cpp:5676
+    } else {
+      this_th->th.th_cg_roots = tmp->up;
+    }
----------------
This looks incorrect, because worker thread can only belong to one CG at a time, not to chain of CGs.


================
Comment at: runtime/src/kmp_runtime.cpp:7280
+    // Store new thread limit; old limit is saved in th_cg_roots list
+    thr->th.th_current_task->td_icvs.thread_limit = num_threads;
+
----------------
protze.joachim wrote:
> If this is the reason for a per task icv, can't we reset the cg value to the initial value after leaving the teams region?
I'd guess the main reason for a per task ICV is the specification requirement.  Technically the implementation can ignore it and make the ICV per contention group, but this sounds a bit risky to me.


Repository:
  rOMP OpenMP

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56804/new/

https://reviews.llvm.org/D56804