In D73249#1835526, @Hahnfeld wrote:

I'm not sure LIBOMP_OMPT_SUPPORT is guaranteed to be there yet when defined in runtime/. Maybe it would be better to move the option to the top-level openmp/CMakeList.txt and rename it to OPENMP_OMPT_SUPPORT (with compatibility support for LIBOMP_OMPT_SUPPORT)

LGTM

Code LGTM, if possible apply clang-format.

As Johannes, I cannot test this. The code seems reasonable.
Please apply clang-format on the changes.

@hans please include to the 10.0 branch

Agreed

You are right, with the current usage a local variable works.

In D72576#1820159, @tianshilei1992 wrote:

In D72576#1818720, @protze.joachim wrote:

From OpenMP spec perspective I see no issue in returning 0 for the provided code even if there would be devices available, as long as this is consistent with the remaining execution of the application.

As I understand the modification, the function would not return 0 if afterwards a target region will be offloaded to a device?

Actually, no because of empty PendingCtorsDtors and empty target entries.

Ok.

In D70412#1820406, @Hahnfeld wrote:

This revision was not accepted before being committed!

The value merged_if0 is initialized in __kmp_task_alloc from the flags argument:
https://github.com/llvm/llvm-project/blob/master/openmp/runtime/src/kmp_tasking.cpp#L1287

From OpenMP spec perspective I see no issue in returning 0 for the provided code even if there would be devices available, as long as this is consistent with the remaining execution of the application.

This patch re-enables the testing of Archer for all builds where TSan is available.
I'd prefer to have this patch in the release for testing :)

Pushed as 3356e268.

In D70395#1764086, @jdoerfert wrote:

Yes. I was expecting functions but macros seems to be the solution used in this code so far.

Macro outsourced to ompt_specific, which is included by both kmp source files.
Tested for LIBOMP_OMPT_SUPPORT on/off builds.

Implement requested changes.

The conclusion of the discussion in the intel forum thread was, that the call from __kmp_resume to __kmp_suspend_initialize_thread is superflous, because the thread will call __kmp_suspend_initialize_thread always during the own initialization before changing a variable to signal readiness.

In D45890#1756796, @gribozavr2 wrote:

In D45890#1756747, @protze.joachim wrote:

• the tests rely on the availability of thread-sanitizer

The tests should be annotated as REQUIRES: tsan or whatever the appropriate tag is.

In D45890#1756465, @gribozavr2 wrote:

The newly added tests fail on buildbots: http://lab.llvm.org:8011/builders/openmp-gcc-x86_64-linux-debian/builds/236

Could you take a look? And please consider reverting in the meanwhile.

Attached the right diff.

Apply clang-format

@viroulep do you have commit access or should we land that for you?

In D62393#1679250, @ABataev wrote:

You're wrong. My output:

omp_in_parallel=0
omp_get_level=1
omp_get_active_level=0

So, omp_in_parallel works correctly on GPU.

It is probably not directly related to this patch, but I think the logic of omp_in_parallel() is not valid. It only works, because the level counting is also broken. This code

int main(){
#pragma omp target 
  #pragma omp parallel if(0)
    if(omp_get_thread_num()==0) {
      printf("omp_in_parallel=%i\n", omp_in_parallel()); 
      printf("omp_get_level=%i\n", omp_get_level()); 
      printf("omp_get_active_level=%i\n", omp_get_active_level()); 
    }
  return 0;
}

should print (and does so with env OMP_TARGET_OFFLOAD=disabled):

omp_in_parallel=0
omp_get_level=1
omp_get_active_level=0

As I understand the code for omp_in_parallel(), the example code would print omp_in_parallel=1 if the level would correctly be increased.

Removed the counters as requested

In D66885#1651428, @dvyukov wrote:

In this new test, I see the same issue related to race on stack variables as reported here:
https://github.com/google/sanitizers/issues/1134

Does it make the test fail? Or just produces a warning? We could switch to a heap-allocated block or a global.

I derived a test from java_race_pc.cc.

In D66885#1649241, @dvyukov wrote:

Who are "we"? And you would prefer to call them where? ;)

TSAN_OPTIONS="ignore_noninstrumented_module=1" works for us

The reported events look good to me.

Removed the Fortran annotation functions :/
Fixed also the other comments

Addressed comments by Jonas.
The tests also support gcc now.

Removed a file which was mistakenly added

Addressing Jonas' comments

This patch fixes the bug for our usecase.
Thanks!

Are there any plans to add the code for the appropriate OMPT callbacks?

Relicensed Archer under Apache license, updated the files.

Great, thanks!
I like the approach with USE_PRIVATE_TOOL.

I think, a test for ompt_finalize_tool could be something along:

The code looks good to me, thanks for fixing the implementation.

This code does not directly use pthreads, but uses C++11 threads.
Why does the compiler not link the necessary threading library, when using C++11 threads? I suggest to define a lit macro like %cxx11_thread which sets the necessary flags. I guess, that on Linux -fopenmp already implies linking the pthread library.

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