LGTM

@rogfer01 th_task_state is used to refer to one of two task teams that are reused for each hot team. These two task teams exist because of a need to start using a new one while there may still be activity on the old task team, so we flip between the two. Each thread has a th_task_state so it knows which task team is in use for a particular parallel region. When a hot team is expanded in size for a larger parallel region, new threads added to the team need to have their th_task_state initialized to exactly what the other threads in the team are using, so that they are all in sync. So instead, we just initialize from a thread that was in the team before the resize.

ping

LGTM

LGTM

LGTM

LGTM

Thanks Johnny -- made all the fixes you mentioned.

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

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