RWTH Aachen University
- User Since
- Apr 2 2015, 4:52 AM (261 w, 1 d)
Mon, Mar 30
@thakis thanks for taking care so quickly!
Thu, Mar 26
Wed, Mar 25
Yes, this looks related to memory consistency. As far as I understand the threads synchronize on th_spin_here, so this is guaranteed to be updated. Any other write before this in __kmp_release_queuing_lock is not guaranteed to be synchronized by a weak memory model. This includes th_next_waiting (which triggers the assertion), but also writes by the user application. That's particularly bad because this should be taken care of by the runtime!
Sat, Mar 14
Fri, Mar 6
Feb 25 2020
Feb 11 2020
Feb 8 2020
This should have a RFC on openmp-dev. As far as I understand the linked patch, the aim is to use std::make_unique which is just syntactic sugar IMO. Unless there are good reasons to switch, I'd object because it requires newer versions of compilers than many HPC systems have.
Feb 3 2020
LGTM from the code, I didn't test that the issue is indeed fixed on i386 though
Feb 2 2020
@protze.joachim for 64 bit, we still need at least 5 threads IIRC
Jan 29 2020
As a general comment, I think this patch should be split into at least two separate ones:
- Load plugins from the same directory as libomptarget.so is found.
- Quick fail mechanism, although I'm not sure how much this saves? In most cases, we won't have the plugins for a foreign architecture (PPC on x86) anyway, that might only be a concern for accelerators.
Jan 23 2020
I'm not sure LIBOMP_OMPT_SUPPORT is guaranteed to be there yet when defined in runtime/. Maybe it would be better to move the option to the top-level openmp/CMakeList.txt and rename it to OPENMP_OMPT_SUPPORT (with compatibility support for LIBOMP_OMPT_SUPPORT)
Jan 16 2020
Jan 15 2020
Jan 14 2020
This revision was not accepted before being committed!
Jan 6 2020
Can you please also submit a patch to libc++ where we copied this code from?
Dec 31 2019
Dec 30 2019
Yes, this must not be changed for compatibility reasons. If this really brings a benefit, we need new entry functions and update the compiler(s).
Dec 11 2019
Dec 7 2019
Dec 6 2019
Dec 2 2019
Nov 29 2019
Nov 19 2019
Random drive-by comments to parts of the patch, I won't review in full.
Nov 16 2019
Testing of openmp is currently very lacking, but I don't think this change will apply cleanly anymore...
Going through my old revisions, is this still something that we want?
I won't continue work on this.
Nov 5 2019
Heads up that this change broke compatibility with old binaries. Not sure if that is something worth to fix, but it should probably at least be spelt out somewhere...
Sep 28 2019
Sep 11 2019
The new test is failing for me because CHECK1 is not satisfied. Instead the line says MERGE-OUTER: 3 new files with 12 new features added; 11 new coverage edges (instead of 11 new features). I'm currently investigating what's wrong here, let me know if you have an idea.
LG iff the following is correct: The leak is when dyn_cast can't cast and returns null, right? If so, please add into the summary (and do so for future revisions, it will speed up review).
Sep 10 2019
The code itself looks good to me (it should probably be clang-formated). If others have comments, please raise them now.
Sep 8 2019
For the record, I don't think this would have worked: I do have libstdc++ available on my system (I just choose SANITIZER_CXX_ABI=libcxxabi for other reasons) and passing -stdlib=libstdc++ when building the test doesn't help as I've already said in D67298. Moreover, there's nothing in the test that requires libstdc++, it just happens to pass due to many chained implications, and restricting a test without understanding the root cause is probably no good idea.
Sep 7 2019
I think I just figured out that the original problem is about static runtime, so it doesn't make sense to run the test with dynamic asan.
Sep 6 2019
I've posted a change that makes the test work for me in D67298, please take a look.
Sep 4 2019
Also, there should be a summary of the changes in here.
Sep 3 2019
Sep 2 2019
Aug 31 2019
This changes error messages, so I'd say it's not NFC.
Aug 30 2019
Aug 15 2019
LG, thanks for the changes.
Please submit the test changes unrelated to the code changes in a separate patch!
The code changes look good to me, but the test doesn't pass on x86. We've faced the same problem when clang-offload-bundler was initially committed and the current testing is the best we were able to do.
Can we close this after D65341 landed?
Aug 14 2019
Okay, so I wasn't happy with the current explanations because I don't like "fixing" an issue without understanding the problem. Here's a small reproducer:
$ cat main.cpp #include "test.h"
Jon, please subscribe to openmp-commits so that commit emails get through immediately. Thanks!
Aug 13 2019
Change check_c_compiler_flag() to check_cxx_compiler_flag().