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

NEEDS replaced by REQUIRES

It was defined in https://reviews.llvm.org/D42432

In D42427#987710, @omalyshe wrote:

In D42427#987692, @protze.joachim wrote:

@omalyshe do you have any advice, what is needed to make the omp_control_tool symbol available on Windows?

My naive approach was to follow the things done for omp_get_thread_num.

Adding omp_control_tool to dllexports is a right way.

LGTM.

@omalyshe do you have any advice, what is needed to make the omp_control_tool symbol available on Windows?

Assuming we have pthread on all platforms but Windows

In D38185#985047, @hans wrote:

Also, r317085 doesn't seem to have gone to any mailing list, which is unfortunate. Maybe because the committer was not subscribed and it got stuck in moderation.

LGTM now

Still surprised that this is necessary for void function calls, but with that change the tests also succeed for intel compiler.

We discussed to relax the wording in the OpenMP spec, so that any implementation that provides ompt_mutex_impl_unknown with value 0 is complying. This will include #define, enum or "static const" values.

Ah, I think I understand your plan now.

LGTM

LGTM

In D41817#973066, @pawosm01 wrote:

Library built with: Clang  | GCC
Testing with:              |
Clang               works  | fails
GCC                 fails  | fails

Ok, I think, I now understood the change.

@omalyshe AFAIK this issue is Case#: 03138964

I would have expected, that

printf("%" PRIu64 ": &lock: %lu\n", ompt_get_thread_data()->value, (unsigned long) &lock);

would also fix the issue.

As I understand the issue, this is not a fault in the test code, but the value provided by parallel-begin is wrong? In that case, I suggest to pass NULL for this architecture/compiler.

In D41817#969877, @pawosm01 wrote:

I didn't even know these tests are allowed to be compiled with anything else than clang 6.0+...

Updated the README

Updated the README

Removed 32 bit ARM from the list of supported architectures until it is tested.

In D41165#963515, @Hahnfeld wrote:

Does the latest change assume current_task == new_taskdata->td_parent (which would make sense to me)? If not this change has more implications than the frame address.

I added the requirement for hardware architecture support to LIBOMP_HAVE_OMPT_SUPPORT.
This way one would also get an error when manually turing LIBOMP_OMPT_SUPPORT=on on an unsupported architecture.

In D41182#955383, @omalyshe wrote:

icc bug

What bug it is and which version of ICC is affected?

In D41165#960531, @Hahnfeld wrote:

This fixes the test with GCC which already had the enter address, right?

I think, the address needs to be reset on return. Please include commit 45a3a39 to this patch.

This change is more a performace optimization. (reset the bit in the bitmap to 0)
The callback was set to NULL before and all callback invocation checks for the NULL-pointer.

Thanks

In D40386#940428, @Hahnfeld wrote:

For released versions (at least for the current way of defining the macros) the following should be possible (not tested):

#define KMP_COMPILER "Intel C++ Compiler " stringer(__INTEL_COMPILER / 100) "." stringer(__INTEL_COMPILER % 100) "." stringer(__INTEL_COMPILER_UPDATE)

Maybe you need to assign the temporary values to other defines for the macro magic to work, I'm not too familiar with how it works...