RWTH Aachen University
- User Since
- Apr 2 2015, 4:52 AM (285 w, 6 d)
Thu, Sep 17
Thu, Sep 3
Jul 8 2020
Jul 7 2020
This is definitely not NFC and breaks API compatibility (but apparently nobody cares anymore?).
Jul 6 2020
Jul 5 2020
The summary also talks about tools/multiplex/tests/*, is that information obsolete?
Jul 2 2020
Jun 29 2020
(In general, all patches must be sent to the respective -commits list. This also makes feedback more likely.)
Jun 19 2020
LIBOMP_HEADERS_INSTALL_PATH is the internal compiler where for example omp.h. I think the header tool is of relevance outside of Clang, so IMO it should go the general include/ directory.
May 18 2020
I don't feel comfortable reviewing this change, maybe @phosek can take a look.
May 15 2020
Does -fopenmp-version work with the Intel Compiler? How will this work if the GCC eventually gets support for OpenMP 5.0?
Apr 21 2020
LGTM as well
This patch makes not visible in all of the openmp project's test
suites. In all but libomptarget/test, it should be possible for a
test author to insert not before a use of the lit substitution for
running a test program.
Apr 10 2020
Apr 9 2020
Mar 30 2020
@thakis thanks for taking care so quickly!
Mar 26 2020
Mar 25 2020
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!
Mar 14 2020
Mar 6 2020
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!