[Openmp-commits] [PATCH] D95820: [OpenMP] Add bounds to num_teams clause (OpenMP 5.1)

[Hongtao Yu via Phabricator via Openmp-commits]

hoy added inline comments.


================
Comment at: openmp/runtime/test/teams/kmp_num_teams.c:68
+  }
+  if (a != nteams * nthreads) {
+    fprintf(stderr, "error: a (%d) != nteams * nthreads (%d)\n", a,
----------------
AndreyChurbanov wrote:
> AndreyChurbanov wrote:
> > hoy wrote:
> > > AndreyChurbanov wrote:
> > > > jdoerfert wrote:
> > > > > hoy wrote:
> > > > > > Hello, I'm wondering if `nteams` could have an uninitialized use here. It is defined conditionally at line 41. I was seeing `nteams` had a random value. This happened in a very specific testing environment and sorry I cannot provide you a repro. Thanks.
> > > > > When the encountering thread finished executing the target construct it should have also executed line 41. That said, there might be a capturing issue here. Do we need to capture `nteams` as `shared` explicitly to avoid a firstprivate copy in the target teams region?
> > > > Once target construct does not have `shared` clause, `map(tofrom:..)` probably needed for `a`, `nteams` and `nthreads`.
> > > OK, I'm seeing `nteams` has different addresses in the master and team threads. Maybe `shared` is needed.
> > I tried to fix the test in commit 091e8da.
> > Strictly the `a` needs to be in `in_reduction` and `reduction`, but I hope atomic to work here.  Otherwise `map(tofrom)` should be changed to `in_reduction(+:a) reduction(+:a)`.
> Sorry, I was in a hurry.  My previous fix should not work with real offload, because the test was intended to check the `teams` construct on a host.  With real offload the test cannot pass because `num_teams` and `thread_limit` won't be propagated to the device. So the proper fix would be to remove the `target` construct, and execute the `teams` on host.  I've committed another patch 5631842d1810 that should definitely fix the problem.  I apologize to miss the test problem during review (I actually don't have a machine with an accelerator at hand).
Thanks for the quick fix!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95820