[Openmp-commits] [PATCH] LLVM OpenMP CMake Overhaul

[Peyton, Jonathan L]

> My original cmake, which was removed, exposed variables which allowed the user to define the flags. Am I mistaken that this is now being required again?
It's not required.  It's optional.  Users can define flags with these variables if they want to: -DLIBOMP_${LANG}FLAGS=' ... ' , -DCMAKE_${LANG}_FLAGS=' ... ' , or CFLAGS=' ... ' envirable where ${LANG} is either C or CXX.

> I think you made a mistake - this goes beyond removing unused stuff
> I believe this removed a check for FreeBSD and thus setting it to be true.
> if("${CMAKE_SYSTEM_NAME}" STREQUAL "FreeBSD")
>     set(FREEBSD TRUE)
FREEBSD was only used once in the MicroTests.cmake file, so I just changed it to test CMAKE_SYSTEM_NAME in that one case which most people are never going to use.  It's not a mistake.

> I don't think this is correct - there are other assemblers on Win which will handle this.
> if(${WINDOWS})
>     enable_language(ASM_MASM)
> endif()
The z_Windows_NT-586_asm.asm file is in the MASM syntax and also only included when building on WINDOWS.  All the enable_language(ASM_MASM) line does is look for a working masm assembler.  These lines are correct and aren't any different than what the old one did.

> libomp_append(flags_local -std=c++0x
> When will this flag die? That's legacy shit from gcc from a long time ago.. it should be c++11, which the minimum version of gcc required to build llvm supports.
I'm not opposed to changing this.  I personally don't care about this flag.

>runtime/cmake/LibompHandleFlags.cmake
>looks like a giant mess. This is exactly the reason it was the way it was before. Please work with others on the llvm side to get permission to for some middle-ground approach.
Agree to disagree.  In fact, I'm not sure how this isn't more clean than the way before.  There are less if()'s and all the flags are only appended if they were supported by the compiler.  There aren't any OS or compiler dependencies in here, only architecture which is to be expected.
All other llvm projects do something similar (you can look at libcxx/CMakeLists.txt or compiler-rt/CMakeLists.txt at the bottom for example).  The only thing different is I've wrapped each flag type in a function.

> Since you're spending all this time.. wouldn't those nasty perl scripts get in the way as well? (When will those die)
I'm actually trying to get rid of expand-vars.pl by using cmake configure_file().  What's going to be harder to get rid of are generate-defs.pl (Windows only) and message-converter.pl.  Those are the three perl scripts that are required as of now.

>My original cmake, which was removed...
>My original system worked and was removed without much justification "just because"
It was removed because it didn’t work, was suffering from bit rot, didn't support 32-bit builds, didn't support Windows, didn't support Fortran modules, didn't have basic CMake Cache variables, didn't support automatic architecture detection, and it didn't check anything (compiler flags, features).
This new CMake build system is more in the spirit of what a configuration system is supposed to do which is check everything before using it.

-- Johnny

-----Original Message-----
From: C Bergström [mailto:cbergstrom at pathscale.com] 
Sent: Tuesday, June 23, 2015 12:54 PM
To: Peyton, Jonathan L
Cc: reviews+D10656+public+dda1fdcafb4c32bf at reviews.llvm.org; Chandler Carruth; howarth.mailing.lists at apple.com; openmp-commits at dcs-maillist2.engr.illinois.edu; llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] LLVM OpenMP CMake Overhaul

On Wed, Jun 24, 2015 at 12:04 AM, Peyton, Jonathan L <jonathan.l.peyton at intel.com> wrote:
> Chris,
>
>> I am strongly apposed to this change. This was designed specifically to ensure that each set of compilers can easily set their own specific flags. Do not remove this feature.
>>Why - who cares? fwiw - most cmake files are named CMakeLists.txt  
>>.cmake is typically used for cmake files which are meant to be reused as "modules".
>> I am a bit annoyed with the history of the cmake build system. My original system worked and was removed without much justification "just because". Now it's being refactored again and it's unclear to me the driving motivation.
>
> Maybe I can help answer these questions with the aid of the email message below.  To get the LLVM OpenMP Runtime set as the default OpenMP runtime library linked when using the -fopenmp flag, we have to get our CMake build to use LLVM conventions/standards.  This meant putting everything in its own pseudo namespace Libomp, libomp_ , or LIBOMP_ and creating a real configuration step which checks for compiler flags, features, etc.  I've followed what libcxx and compiler-rt do by having a config-ix.cmake file which does all this.  Although the compiler flags approach I had worked for gcc,clang,icc,and msvc, this method of checking the compiler flags is more generic and allows any compiler that supports the flag to use it.  Also, there are at least three other methods of adding custom flags (-DLIBOMP_${LANG}FLAGS=' ... ' , -DCMAKE_${LANG}_FLAGS=' ... ' , or good ole CFLAGS=' ... ' envirable).  The ultimate goal here is to get the LLVM OpenMP Runtime set as the default, so I have to do whatever it takes to achieve that.  If I get the ok to keep the old compiler flag method, I will.

We may have conflicting goals or requirements then. I think it's a great thing to advance llvm, but I don't personally care what llvm defines as a default for OMP runtime. Anyone building libomp which isn't using it as part of llvm probably will feel the same way.

My original cmake, which was removed, exposed variables which allowed the user to define the flags. Am I mistaken that this is now being required again?

I think you made a mistake - this goes beyond removing unused stuff

I believe this removed a check for FreeBSD and thus setting it to be true.
if("${CMAKE_SYSTEM_NAME}" STREQUAL "FreeBSD")
    set(FREEBSD TRUE)
------------
I don't think this is correct - there are other assemblers on Win which will handle this.

if(${WINDOWS})
    enable_language(ASM_MASM)
endif()
----------
libomp_append(flags_local -std=c++0x

When will this flag die? That's legacy shit from gcc from a long time ago.. it should be c++11, which the minimum version of gcc required to build llvm supports.

runtime/cmake/LibompHandleFlags.cmake
looks like a giant mess. This is exactly the reason it was the way it was before. Please work with others on the llvm side to get permission to for some middle-ground approach.
-------------
Since you're spending all this time.. wouldn't those nasty perl scripts get in the way as well? (When will those die)

add_custom_command(
            OUTPUT  ${filename}
            COMMAND ${PERL_EXECUTABLE}
${LIBOMP_TOOLS_DIR}/expand-vars.pl --strict ${LIBOMP_EVFLAGS}
                ${libomp_extra_evflags} ${file_dir}/${filename}.var ${filename}
            DEPENDS ${file_dir}/${filename}.var kmp_version.c ${LIBOMP_TOOLS_DIR}/expand-vars.pl