- User Since
- Sep 24 2015, 4:45 AM (261 w, 6 d)
Fri, Sep 25
I still see some clang-specific and system link directories listed in the linker line before the directories from LIBRARY_PATH:
Thu, Sep 24
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?
Wed, Sep 23
Removed accidently added printf from a test.
Mon, Sep 21
LGTM, the relevant OMPT tests pass with this patch
Can you add a test?
Sun, Sep 20
LGTM as well.
Tue, Sep 15
With this patch, I would expect runtime/test/ompt/tasks/dependences_mutexinoutset.c to succeed. Running the test with gcc-9/10, this test reports the dependency as ompt_dependence_type_inout and not ompt_dependence_type_mutexinoutset. Since the OMPT interface just translates the internal values, I assume something is wrong with the flags of the dependency.
Sun, Sep 13
The GOMP wrapper makes sense to me.
Several inline comments for the test.
Sat, Sep 12
Mon, Sep 7
Thanks for submitting this patch!
Aug 20 2020
Since I spent hours to hunt down several race conditions in libomp in the last months, please fix races immediately, when they are pointed out. There is no such thing as a benign race!
Aug 18 2020
Aug 16 2020
Not sure whether greg still plans to work on this. Anyway a inline comment to the PATH_MAX
Aug 14 2020
Aug 7 2020
Aug 5 2020
Jul 31 2020
Jul 29 2020
Updated to be a bit more specific, where the external version is used.
Jul 28 2020
Thanks for reviewing.
Jul 26 2020
You are basically right. In implementation, a function is generated for every mapper to do all internal mapping. More details can be found at https://github.com/lingda-li/public-sharing/blob/master/mapper_runtime_design.pptx
stderr is unbuffered by default. Are we sure this behavior is not changed when the output is redirected into a file, as for example during execution in a batch job? What were the worries about having the fflush there?
Jul 25 2020
From my perspective, the declare_mapper_target.cpp code is semantically equivalent to:
Jul 24 2020
I included this change in D84557, where I enable nvptx64 testing for most libomptarget tests.
afa1afd4108d changed the minimum required version in all subdirectories, including openmp
The cmake version was bumped to 3.14
I carefully made sure, that the freshly built clang was used to execute the test. I opened https://bugs.llvm.org/show_bug.cgi?id=46836 to track the issue and made it release blocker.
This patch breaks compilation of previously working code.
I'm looking through the tests in this directory and try to run them on a GPU.
I'm not enough invested in the compiler implementation to judge this patch.
I can confirm that this patch fixes the segmentation fault I have seen.
After harbormaster highlighted the issue, this is the patch to solve the issue.
Jul 23 2020
Jul 22 2020
$ /usr/bin/python bin/llvm-lit -vv -a projects/openmp/libomptarget/test/offloading/target_depend_nowait.cpp
"./bin/clang++" "-fopenmp" "-pthread" "-fno-experimental-isel" "-I" "../openmp/libomptarget/test" "-I" "./projects/openmp/libomptarget/../runtime/src" "-L" "./lib" "-fopenmp-targets=x86_64-pc-linux-gnu" "../openmp/libomptarget/test/offloading/target_depend_nowait.cpp" "-o" "./projects/openmp/libomptarget/test/offloading/Output/target_depend_nowait.cpp.tmp-x86_64-pc-linux-gnu"
For the commit of this patch, the test fails with and without env LIBOMPTARGET_DEBUG=1. I'm using a release build, but have -DLIBOMPTARGET_ENABLE_DEBUG=on. This allows to activate debug output by setting the env variable.
The other tests failed after the commit. They started to succeed with various commits.
At above mentioned commit, only this single test fails.
Starting with the commit of this patch, the libomptarget test llvm-project/openmp/libomptarget/test/offloading/target_depend_nowait.cpp fails. Here is a stacktrace of the segfault:
$ gdb target_depend_nowait.cpp.tmp-x86_64-pc-linux-gnu (gdb) run ... Program received signal SIGSEGV, Segmentation fault. (gdb) bt #0 0x0000000000400fcf in .omp_outlined._debug__ (.global_tid.=0x2aaab96b9be0, .bound_tid.=0x2aaab96b9bd8) at target_depend_nowait.cpp:22 #1 0x0000000000401e8d in .omp_outlined..23 (.global_tid.=0x2aaab96b9be0, .bound_tid.=0x2aaab96b9bd8) at target_depend_nowait.cpp:18 #2 0x00002aaaab574213 in __kmp_invoke_microtask () from libomp.so #3 0x00002aaaab51338e in __kmp_invoke_task_func () from libomp.so #4 0x00002aaaab5126bf in __kmp_launch_thread () from libomp.so #5 0x00002aaaab55d3d0 in __kmp_launch_worker(void*) () from libomp.so #6 0x00002aaaabbd6e65 in start_thread () from libpthread.so.0 #7 0x00002aaaabee988d in clone () from libc.so.6
declare_mapper_target.cpp still fails for me consistently in RUN line 2, so for the nvptx version. I execute on x86_64 with Tesla V100 and cuda 10.0.
When I execute the test with export LIBOMPTARGET_DEBUG=1, the test succeeds.
In case of failure, the test prints Sum = 1024, in case of success, the test prints Sum = 2048 as expected.
Jul 21 2020
Thanks for the update, lgtm
Sorry, I was looking at the date and thought the commit was 2 days old instead of 1 year :)
You are right, the test started to fail after the D67833 commit.
@DavidTruby please change the default to false and I'll accept the patch.
Jul 20 2020
What would be needed to remove this strange copying step? Do the tests really rely on these directories?
I could run the tests successfully with cmake -DLIBOMP_COPY_EXPORTS=FALSE.
Jul 18 2020
The test fails for me with segfault in line 22.
Jul 17 2020
On a different system, I also get warnings for CMP0074.
The two policies were introduced with cmake 3.12. As long as we don't bump the required cmake version to at least 3.12, we need to set the policies.
Jul 16 2020
I think, the better solution is to use the _pc version of the access functions to use the "real" access location.
Jul 15 2020
I reverted the commit, because all buildbots failed for these tests:
TEST 'ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.RaceWithOffset' FAILED TEST 'ThreadSanitizer-Unit :: rtl/./TsanRtlTest-x86_64-Test/ThreadSanitizer.RaceWithOffset2' FAILED
Jul 14 2020
After a pull from master, all tests were successful with this patch.
I added a missing a return statement
I agree, that the patch changes the transitive suppression of reports. Unfortunately, this would bring back the overhead of reconstructing the remote stack trace.
I moved the loops into functions.
I think for the previous controlflow, AddRacyStacks was necessary because of the early return false in line 475. I removed it, as the functions now add the entry directly to the vector.
Jul 11 2020
Jul 7 2020
Jul 6 2020
Jul 5 2020
Addressed @Hahnfeld 's comments
Jul 4 2020
Jul 3 2020
I have no idea, what Harbormaster is trying to tell me.
Jul 2 2020
In other tests we start gcc versions with gcc-4.
I added those flags for the task reduction case without explicitly testing gcc-4/gcc-5
Sorry, in my previous update I missed to include the other changes (diff to the wrong commit)
Does clang-11 now default to 50 behavior?
In this case, we can remove -fopenmp-version=50 from the tests, right?
My comment was just an observation based on the code modification. I have not much understanding of the implications of this flag other than the few lines I read in the documentation and the comparison @tianshilei1992 posted.
The intention was to check the return address for the first two tasks. Unfortunately, the CHECK did not capture the return address of the second task creation, but instead compared to the return address of the first task.
I found another issue with this test (addressing Saiyedul's error). I'll post an updated patch in a minute
I think D82267 should fix the issue for dependence.c