Do we really need this change?
I found a comment from John M-C at https://github.com/OpenMPToolsInterface/OpenMP-4.5-spec/commit/7bed0e0e6b96e0cfa6d964de2101c2172a0a47e7#commitcomment-24561958 (that commit replaced ompt_frame_t to omp_frame_t in the Spec):

LGTM.

LGTM.

This doesn't work, parallel_id=0 for these two events

Implemented changes look good.

LGTM.

Found a small issue with CHECK.
All other LGTM.

In D41942#1001675, @Hahnfeld wrote:

I think if you re-capture a variable with FileCheck that's the same as capturing another variable except that you can't use the old value. So I'm not sure if the changes make any difference, besides naming...

LGTM.

FAIL: libomp :: ompt/misc/control_tool_no_ompt_support.c (66 of 161)

• TEST 'libomp :: ompt/misc/control_tool_no_ompt_support.c' FAILED ****

Script:

C:/src/llvm/build.release/./bin/clang.exe -fopenmp -I C:/src/llvm/projects/openmp/runtime/test -I C:/src/llvm/build.release/projects/openmp/runtime/src -L C:/src/llvm/build.release/bin C:\src\llvm\projects\openmp\runtime\test\ompt\misc\control_tool_no_ompt_support.c -o C:\src\llvm\build.release\projects\openmp\runtime\test\ompt\misc\Output\control_tool_no_ompt_support.c.tmp && C:\src\llvm\build.release\projects\openmp\runtime\test\ompt\misc\Output\control_tool_no_ompt_support.c.tmp

There was no NEEDS line in that file. Does omp50 get defined anywhere? Grepping the OpenMP repository for it yields no matches.

In D42427#987666, @hans wrote:

This works for me if the NEEDS are replaced with REQUIRES.

ompt/misc/control_tool_no_ompt_support.c still fails though:

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.

Intel is saying that they do extensive testing of the runtime - whatever that means on Windows...

Currently we have limited testing of OMPT features on Windows.

LGTM

LGTM.
Verified on some other tests in Intel environment.

LGTM.

Should we add a test for the case ompt_* calls from non-OpenMP thread?

icc bug

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

So maybe @omalyshe can convince the compiler team to add the macros that would simplify life? :-)

Compiler itself somehow cope with it when printing the version. I'll try find out- how.
$ icc -V
Intel(R) C Intel(R) 64 Compiler for applications running on Intel(R) 64, Version 18.0 Build 20171115
Copyright (C) 1985-2017 Intel Corporation. All rights reserved.

LGTM.

In D40083#940557, @Hahnfeld wrote:

In D40083#940524, @omalyshe wrote:

In D40083#940432, @Hahnfeld wrote:

You should still be able to set OPENMP_TEST_C_COMPILER and OPENMP_TEST_CXX_COMPILER as before.

I failed to specify a test compiler with previous LIBOMP_TEST_* staff because I tried -DLIBOMP_TEST_C_COMPILER=gcc but it actually should have been -DLIBOMP_TEST_COMPILER=gcc. Somehow the old macro had _<lang>_ part missed ;)
It works for me now and I believe it will work for OPENMP_TEST_* too.
Thank you!

Yes, I fixed that inconsistency in the previous refactoring. But I'm a bit confused because LIBOMP_TEST_* shouldn't work anymore?

In D40083#940432, @Hahnfeld wrote:

You should still be able to set OPENMP_TEST_C_COMPILER and OPENMP_TEST_CXX_COMPILER as before.

I failed to specify a test compiler with previous LIBOMP_TEST_* staff because I tried -DLIBOMP_TEST_C_COMPILER=gcc but it actually should have been -DLIBOMP_TEST_COMPILER=gcc. Somehow the old macro had _<lang>_ part missed ;)
It works for me now and I believe it will work for OPENMP_TEST_* too.
Thank you!

Why do we limit the test compiler to be the same as the build compiler for libomp?
CMAKE_TEST_<lang>_COMPILER option would be useful to be able to run tests with different compilers.

In D40386#939020, @Hahnfeld wrote:

Question to Intel: At the moment we have to add a new compiler version each year. Can we switch to the same process used for Clang and GCC, i.e. automatically getting the version and composing the string?

Fix for non-hwloc build

In D40384#939079, @sconvent wrote:

In D40384#939025, @Hahnfeld wrote:

In D40384#938762, @omalyshe wrote:

In D40384#938757, @protze.joachim wrote:

@omalyshe Which version of icc introduced the ident information for workshare constructs?

ICC with the ident information has not been released yet

In that case cancel/cancel_worksharing.c should fail for icc-17, icc-18 too?

No, the ident information was only missing for the worksharing constructs. For cancellation constructs, the compiler generates a call to '__kmpc_cancel(...)' with the correct kind parameter. icc-16 however did not generate the correct call.

In D40384#938757, @protze.joachim wrote:

@omalyshe Which version of icc introduced the ident information for workshare constructs?

LGTM.

LGTM.

Added test case for the loop construct with '#pragma omp cancel' followed by the sections construct with cancellation point

LGTM.

In D30450#688282, @Hahnfeld wrote:

Do we really need this? In other words: Is there a use case for not building the OpenMP runtime for the latest standard?

Updated the diff to address Jonathan's comment.

The full diff attached.

LGTM.

LGTM.

LGTM.

LGTM.

Is it OK that ompt_parallel_id_new() is called several times for one region? First in line 393 and then inside kmp_serialized_parallel() it can be called once or twice.

LGTM.

LGTM.

LGTM.

LGTM. Can be committed.

LGTM. Please, commit.

LGTM. Please, commit.