[PATCH] D152054: [OpenMP] Codegen support for thread_limit on target directive

Thu Sep 7 13:55:33 PDT 2023

mstorsjo added inline comments.

================
Comment at: openmp/runtime/test/target/target_thread_limit.cpp:28
+// OMP51: target: parallel
+// OMP51: target: parallel
+// OMP51-NOT: target: parallel
----------------
sandeepkosuri wrote:
> mstorsjo wrote:
> > mstorsjo wrote:
> > > This test fails when running (on Windows) on GitHub Actions runners - see https://github.com/mstorsjo/llvm-mingw/actions/runs/6019088705/job/16342540379.
> > > 
> > > I believe that this bit of the test has got a hidden assumption that it is running in an environment with 4 or more cores. By setting `#pragma omp target thread_limit(tl)` (with `tl=4`) and running a line in parallel with `#pragma omp parallel`, it expects that we'll get 4 printouts - while in practice, we'll get anywhere between 1 and 4 printouts depending on the number of cores.
> > > 
> > > Is there something that can be done to make this test work in such an environment too?
> > Can someone involved in this patch take on fixing it so that it works on machines with fewer than 4 cores? I'm not sure what's the most appropriate path forward here, as it breaks clearly in such configs (even if it might not be hit by one of the official llvm buildbots, but it shows up as breakage in my nightly builds every day now) - reverting seems a bit harsh. I guess I could just rip out this part of the test?
> @mstorsjo , I noticed that you have committed this https://github.com/llvm/llvm-project/commit/c2019c416c8d7ec50aec6ac6b82c9aa4e99b0f6f
> 
> Does this solve your problem ?
Yes, that commit fixed the issue.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054