User Details
- User Since
- Sep 24 2015, 4:45 AM (353 w, 1 d)
Today
Yesterday
Tue, Jun 28
May 16 2022
Are the llvm version variables defined for stand-alone builds? So, will stand-alone builds of OpenMP continue to work and include libomptarget code?
May 3 2022
lgtm now
May 2 2022
Thanks for the fix! I'm quite confident that this change will fix the sporiadic test failures (most probably under high system load).
Apr 29 2022
Apr 6 2022
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!
Apr 1 2022
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-*.
Mar 7 2022
Mar 3 2022
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".
Feb 24 2022
Feb 21 2022
I think in openmp/tools currently only libraries and header files get installed, but no binaries.
Jan 31 2022
Dec 15 2021
Thanks for the update. LGTM.
Can you commit yourself, or should I commit the fix for you?
Dec 8 2021
Just return, if (Data->Parent == nullptr).
Dec 7 2021
lgtm
Nov 22 2021
Nov 17 2021
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.
Nov 16 2021
Nov 12 2021
I mainly checked the libomp code and put some inline comments.
Nov 10 2021
Nov 2 2021
Oct 29 2021
Oct 25 2021
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
Oct 22 2021
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.
Oct 21 2021
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.
start_tool = (ompt_start_tool_t)dlsym(h, "ompt_start_tool");
Oct 19 2021
Please update the diff with context (-U9999)
Please update the diff with context (-U9999)
Oct 18 2021
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
Oct 15 2021
Oct 14 2021
Oct 7 2021
Oct 6 2021
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.
Sep 29 2021
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?
Sep 24 2021
Added several questions inline
Sep 21 2021
lgtm
Sep 10 2021
Aug 30 2021
Aug 28 2021
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)
Aug 23 2021
Aug 19 2021
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.
Aug 14 2021
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.
Aug 13 2021
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.