This patch fixes the issue reported in https://github.com/llvm/llvm-project/issues/54422 for me.

LGTM

In D122667#3417163, @AndreyChurbanov wrote:

The kmp.h is not installed, it is used only for library build. And hwloc library is used by libomp dynamically only if it is accessible at runtime, it is not "linked" with libomp.

Sorry, I was in a hurry:(. Looks like the libomp depends on libhwloc when LIBOMP_USE_HWLOC is true. So @tianshilei is right, we would better use target_link_libraries. Only I am not sure it worth doing in this patch.

In D122667#3417095, @Ericson2314 wrote:

@tianshilei is right. Basically the older CMake stuff like include_directories only worked by side effects, and therefore is impossible to use correctly and modulary and is just generally terrible. the new target stuff is much better, and actually allows tracking who depends on what.

target_link_libraries is definitely a good idea, but I not trying to force @s-sajid-ali to fix the world just to fix the bug :). If we can just get away with target_include_directories that is a good initial step.

@AndreyChurbanov Can you explain

No, it is used for library internals only. And libompd wants to include libomp's internal header, so I am not sure how target_include_directories can apply here.

in a bit more detail? I know CMake well, but not the LLVM openmp stuff.

Is kmp.h that includes hwloc.h installed or not? If it is installed, then what @s-sajid-ali is correct even if the header is not really designed for external use. If it is not installed, then we will have to be more careful about what "target" to use.

In D122667#3416936, @AndreyChurbanov wrote:

Current patch does not work. I think the solution would be to add same conditional include_directories to openmp/libompd/src/CMakeLists.txt.

In D122667#3416686, @Ericson2314 wrote:

In openmp/runtime/src/CMakeLists.txt in particular we have

if(${LIBOMP_USE_HWLOC})
   include_directories(${LIBOMP_HWLOC_INSTALL_DIR}/include)
endif()

The problem is that include_directories just affects the current build, but hwloc.h is being included in a *public* header, right?

No, it is used for library internals only. And libompd wants to include libomp's internal header, so I am not sure how target_include_directories can apply here.

Addressed comments:

• renamed auxiliary function and added comment with its description;
• fixed test to use signaling instead of potentially unsafe delays.

In D119676#3355662, @jlpeyton wrote:

... just for clarification, is it correct that priority==0 means "use the regular per-thread deques" and are assumed to be lowest (and default) priority?

Yes. The description of the priority clause says: "The default priority-value when no priority clause is specified is zero (the lowest priority)."

LG to me as well.
Thanks.

Then the "use omp_lib_kinds" lines can be removed, as they supposed to make
the omp_allocator_handle_kind visible in the interfaces, but probably do not work for all compilers
(or when omp* modules were not built or not accessible).

LGTM

Addressed Ravi's comment.

Please rebase, as the patch cannot be applied currently.

Removed target triple from test compilation.
This eliminates the dependency on libomptarget.

LGTM

LGTM

LGTM

Please rebase before the commit (message catalog has been changed).

In D114083#3176908, @mstorsjo wrote:

@AndreyChurbanov - is this ok with you?

LGTM

In D108574#3138299, @protze.joachim wrote:

I guess the question is, whether we expect more reserved locators will be introduced in the future. Since the spec actually introduced a new section, I thought this might really happen.

The problem with absent documentation can be addressed in a separate patch I think.

In D108574#3080797, @protze.joachim wrote:

Writing up the OMPT interface for omp_all_memory, I realized that treating omp_all_memory individually might not be the best choice, considering that the spec talks about "Reserved Locators".

How final is this compiler interface?

Library recognizes the new type via either
(dependence_address == NULL && dependence_flag == 0x80 (set highest bit of 1-byte flag))
or
(dependence_address == SIZE_MAX).

These are actually two interfaces, and compiler has a choice which one to use.

LGTM

LGTM

LGTM

LGTM

LGTM

In D110109#3089150, @kkwli0 wrote:

@AndreyChurbanov There is one more issue. kmp_atomic_float10_max_min.c specifies -mlong-double-80 but some platforms (e.g. Power) do not support such option.

In D112174#3077754, @protze.joachim wrote:

start_tool = (ompt_start_tool_t)dlsym(h, "ompt_start_tool");

I find the dlsym description confusing:

The function dlsym() [...] return[s] the address where that symbol is loaded into memory.

If the symbol is loaded into memory, how can the address be NULL?

Since the value of the symbol could actually be NULL (so that a NULL return from dlsym() need not indicate an error)

With if (!start_tool) we check the address of the symbol to be !=NULL, not the symbol itself.

If the symbol is NULL, but the function returns the address to the symbol, I would need to dereference the address to see the NULL, i.e. *start_tool == NULL? What do I miss here?

In D110109#3077907, @protze.joachim wrote:

Why are the types in __kmpc_atomic_val_4_cas_cpt different from the types in __kmpc_atomic_val_4_cas? I think, all the functions should use fixed-size integer to respect the width suggested by the name of the function.

In D110109#3077855, @kkwli0 wrote:

Is this patch only applicable to KMP_ARCH_X86 || KMP_ARCH_X86_64 arch? The newly added test cases fail on non-X86 platform due to undefined symbols. The __kmpc_atomic_*_cas_* prototypes got preprocessed out inside the extern "C" hence the names are mangled.

Added assertions on check of __itt_domain_create() result.
Theoretically the function can return NULL pointer that would indicate broken initialization of the ittnotify library.
Though we should check the broken initialization earlier, I added the assertions just in case.

LGTM

LGTM

LGTM