[Openmp-commits] [Openmp-dev] Large Refactor of CMake build system

[Peyton, Jonathan L]

> #1 The patch fixed the problem. If you feel it's a good fix then push it. I wasn't clear on the exact behavior.
If you're referring to the patch from: http://lists.cs.uiuc.edu/pipermail/openmp-dev/2015-July/000791.html
Then something is amiss because that patch is solely for debugging purposes.  Here is that patch in text form:

diff --git a/runtime/cmake/LibompCheckLinkerFlag.cmake b/runtime/cmake/LibompCheckLinkerFlag.cmake
index 1c4a085..ec49a8b 100644
--- a/runtime/cmake/LibompCheckLinkerFlag.cmake
+++ b/runtime/cmake/LibompCheckLinkerFlag.cmake
@@ -23,7 +23,8 @@ function(libomp_check_linker_flag flag boolean)
      set(CMAKE_SHARED_LINKER_FLAGS \"${flag}\")
      add_library(foo SHARED src_to_link.c)")
   set(failed_regexes "[Ee]rror;[Uu]nknown;[Ss]kipping;LINK : warning")
-  set(base_dir ${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/CMakeTmp/link_flag_check)
+  #set(base_dir ${CMAKE_BINARY_DIR}${CMAKE_FILES_DIRECTORY}/CMakeTmp/link_flag_check)
+  set(base_dir ${CMAKE_BINARY_DIR}/check_${boolean})
   file(MAKE_DIRECTORY ${base_dir})
   file(MAKE_DIRECTORY ${base_dir}/build)
   file(WRITE ${base_dir}/src_to_link.c "${library_source}")

This patch only changes where the check takes place.  It leaves the directories and files behind for the user to debug and see why a linker flag check is failing or passing when it shouldn't be.
This patch does not fix anything.  

>From the log I posted before (pathcc.log):
-- Performing Test LIBOMP_HAVE_STATIC_LIBGCC_FLAG
-- Performing Test LIBOMP_HAVE_STATIC_LIBGCC_FLAG - Failed
This suggests that the pathcc free binary (5.0.5) works as you've suggested (pathcc doesn't support the flag), and it doesn't include it.  This was run without the debugging patch above.

> If other compilers are compatible with gcc link flags - I doubt they would see a problem.
This isn't the point of a CMake configure/build system even if the main compiler support is gcc/clang.  The CMake configure/build system is set up so compilers only include flags they support.  That's the purpose of the checks and conforms to the other llvm projects.  You've mentioned the -static-libgcc flag and nothing else and I'm trying to see why pathcc would include it, but can't reproduce the problem and neither can you as far as I can tell.

> Long term I'd be fine and probably prefer you to just detect the PathScale compiler and not set anything.
This seems completely non-user-friendly to the rest of the pathcc users.  I'd rather just fix the actual problem if one exists which I'm still not convinced there is.
If *you* need the CMake to be very particular for compiler flags for your particular setup, then you can create a PathScaleKillFlags.cmake file which clears all the LIBOMP_HAVE_XYZ_FLAGS, include it right after the include(config-ix) in the main CMakeLists.txt, then they won't be included and it is somewhat modular.  Or, you can come up with a better solution for your problem.

To further prove that this new system should stay in I've now tried to build using pathcc with the old CMake system before the refactor and I get a warning:
CMake Warning at cmake/HelperFunctions.cmake:31 (message):
  Could not find cmake/PathScale/CFlags.cmake: will only use default flags
Call Stack (most recent call first):
  CMakeLists.txt:478 (warning_say)


CMake Warning at cmake/HelperFunctions.cmake:31 (message):
  Could not find cmake//AsmFlags.cmake: will only use default flags
Call Stack (most recent call first):
  CMakeLists.txt:502 (warning_say)

And then the build ***fails*** on the first source file with:

Scanning dependencies of target omp
make[2]: warning:  Clock skew detected.  Your build may be incomplete.
make[2]: Warning: File `src/CMakeFiles/omp.dir/flags.make' has modification time 67 s in the future
[ 20%] Building C object src/CMakeFiles/omp.dir/kmp_ftn_cdecl.c.o
In file included from /nfs/fx/proj/openmp/users/jlpeyton/openmp_llvm_commit/itt/libomp_llvm/runtime/src/kmp_ftn_cdecl.c:16:
In file included from /nfs/fx/proj/openmp/users/jlpeyton/openmp_llvm_commit/itt/libomp_llvm/runtime/src/kmp.h:88:
/nfs/fx/proj/openmp/users/jlpeyton/openmp_llvm_commit/itt/libomp_llvm/runtime/src/kmp_lock.h:385:5: error: must use 'struct' tag to refer to type 'kmp_base_queuing_lock'
    kmp_base_queuing_lock qlk;
    ^
    struct
In file included from /nfs/fx/proj/openmp/users/jlpeyton/openmp_llvm_commit/itt/libomp_llvm/runtime/src/kmp_ftn_cdecl.c:16:
/nfs/fx/proj/openmp/users/jlpeyton/openmp_llvm_commit/itt/libomp_llvm/runtime/src/kmp.h:1928:10: error: unknown type name 'bool'
         bool                   in:1;
         ^
/nfs/fx/proj/openmp/users/jlpeyton/openmp_llvm_commit/itt/libomp_llvm/runtime/src/kmp.h:1929:10: error: unknown type name 'bool'
         bool                   out:1;
         ^
/nfs/fx/proj/openmp/users/jlpeyton/openmp_llvm_commit/itt/libomp_llvm/runtime/src/kmp.h:2694:8: error: must use 'struct' tag to refer to type 'kmp_adaptive_backoff_params_t'
extern kmp_adaptive_backoff_params_t __kmp_adaptive_backoff_params;
       ^
       struct

Since I've been *able* to build with pathcc using the new CMake system but *not able* to build with the old CMake system, I'd say these CMake refactor changes are for the best for the general pathcc user.

-- Johnny

-----Original Message-----
From: C Bergström [mailto:cbergstrom at pathscale.com] 
Sent: Monday, July 20, 2015 10:55 PM
To: Peyton, Jonathan L
Cc: Hans Wennborg; Chandler Carruth; openmp-commits at dcs-maillist2.engr.illinois.edu; openmp-dev at dcs-maillist2.engr.illinois.edu
Subject: Re: [Openmp-commits] [Openmp-dev] Large Refactor of CMake build system

On Tue, Jul 21, 2015 at 10:12 AM, Peyton, Jonathan L <jonathan.l.peyton at intel.com> wrote:
> First, I want the code to reach as many people as possible including pathcc users.  So on my own time, I have downloaded the Pathscale compiler from: http://www.pathscale.com/ekopath-compiler-suite which I did not know was open sourced, and tried to build libomp with pathcc on Linux, x86_64 (Yes, still Intel world in terms of architecture, but your original complaint suggested it was with the compiler) with Chandler's latest flag checks and succeeded with the log of the entire process attached.
> There's numerous warnings about deprecated register storage (-Wdeprecated-register), and with some further investigation this is caused by the -std=c++11 flag and as I recall, Chris, you were the driving force behind getting that changed.  I don't see either -Wno-deprecated-register or -Wdeprecated-register flag available for the open sourced pathcc so I don't see how to fix that warning even though pathcc's warning message suggests it exists.  It's certainly no showstopper.
>
> I’ll reiterate once more.  I feel the changes should stay.

#1 The patch fixed the problem. If you feel it's a good fix then push it. I wasn't clear on the exact behavior.
--------
In response to your personal efforts - I apologize as some of the information on our website may not be clear.

EKOPath 4 was fully open source - EKOPath 5+ has returned to a closed model.
The EKOPath page contains a link for a nightly build. If you adjusted the date in the URL some of the issues you see may be fixed already.

Without that patch it tries to link using libgcc-static. (Which our compiler doesn't support and will error on)
------------
Historically, "we" (PathScale) have been the 1st to adopt usage of the Intel OMP runtime afaik. This dates back to before it existed inside the llvm community. We had to mirror the source on github and basically fork because there wasn't an easy way to handle passing the changes up/down. As this process of integration with llvm has evolved
- we've continued to submit and work with others on the cmake build system. If other compilers are compatible with gcc link flags - I doubt they would see a problem. Long term I'd be fine and probably prefer you to just detect the PathScale compiler and not set anything.
This applies across *ALL* platforms. (OSX, Solaris, Win.. etc)

I've contributed to the Aarch64 port and will continue to send patches if upstream is friendly. What I'm seeing now is great on one side, but not really cool on the other. The 3.7 Release branch should be stable.
If you really want to test against a variety of places for non-llvm builds I can help facilitate that.

I hope this is resolved very soon.