In my quick test, env KMP_SETTINGS=1 OMP_PROC_BIND=spread ./a.out reports OMP_PLACES='cores'. I could not find the code path that sets places to cores in this case, but I think it would be more consistent to have consistent setting in the unset and the invalid case.

LGTM as well.

I found two issues with this patch regarding the default clause:

• The spec does not require a default clause. I get error: expected expression if I omit a default clause. The error is gone if I add default().
• The spec does not allow an empty default() clause, but rather expects default(nothing)or omission of the default clause. This patch accepts the empty default clause.

LGTM, and should work for the runtime tests.

In D93738#2476692, @tianshilei1992 wrote:

Building runtimes is not part of building LLVM. It is different from building OpenMP using CMake argument LLVM_ENABLE_PROJECTS .

If I build llvm-project using an old compiler, the dependencies are necessary in order to have clang built before running the OpenMP tests. Dropping the dependencies might cause issues in parallel builds, but will make the check-openmp targets incomplete.
What is the problem in having the dependency there?

Please consider the special cases of task_if0, which preferably should use __kmpc_omp_task_with_deps:

I'm surprised to find no dlclose matching the dlopen. Instead of calling some function for init/destroy, can't we just use library constructor/destructors in the plugin? All MemoryManagers for a plugin should then be destroyed before the plugin is explicitly dlclosed.
I'm also surprised that LoadRTLs does not dlclose the library in case of missing symbols.

As discussed in yesterdays call, I marked the use of all __kmpc_ functions as deprecated and moved the declaration of those __kmpc functions into a new header file.
I also removed most OMPT-related changes from this patch and will open a separate differential for those. Therefore several OMPT tests fail after this patch.

In D92197#2425687, @jdoerfert wrote:

I'm not convinced. I didn't go over everything but I think we should discuss the goal and choices here first.

I checked the OmptReturnAddressGuard and it looks like it is a noop to store a return address if one is set already, right? (assuming same thread)
Could we unconditionally execute OMPT_STORE_RETURN_ADDRESS(gtid); instead to avoid the _aux stuff?

Replace the KMP_ASSERT by KMP_DEBUG_ASSERT.
Fixing several corner cases

In D91692#2417797, @mstorsjo wrote:

This broke building with OMPT disabled, but I went ahead and pushed a fix for that in 6b429668ded1aa7de4d586fc6dc8a469799b4d9c, by moving OmptReturnAddressGuard to inside the OMPT_SUPPORT ifdef.

Also include OMP_TOOL_VERBOSE_INIT in the output for OMP_DISPLAY_ENV as requested.

Use macros for the conditional verbose output.
We also added a test for the archer-specific output.

The tests have the same format type issue as in D90967. Feel free to include the fixes for the calloc tests in this review.

Testing with cmake . -DOPENMP_TEST_FLAGS="-Werror -Wno-#warnings" && ninja check-libomp breaks for the newly added tests.

I added run lines to the tests with race, to verify that the races are still detected, if ignore_serial=1 .

I acknowledge the advantage of switch statements, in that they will warn on missing cases in the future. So, I went back to the switch statements.

In D90752#2390115, @lxfind wrote:

Are all uses of on_ompt_callback_master expected to be replaced?
I see this is still using it:
https://github.com/llvm/llvm-project/blob/master/openmp/tools/multiplex/tests/custom_data_storage/first-tool.h#L126

The ompt_callback_master_t type is deprecated, but not removed. So, omp-tools.h still needs to define the type.
I moved the existing master.c test to masked.c and added a new master.c test, which just tries to register the deprecated callback.

Please revert your commit while you work on a fix, as it breaks building the runtime code.

Can we have tests for ptr = NULL, size=0? Probably just replace alloc and free by these cases and check returned address after each call?

Original intention was to have if(0) on the first task. Unfortunately, clang drops the out dependency for detached tasks in that case: https://bugs.llvm.org/show_bug.cgi?id=46185
Once the clang bug is fixed, we can add the if(0) and make the test more robust.

This implies also some changes to the OMPT tests

Pushed as e99207feb4b901e8f7bb6d3e70388d31fafc4330, missed to include the Revision line.

I tested this with older clang releases (at least back to clang 9.0) and could reproduce the assertion. The error doesn't seem to be related to this patch, but the test just reveals the issue.

The test asserts for x86 offloading:

lgtm

The test started to fail recently

Fixed handling of ompt_task_cancel.

This will break many cases, where GOMP or even kmp functions call external kmpc functions rather than only internal kmp functions. In such case, the return address will point to the runtime function.
The idea of the macro is to ensure that only the first entry to the runtime will set the address.
Before we can make this change (and I would be more than happy about this change ;), we need to make sure, that no runtime code calls the external __kmpc interface (at least, if the external function tries to store the return address).

lgtm

ENABLE_LIBOMPTARGET determines the initial default value for OPENMP_ENABLE_LIBOMPTARGET. Therefore the variable is only relevant in the first cmake run, but has no effect in successive cmake or ccmake configures. It will never overwrite the value of OPENMP_ENABLE_LIBOMPTARGET.

In openmp/CMakeLists.txt there is ENABLE_LIBOMPTARGET to decide about the default value for the CMake option OPENMP_ENABLE_LIBOMPTARGET. This way libomptarget gets disabled on apple and win32 by default.

Will it still be possible to build the OpenMP runtime without LLVM headers/sources? Can we disable libomptarget if LLVM headers are not available?

I still see some clang-specific and system link directories listed in the linker line before the directories from LIBRARY_PATH:

Removed HOST_DEVICE definition from the header as suggested in the reviews.

With the OMPT tests succeeding, this patch looks good to me.

Can you push the patch or should I?

Removed accidently added printf from a test.

LGTM, the relevant OMPT tests pass with this patch

Can you add a test?

LGTM as well.