In D128896#3623818, @tianshilei1992 wrote:

I don't understand. Here we all do >> operation. How could it cause overflow?

Are the llvm version variables defined for stand-alone builds? So, will stand-alone builds of OpenMP continue to work and include libomptarget code?

lgtm now

Thanks for the fix! I'm quite confident that this change will fix the sporiadic test failures (most probably under high system load).

This patch LGTM.

I checked the runtime tests and found that openmp/runtime/test/barrier/omp_barrier.c already triggers different combinations of barriers. Do the combinations cover all configurations you tried manually?
Regarding reductions, I think it would make sense to add similar run lines to openmp/runtime/test/worksharing/for/omp_for_reduction.c in order to cover different code paths in the runtime.

I agree, the expected output does not fit the general case of reductions.
As a workaround, I would change the check prefix for reduction 2-n from CHECK to MULTIPLE and add --check-prefixes CHECK,MULTIPLE for the RUN-lines, where multiple pairs of reduction callbacks are expected. So the default would be "at least one" pair of callbacks.

I think, in addition to manually selecting the barrier implementation, we should add additional run lines to specific tests that trigger the different barrier implementations. I added a related, OMPT-specific comment to D123193.

Can you add extra run lines to openmp/runtime/test/ompt/synchronization/reduction/*_reduce.c in order to trigger the different barrier implementations?

Thanks for working on this!

I think, the name of the variable is confusing. The tests triggered and influenced by your example command are not limited to libomp. Currently the LIBOMP_ prefix is mainly used for cmake variables. The make target to run the tests is called check-* rather than test-*.

For the test I suggest signalling like used for OMPT tests (see openmp/runtime/tests/ompt/ompt_signal.h) to enforce the execution ordering rather than sleep "synchronization".

In D119988#3341375, @tianshilei1992 wrote:

I change it back to 256.

I think in openmp/tools currently only libraries and header files get installed, but no binaries.

Thanks for the update. LGTM.
Can you commit yourself, or should I commit the fix for you?

Just return, if (Data->Parent == nullptr).

lgtm

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. During OMPT discussion, Deepak also raised the issue, that these future reserved locators might not be limited to inout/out dependencies.
Therefore, I thought it would make sense to let the flag just indicate that this involves a reserved locator and encode the kind of locator in the address argument.

In D114005#3135157, @dreachem wrote:

What happens for a teams construct? The implicit tasks in that case are all initial tasks. Should this Data->Team->Delete() apply there as well, or only for the implicit parallel region for the whole program?

I mainly checked the libomp code and put some inline comments.

Thanks for the clarification.
lgtm

@jlpeyton can you check with your compiler team, what is wrong with icc 2021? icx works fine.

Tests should not burn CPU cycles. We use my_sleep from omp_my_sleep.h or delay from ompt-signal.h instead to allow other tests make progress in the meantime.

Looks like we missed to update the code in kmp_csupport, when the management of lwt was refactored.
lgtm

In my local test execution I just got:

lgtm

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".

We will finish and discuss the spec ticket during the OMPT call next week. I'll commit after the call - just in case we decide for different names.

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.

Please update the diff with context (-U9999)

Please update the diff with context (-U9999)

I already know, that OMPD folks will not be happy about this implementation. Nevertheless, it is a valid and better implementation than before.

Overall looks good to me besides one inline comment.
I successfully tested with gcc/11, icc/19 and latest clang.

lgtm

The commit is put on hold as discussed in 10/6 OpenMP in LLVM call. Someone from AMD (@dhruvachak ?) will work on upstreaming an alternative implementation, which will most probably result in reverting D99803 as well.

If this test still has issues after the latest patch changing to 100%, basically avg() < 2* expected, we could change to median() < 2*expected. This would effectively ignore the larger half of values.

kmp_ftn_entry.h provides the implementation for all OpenMP API functions. I'm curious to see what happened, that it is not included.

Which platform has the problem? Static build does not preclude linking libdl, which will provide dlsym. So, an alternative solution might be to link libdl for the static library?

Added several questions inline

lgtm

I think, the proper solution is D108868: only add the dependency, if the tools are built at the same time (i.e., the corresponding build targets exist)

In D101960#2960641, @JonChesterfield wrote:

Pasting env LD_LIBRARY_PATH= and env LIBRARY_PATH into the printed commandline, as opposed to through magic, would solve most of my day to day frustration here. libomp.so and libomptarget.so are in two different directories, LD_LIBRARY_PATH turns out to be colon delimited. LIBRARY_PATH is presently used to find the deviceRTL though I'd like to change that.

During further testing. I found that nvptx-new had a similar issue. I also had to move the depend code in the runtime cmake file, but I could verify in the ninja.build file, that this change now really adds the expected necessary dependency.

In D108404#2955774, @JonChesterfield wrote:

Thank you very much for getting to the bottom of this! One to add to the llvm-13 branch too I think

The reason for using ignore_noninstrumented_modules=1 is the general interception of pthread / libc functions.
If the instrumentation pass would name shift the functions during compilation, turning off the analysis for non-name shifted functions, i.e., function calls from non-instrumented code, could be done at runtime without looking at the caller.

In D108046#2944952, @dvyukov wrote:

Semantically I could also annotate Mutex semantics. But, from my understanding this introduces HappensBefore edges into the analysis, where I don't want to have them. They would hide even more data races than what I try to solve.

I see. This makes sense.

Re performance, what matters more now is performance of the new runtime that is slowly being upstreamed in pieces:
https://github.com/dvyukov/llvm-project/pull/3

Going back to the branch instead of the multiplication. Doesn't seem to have a performance impact.

I successfully removed the performance regression from the patch.