Page MenuHomePhabricator

Remove OMP spec versioning
ClosedPublic

Authored by tlwilmar on Wed, Jul 10, 2:46 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

tlwilmar created this revision.Wed, Jul 10, 2:46 PM

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...)

@tlwilmar D64625 conflicts with moving the headers. Let me know if you want to land this first, but I'd like to get the fix into the next release which is set to branch next week.

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.

How does this play with -fopenmp-version= clang switches;
This doesn't remove any public api, only makes all of it available "unconditionall", correct?

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.

How does this play with -fopenmp-version= clang switches;
This doesn't remove any public api, only makes all of it available "unconditionall", correct?

AFAICT, yes.

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

@tlwilmar D64625 conflicts with moving the headers. Let me know if you want to land this first, but I'd like to get the fix into the next release which is set to branch next week.

OK, I will try to get your suggested change above into the patch today.

runtime/CMakeLists.txt
72–73 ↗(On Diff #209062)

Jonas,
Could you clarify a bit why hardcoding values in 3 files can be better than having values in a single place? We will need to change versions once support for new specification implemented, and doing this in one place looks preferable over changing constants in 3 files.

Or am I missing something?

Thanks, Andrey

Hahnfeld added inline comments.Fri, Jul 12, 7:06 AM
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!

tlwilmar marked an inline comment as done.Fri, Jul 12, 1:08 PM
tlwilmar added inline comments.
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!

This revision is now accepted and ready to land.Fri, Jul 12, 2:43 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jul 12, 2:45 PM