[Openmp-commits] [PATCH] D64534: Remove OMP spec versioning

[Terry Wilmarth via Phabricator via Openmp-commits]

tlwilmar marked an inline comment as done.
tlwilmar added inline comments.


================
Comment at: runtime/CMakeLists.txt:72-73
 libomp_check_variable(LIBOMP_LIB_TYPE normal profile stubs)
-set(LIBOMP_OMP_VERSION 50 CACHE STRING
-  "The OpenMP version (50/45/40/30)")
-libomp_check_variable(LIBOMP_OMP_VERSION 50 45 40 30)
 # Set the OpenMP Year and Month assiociated with version
-if(${LIBOMP_OMP_VERSION} GREATER 50 OR ${LIBOMP_OMP_VERSION} EQUAL 50)
-  set(LIBOMP_OMP_YEAR_MONTH 201611)
-elseif(${LIBOMP_OMP_VERSION} GREATER 45 OR ${LIBOMP_OMP_VERSION} EQUAL 45)
-  set(LIBOMP_OMP_YEAR_MONTH 201511)
-elseif(${LIBOMP_OMP_VERSION} GREATER 40 OR ${LIBOMP_OMP_VERSION} EQUAL 40)
-  set(LIBOMP_OMP_YEAR_MONTH 201307)
-elseif(${LIBOMP_OMP_VERSION} GREATER 30 OR ${LIBOMP_OMP_VERSION} EQUAL 30)
-  set(LIBOMP_OMP_YEAR_MONTH 201107)
-else()
-  set(LIBOMP_OMP_YEAR_MONTH 200505)
-endif()
+set(LIBOMP_OMP_YEAR_MONTH 201611)
 set(LIBOMP_MIC_ARCH knc CACHE STRING
----------------
Hahnfeld wrote:
> AndreyChurbanov wrote:
> > Hahnfeld wrote:
> > > 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...)
> > 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
> 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!
OK, if I can get the patch accepted soon, Johnny might be able to get it checked in today.  Thanks!


Repository:
  rOMP OpenMP

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64534/new/

https://reviews.llvm.org/D64534