Remove all older OMP spec versioning from the runtime and build system.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Great cleanup, one very minor comment inline.
runtime/CMakeLists.txt | ||
---|---|---|
72โ73 โ | (On Diff #209062) | A quick grep shows only one usage in omp_lib.{f,f90,h}.var where we can actually hardcode the version. (This would finally mean that the .var files only access constant variables like LIBOMP_VERSION_* and we could avoid replacing them at configure time. But this can be cleaned up in future patches...) |
How does this play with -fopenmp-version= clang switches;
This doesn't remove any public api, only makes all of it available "unconditionall", correct?
I think this is a good thing in the end, but this warrants a little
bit more prominent announcement/RFC beforehand, on openmp-dev+llvm-dev.
AFAICT, yes. Plus -fopenmp-version doesn't do much, I'm only aware of different treatment for one corner case in addition to changing the value of _OPENMP.
I think this is a good thing in the end, but this warrants a little
bit more prominent announcement/RFC beforehand, on openmp-dev+llvm-dev.
There was a thread on openmp-dev: http://lists.llvm.org/pipermail/openmp-dev/2019-February/002356.html
I don't agree that this should be cross-posted to llvm-dev, everyone caring about OpenMP should be subscribed to openmp-dev. Furthermore, building the latest version was more or less the default, except you keep your CMake configuration over many years.
Ok.
Plus -fopenmp-version doesn't do much, I'm only aware of different treatment for one corner case in addition to changing the value of _OPENMP.
That does not sound right, it certainly results in different sema (see also: gcc9 force-bumping to openmp4),
and i sure hope it disables features that aren't in that spec version.
I think this is a good thing in the end, but this warrants a little
bit more prominent announcement/RFC beforehand, on openmp-dev+llvm-dev.There was a thread on openmp-dev: http://lists.llvm.org/pipermail/openmp-dev/2019-February/002356.html
Would be a great idea to at least bump it, with the link to the patch.
I don't agree that this should be cross-posted to llvm-dev, everyone caring about OpenMP should be subscribed to openmp-dev.
Furthermore, building the latest version was more or less the default, except you keep your CMake configuration over many years.
Yes
runtime/CMakeLists.txt | ||
---|---|---|
72โ73 โ | (On Diff #209062) | Jonas, Or am I missing something? Thanks, Andrey |
runtime/CMakeLists.txt | ||
---|---|---|
72โ73 โ | (On Diff #209062) | My reasoning was that we don't need to replace variables in include/ anymore because no value depends on the CMake configuration. I didn't think about raising the value once a new spec is released, good point. I'd still argue that this is pretty rare (every 2-3 years based on the recent history), but I acknowledge that inconsistency between the different files is bad. I'm fine with leaving the variable for now and possibly revisit the topic later on. Thanks! |
runtime/CMakeLists.txt | ||
---|---|---|
72โ73 โ | (On Diff #209062) | OK, if I can get the patch accepted soon, Johnny might be able to get it checked in today. Thanks! |