Hi -- can you tell me which specific tests failed reliably? Was it one particular arch? I want to make sure I'm covering those tests and will give your change a try and review soon. Thanks!

Just a side note that dist barrier is an Intel-hardware-specific barrier. It's not likely to work well for non-Intel hardware.

Looks good to me.

LGTM

LGTM

LGTM

Okay, that makes sense. THanks! LGTM

Check potential KMP_AFFINITY_SUPPORTED changes mentioned in comments.

LGTM

LGTM

@kkwli0 Other than some clang-format and clang-tidy issues (ignorable), builds look okay, so I think this is ready to try out.

Johnny added overloaded operator new() and operator delete() functions to the two kmp_flag classes that have virtual destructors. He took out the removal of the -Wl,-z,defs flag from the previous patch as well.
He applied it to LLVM and built all of LLVM using clang-12 (with no warnings now and no undefined references), then ran check-openmp and everything was OK.
Kelvin, please wait for the x86 build bot to give a passing result, before attempting to test. Thanks!

Patch notes from Jonathan Peyton: The -Wl,-z,defs flag sent in from LLVM cause undefined references which cause link-time errors in object files due to the new empty virtual destructors. We eliminate this flag for the host OpenMP runtime which eliminates the undefined reference errors. This prevents the host runtime from requiring linking to the standard C++ runtime library.

Kelvin, please try again with this new patch. As of this moment, at least, it applies on top of main branch.

kmp_barrier.h was missing; added back.

This update contains the original patch plus https://reviews.llvm.org/D104638, https://reviews.llvm.org/D104788, and a few more additions to fix the warnings mentioned in D104788.

Added test lines.

Nawrin -- I added a comment about it. I'm hesitant to document that part of this experimental feature in its current form -- it's needed for experimentation at the moment, but is likely to change in syntax or otherwise. It's for having an app use a limited outer level of parallelism, and then library calls into selected libraries can turn on nesting and avoid oversubscription. So, I've left out that option in the commit message, but added the comment to the code, emphasizing that it is an experimental option on an experimental feature.

A few updates that were missed in the original patch were added. Condition fix in the init function, and a topology method check to make use of hwloc if available.

In D102188#2755329, @tlwilmar wrote:

In D102188#2752410, @ye-luo wrote:

Why "12 threads per parallel region" instead of 24? I thought hyper-threads are used by default. Maybe you assume no-SMT in your example? it will be helpful if you make it clear.

Yes, it works that way if SMT is available. So, if there are 2 threads available per core, it would allow for a third nesting level, with size 2.

In D102188#2752325, @JonChesterfield wrote:

There's a bunch of potentially worthwhile topology descriptions we might want. Some systems do big/little cores. Some x64 chips have numa, e.g. the threadripper cores that don't have direct access to dram. There's also the mapping from offload target to cores, where some cores may have a better/direct connection to some GPUs.

N sockets with ~equal chips and lower bandwidth between them seems a good start point. E.g. a recent epyc probably wants to present as 8 top level nodes, each with 8 cores.

In D102188#2752410, @ye-luo wrote:

Why "12 threads per parallel region" instead of 24? I thought hyper-threads are used by default. Maybe you assume no-SMT in your example? it will be helpful if you make it clear.

LGTM

Hi Johnny,
When I just commit via the instructions on our wiki, I just see this separate e-mail labeled "[Diffusion]" which looks like a separate review in Phabricator. When others push their changes, it appears attached to the original phabricator review, and seems to close it automagically. What am I missing there?

Added masked to cons_text_c.

Addressed Nawrin's comment fixes.

LGTM

LGTM

LGTM

One missed change added.

Build problems associated with target attribute for waitpkg should be fixed now. Please try the new diff.

Fixed build problems associated with target attribute for waitpkg.

LGTM

LGTM

LGTM

In D64534#1582486, @Hahnfeld wrote:

@tlwilmar D64625 conflicts with moving the headers. Let me know if you want to land this first, but I'd like to get the fix into the next release which is set to branch next week.

LGTM

LGTM!

LGTM

LGTM

*ping*

LGTM

*ping*

*ping*

*ping*

LGTM

Fix update for non-CG_root threads in __kmp_free_thread.

I was going for the per-data-environment specification for the ICV, though I agree it is only needed per contention group. Is there any consensus on this? Leave as is, or remove per-data-environment ICV?

Sorry Andrey -- I missed the Windows fix. Added now.

Renamed parameters to match 5.0 spec. Added test.

In D55578#1343476, @protze.joachim wrote:

Is the intention to have this ready for the 8.0 release?

LGTM

Tests added.

In D55578#1328082, @jcownie wrote:

Since you're using it twice already, is it worth abstracting the "find the appropriate function using dlsym" logic into a function?
I imagine that there will be other user-interface functions which will also need this, so abstracting it sooner rather than later seems a good thing to do.

In D54342#1313542, @tlwilmar wrote:

In D54342#1313515, @gtbercea wrote:

After this patch, omp_get_num_devices() seems to wrongly return 0 even when devices are present.

OK, on my internal build these are showing up as weak symbols with nm. But on my LLVM build, not. Will look into the LLVM build to make sure it allows for weak attribute.

In D54342#1313515, @gtbercea wrote:

After this patch, omp_get_num_devices() seems to wrongly return 0 even when devices are present.

In D54342#1313417, @ABataev wrote:

This commit conflicts with libomptarget. libomptarget have the same functions and the code may be confused during execution which version of the functions should be executed. This conflict should be resolved somehow. Before that, I think, this patch must be reverted.

LGTM

LGTM