Since there is only one other ICV which might be handled similary (bind-var), I will not further argue for making a special case for thread-limit-var. So leave it as it is.

LGTM, thanks for catching this!
The output is written to the build dir anyways:

Please provide test cases, so that we can understand the impact of this change.

I'm a bit confused about the explicit copy per task.

The code is updated to be compatible with OMPT as in OpenMP 5.0.

In D55725#1370148, @jdenny wrote:

• libgomp.so, libiomp5.so: My understanding is that these symlinks exist solely for backward compatibility. This patch currently doesn't affect them (doesn't bother to install them to Clang-dedicated directories). Any reason to change that?

The patch in general looks good to me.

Updated as requested

Fix the last mentioning of ompt.h

Is this good to go for 8.0?

Implemented changes by Hansang.

Does the observation in https://bugs.llvm.org/show_bug.cgi?id=36772 match your expectation regarding the remaining race?

• I activated the omp_get_initial_device() call to get the number at runtime.
• I also changed the name of ompt.h to omp-tools.h as defined in the SPEC

Implemented in r349458.
Missed to include the review number in the commit message

LGTM

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

Implemented most changes requested by Hansang

This will not compile with the current version of the OMPT interface, needs updates first

LGTM

LGTM

In D51687#1230375, @gtbercea wrote:

Considering your comment in the description about requiring latest Clang perhaps you should revisit this patch: D46842

LGTM

Was hold back from 7.0, to keep the implementation according to TR6.

@Hahnfeld and I discussed the behavior and found a very inconsistent behavior. Let's consider something like the following code, under the constraint, that device(1) is busy (will fail to offload) and device(0) is free:

Thanks for catching this. It seems like we need some Fortran tests for OMPT.
LGTM

In D50522#1194805, @AlexEichenberger wrote:

So you like deciding available on first use? This is what your comment seems to imply, but I am not 100% sure.

LGTM

I second this change, since this should hopefully allow to include OpenMP in the release build, until someone can fix the build of OMPT on Windows.

I would like to commit this patch before the 7.0 branch.
Changing the name of the bit can be done in a later patch.
@jmellorcrummey do you agree, can you remove your request for changes?

I apply changes requested by hbae on commit.

LGTM

I apply suggested changes by hbae on commit.

The change is symetric with what is implemented for cuda.
LGTM

Lgtm

LGTM

LGTM

As discussed on the mailing list, the flag should only be dropped on Mac OS. My pragmatic solution would be:

In D47717#1135481, @jmellorcrummey wrote:

I think we should change the name of the bit ompt_enabled.enabled to ompt_enabled.activated. That would make joachim’s distinction clear. I was commenting on the setting of OMP_TOOL=enabled. There is a difference between enabled and activated.

The OMPT tool can decide at multiple points to be inactive, here we look at:

The problem, which gets visible in this test case is the use of __builtin_frame_address(1), which is documented to be not safe.
Is there a better way to get the canonical frame address of the calling function? Also the address returned by __builtin_frame_address seems to be different from the canonical frame address. How can we get the requested address?

taskloop is actually a tasking construct and explicitly no worksharing construct. So, please move the test in the task directory.

LGTM

@dvyukov thanks for the detailed reasoning.

Result of discussion in Tools WG was that the spec will keep omp_frame_t. I will submit this patch together with D43568

LGTM

Reasons to keep the name archer (or archer-rt):

• We already have that name, we don't need to come up with a new name :)
• You can easily find related publications under that name, which explain the reasoning behind the library
• The programmer should never see the name, once the tool is integrated into the runtime workflow

I added a testcase. I added it just for Linux, because I have no machine ready to test dl-loading on other OS.

I updated this differential to only export the function.

LGTM.

The idea was that the WAIT in line 11 should ensure that both initial threads arrived.
But actually the runtime is not initialized before line 11. To fix the race, we need to call into the runtime in a way, that makes both threads initial threads before the SIGNAL in line 9.

LGTM

LGTM

As far as I know, the current implementation is very close to the specification in TR6.
Here we have a tiny diff between the spec and the implementation. I agree that we should not apply this patch if we will roll back the change in the spec.

Use the captured pattern as pointed out by Olga

The initial issue in this patch is resolved by D43195. So removed the additionally printed address for AARCH64, but still allow the testcases to match any printed address.

I applied clang-format on commit.

This behavior is right (p.417,l.5):

ok

NEEDS must be REQUIRES