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

Thu Sep 7 07:50:24 PDT 2023

sandeepkosuri added inline comments.

================
Comment at: openmp/runtime/test/target/target_thread_limit.cpp:28
+// OMP51: target: parallel
+// OMP51: target: parallel
+// OMP51-NOT: target: parallel
----------------
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 ?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152054