[Openmp-dev] Initial Intel Cmake Build System Patch

["C. Bergström"]

On 06/ 9/14 11:52 PM, Peyton, Jonathan L wrote:
>> After the NetBSD stuff - I may contribute a Solaris patch for fun...
> It is always fun to port things! :)
>
>> The Windows part doesn't seem to give any consideration for other compiler (Fortran/ASM/C/C++)?
>> if(${WINDOWS})
>> # I guess it's ok for me to do a follow-up patch for other compiler support? I could see the patch being intrusive or making the code ugly though. :-/ # My first thought is remove the if/elseif logic entirely and move all these > variable to an external toolchain file runtime/cmake/icc.cmake It would have a lot of variables like
>> WIN_FC_FLAGS_64 = "-nologo -Qdiag-disable:177,5082 -GS -DynamicBase"
>>
>> if(${DYNAMIC_LIBRARY})|
>> ||list(APPEND ||WIN_FC_FLAGS ||-Zi)|
>> endif()
>> if(${RELEASE})|
>> ...
>> |
> A long time ago, we used to support Visual Studio on Windows.  Currently, only icl for the C/C++ compiler and ml/ml64 for the assembler on Windows.
> Go ahead and add other compiler support for Windows.  That would be great.  I don't know about moving icc specific stuff to a different file.  I was real relaxed about wrapping the Windows specific flags under
> If(${ICC}).  It might be strange to have LinkerFlags.cmake, CFlags.cmake, etc. and also an icc.cmake (which would include Windows only icc?).
You misunderstand - I mean 1 configure file per toolchain. No 
LinkerFlags.cmake.. etc.. For ICC you'd create 1 file with all the 
things you'd need. icc.cmake

Sorry, the more I look at this the more I'm apposed to your original 
design. It will lead to spaghetti mess as time goes on an increasingly 
become harder to maintain. /* Sure for only ICC or another compiler it's 
fine, but would fly with multiple gcc versions, clang, icc, MSVC, 
pathscale, UH compiler.. etc */
>
>> ###
>> Nits
>>
>> I think there's a "cmake" way to get the OS version?
>> +function(set_mac_os_new return_mac_os_new)
>>
>> Would this do what you want?
>> http://www.cmake.org/Wiki/CMake_Useful_Variables
>> CMAKE_SYSTEM
> I am not sure there is a useful way to get the OSX version in a purely cmake way.  The CMAKE_SYSTEM variable doesn't correspond to the OS X version, only the Darwin Kernel version.
> I would love to get rid of this though.
Custom command is easy enough to do this. There may even be .cmake in 
the wild which helps. Maybe poking the cmake dev list is a good idea 
here. Alternatively a feature test..
>
>> Not everyone (very few) systems will have cmake this new. cmake is typically very easy to bootstrap, but may be more friendly if backed off a bit. (Doesn't impact me, but esp for TIMESTAMP. Custom command or some >default could be provided..)
>> +# - this function alone causes the need for CMake v2.8.11 (TIMESTAMP)
> I was thinking hard about this before and I think I will add some code which will call the date command on unix systems.  The problem was Windows does not have a clean way to get the TIMESTAMP.  The current way Windows does is using some Perl, but I refuse to add any more Perl.  I think you are right to back off a bit.
>
>> Why not set SSE2? (Anyone who cries fowl.. are you really going to be running OMP on a processor THAT old?)
> I think before changing this, it needs to be shown that real performance improvements could be gained.
Anecdotal evidence, but SSE2 is probably one of the most helpful x86 
extensions from my experience. Why not enable it?