User Details
- User Since
- Sep 24 2015, 4:45 AM (278 w, 23 h)
Yesterday
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.
Mon, Jan 18
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.
Thu, Jan 7
LGTM, and should work for the runtime tests.
Mon, Jan 4
Sun, Jan 3
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?
Dec 17 2020
Please consider the special cases of task_if0, which preferably should use __kmpc_omp_task_with_deps:
Dec 10 2020
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.
Dec 5 2020
Dec 4 2020
Dec 1 2020
Nov 30 2020
Replace the KMP_ASSERT by KMP_DEBUG_ASSERT.
Fixing several corner cases
Nov 29 2020
Nov 26 2020
Nov 25 2020
Nov 22 2020
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.
Nov 18 2020
Nov 17 2020
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.
Nov 16 2020
Nov 13 2020
I added run lines to the tests with race, to verify that the races are still detected, if ignore_serial=1 .
Nov 12 2020
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.
Nov 11 2020
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.
Nov 10 2020
Please revert your commit while you work on a fix, as it breaks building the runtime code.
Nov 9 2020
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?
Nov 5 2020
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.
Nov 4 2020
This implies also some changes to the OMPT tests
Pushed as e99207feb4b901e8f7bb6d3e70388d31fafc4330, missed to include the Revision line.
Nov 3 2020
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.
Nov 2 2020
The test asserts for x86 offloading:
lgtm
The test started to fail recently
Oct 30 2020
Oct 29 2020
Fixed handling of ompt_task_cancel.
Oct 27 2020
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).
Oct 21 2020
lgtm
Oct 16 2020
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?
Oct 5 2020
Sep 30 2020
Sep 25 2020
I still see some clang-specific and system link directories listed in the linker line before the directories from LIBRARY_PATH:
Sep 24 2020
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?
Sep 23 2020
Removed accidently added printf from a test.
Sep 21 2020
LGTM, the relevant OMPT tests pass with this patch
Can you add a test?
Sep 20 2020
LGTM as well.