PPC64 patch from Intel's fourth cmake patch

Hi all,

I managed to create a git patch on top of latest Intel’s cmake patch from Jonathan Peyton.

(See attached file: ppc64-patch-from-fourth-intel-cmake)

This patch adds the ppc64 architecture and enables the current Makefile system, the old cmake system, and the new cmake system on that architecture.
It completely replaces the previous patch I sent on the openmp-commits list.

Please let me know of any comments.

Thanks

– Carlo

ppc64-patch-from-fourth-intel-cmake (33.9 KB)

Hi Carlo!

Thanks a lot for sending this

Here's my off the cuff review

Hi C. Bergstrom,

Thanks very much for the review. Below, my detailed comments on this, interspersed with your comments:

Carlo,

If you send a new patch soon, I’ll commit it before I push to the 3.5 branch.

Hi James,

Attached a second version of my patch addressing C. Bergström concerns (again, thanks for this).

(See attached file: ppc64-second-patch-from-fourth-intel-cmake)

There were issues that remained unsolved - here are my answers:

  1. I removed the use of uname from everywhere. The user has to specify the value of the variable “arch” on the command line (e.g. ppc64), otherwise 32e is assumed.

  2. The old cmake has a very complex way of setting the OpenMP version (3 variables are used). I removed all my attempts to make them homogeneous, and left it as it was before I worked on it.

  3. I left the implementation of kmp_invoke_microtask as Hal Finkel did it because there is no time to implement a better version - incidentally, this will work for the foreseeable future.

Please, let me know of any comments and I’ll quickly fix the thing and send a revised patch.
In the meantime, thanks for holding this while I was fixing my patch.

Cheers

– Carlo

graycol.gif“Cownie, James H” —08/06/2014 05:23:22 AM—Carlo, If you send a new patch soon, I’ll commit it before I push to the 3.5 branch.

ppc64-second-patch-from-fourth-intel-cmake (33.2 KB)

Thanks, I’ll leave this until tomorrow morning (UK time) to give a window for any final reviews, then commit it assuming no objections.

graycol.gif

I don't see any blockers, but I do have some general feedback.

Hi C. Bergström,

My answers below interspersed with your comments.

Apologies - My comments were meant to be general and your patch just made some fugly areas more visible. Hopefully this clears up the confusion. For our BGQ (IBM Power7 A2) work I certainly appreciate this.

Thanks

Hi,

No apologies needed - I am glad that you highlighted these issues and that you helped making the patch stronger.
Let me see what I can do about the imlementation of kmp__invoke_microtask.

Thanks

– Carlo

Inactive hide details for "C. Bergström" ---08/06/2014 01:37:30 PM---On 08/ 6/14 11:27 PM, Carlo Bertolli wrote: >“C. Bergström” —08/06/2014 01:37:30 PM—On 08/ 6/14 11:27 PM, Carlo Bertolli wrote: >

C. Bergström, Carlo,

---------
using uname to detect the host/target was rejected before. Please use
the "cmake" way of detecting things.
+EXECUTE_PROCESS( COMMAND uname -m COMMAND tr -d '\n' OUTPUT_VARIABLE
DETECT_ARCH )
+EXECUTE_PROCESS( COMMAND uname -m COMMAND tr -d '\n' OUTPUT_VARIABLE ARCH )

OK. I did some investigation on this and have done it again just now but was not able to find a way of doing this natively in cmake.
Actually, I found comments about how cmake does not support this natively (may be an old cmake version).
Can you suggest how to do this? (a link to a web page with an example is fine too).

I have done some research on this topic. You are both right :slight_smile: . There is a cmakey way to get at uname output, and this particular variable is CMAKE_HOST_SYSTEM_PROCESSOR which
Is defined by uname -p (slightly different but seems to be preferred over uname -m for reasons I'm not sure). You can actually see this in the standard cmake modules located at:
/path/to/cmake_install_root/share/cmake-2.8/Modules/CMakeDetermineSystem.cmake

The problem with using this as an architecture detection mechanism: it is not standard. Some systems will define uname -p as x86_64, amd64, x64, etc. This information is embedded in the kernel, and I was not inclined to try to gather all possible values into a giant if() statement to determine if it was 64 bit x86 architecture (same for 32 bit x86).

So if there is a need to put something like this back in, use the CMAKE_HOST_SYSTEM_PROCESSOR variable because it won't mess up Windows builds that don't even have uname to depend on.

I don't see any blockers, but I do have some general feedback.
-------------
Why would we set 64bit pointer size to 32e - Seems wrong

+ if("${CMAKE_SIZEOF_VOID_P}" STREQUAL "4")
+ set(arch 32 CACHE STRING "The architecture
to build for (32e/32/arm/ppc64). 32e is Intel(R) 64 architecture, 32 is
IA-32 architecture")
+ else()
+ set(arch 32e CACHE STRING "The architecture
to build for (32e/32/arm/ppc64). 32e is Intel(R) 64 architecture, 32 is
IA-32 architecture")
--------

This code was in place before I made my additions. I am surprised that it was not noticed before.
However, the code seems fine by me: if the size of the void * type is 4, then we assume the arch variable to the default 32 (x86_32). Otherwise, it assumes a 8 bytes void * type and it sets it to default 32e (x86_64).

Yes. This code is mine. Because of the above reasons, I thought "Well, there has to be some default architecture instead of the build system just error-ing out." So I picked x86 architecture as it is the most popular desktop architecture; perhaps I'm biased :). CMAKE_SIZEOF_VOID_P is determined via the compiler you send cmake -DCMAKE_C_COMPILER=<compiler>. I also had the configuration spit out the architecture it believes it is going to build to inform the user.

-- Johnny

Ok I didn't realize it was still such a pita.

Others appear to have solved this though
https://github.com/petroules/solar-cmake/blob/master/TargetArch.cmake

Seems to cleanly wrap the if() statements and provide a reliable way to detect what's needed. I do slightly question if it will be perfect for PPC64, but I guess it wouldn't be too hard to patch.

If this isn't sufficient I can keep looking or maybe give a hand fixing it.

llvm/clang may have something similar as well..

Committed at Rev 215093.

I’ll now also commit to the 3.5 branch.

graycol.gif

Jamie,
It seems that the latest commits for ppc64 seem to have regressed the fat build on darwin. After the commit of

http://lists.cs.uiuc.edu/pipermail/openmp-commits/2014-August/000030.html

on x86_64-apple-darwin11, I could still build the fat libiomp5 using…

cd openmp-3.6.0/runtime
mkdir build
cd build
cmake -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -Dos=mac -Darch=32 …

make -j 1

cmake -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -Dos=mac -Darch=32e …

make -j 1

make fat

However after the ppc64 changes went it, the build regressed such that I have to use ‘rm -fr *’ before the second call to cmake for -Darch=32e
otherwise, I get…

[ 2%] Generating iomp_lib.h
[ 5%] Generating …/exports/mac_32e/include_compat/iomp_lib.h
[ 5%] Built target inc
[ 8%] Generating kmp_i18n_id.inc
[ 11%] Generating kmp_i18n_default.inc
[ 13%] Generating omp.h
[ 13%] Built target needed-headers
Scanning dependencies of target iomp5
[ 16%] Building C object CMakeFiles/iomp5.dir/src/kmp_ftn_cdecl.c.o
/bin/sh: /Users/howarth/llvm-svn/openmp-3.6.0/runtime/build/clang: No such file or directory
make[2]: *** [CMakeFiles/iomp5.dir/src/kmp_ftn_cdecl.c.o] Error 127
make[1]: *** [CMakeFiles/iomp5.dir/all] Error 2
make: *** [all] Error 2

on the second ‘make’ for -Darch=32e

Jack

graycol.gif

Jack,

From http://www.cmake.org/Wiki/CMake_Useful_Variables

CMAKE_C_COMPILER

the compiler used for C files. Normally it is detected and set during the CMake run, but you can override it at configuration time. Note! It can not be changed after the first cmake or ccmake run. Although the gui allows to enter an alternative, it will be ignored in the next ‘configure’ run. Use for example:
CC=gcc-3.3 CXX=g+±3.3 cmake
to set the compiler. (You can also set CMAKE_C_COMPILER_INIT, before any PROJECT() or ENABLE_LANGUAGE() command.) Any other way (like writing make CC=gcc-3.3 CXX=g+±3.3) will not work. When using distcc or similar tools, you need to write:
CC=“distcc gcc-3.3” CXX=“distcc g+±3.3” cmake
However, this will empty all your CMAKE_…FLAGS… above.

Note that “It can not be changed after the first cmake or ccmake run” . This means it cannot be specified again even if it is the same compiler name.

You should get this undesired message if you specify the compilers on a subsequent cmake run:

– Configuring done

You have changed variables that require your cache to be deleted.

Configure will be re-run and you may have to reset some variables.

The following variables have changed:

CMAKE_C_COMPILER= clang

CMAKE_CXX_COMPILER= clang++

Your error message suggests that cmake set the compiler path to /Users/howarth/llvm-svn/openmp-3.6.0/runtime/build/clang which I’m guessing does not exist!

So you should be able to do this without any rm –rf * command:

cd openmp-3.6.0/runtime

mkdir build

cd build

cmake -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -Dos=mac -Darch=32 …

make -j 1

cmake -Darch=32e …

make -j 1

make fat

– Johnny

graycol.gif

Jonathan,
I looks like…

CC=clang CXX=clang++ cmake -Dos=mac -Darch=32 …

make -j 1
CC=clang CXX=clang++ cmake -Dos=mac -Darch=32e …

make -j 1
make fat

…suffices. The problem was that we wanted to use a loop in our fink packaging script like…

pushd projects/openmp/runtime
mkdir build
pushd build
for a in 32 32e
do
cmake -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -Dos=mac -Darch=$a …
make -j 1
done
make fat
popd
popd

which makes it hard to conditionalize the use of -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++. So it
seems this can be simplified to…

pushd projects/openmp/runtime
mkdir build
pushd build
for a in 32 32e
do
CC=clang CXX=clang++ cmake -Dos=mac -Darch=$a …
make -j 1
done
make fat
popd
popd

Jack

graycol.gif

I guess if you wanted to stay away from the environment variables you could do this:

pushd projects/openmp/runtime

mkdir build

pushd build

cmake –DCMAKE_C_COMPILER=clang –DCMAKE_CXX_COMPILER=clang++ …

for a in 32 32e

do

cmake -Dos=mac -Darch=$a …

make -j 1

done

make fat

popd

popd

Either way is fine. I’m just trying to offer a solution without environment variables if that is what you are looking for.

– Johnny

graycol.gif

Others appear to have solved this though https://github.com/petroules/solar-cmake/blob/master/TargetArch.cmake

Seems to cleanly wrap the if() statements and provide a reliable way to detect what's needed. I do slightly question if it will be perfect for PPC64, but I guess it wouldn't be too hard to patch.

I think it is a stretch to call this clean. It is slick, but it relies on predefined compiler macros to detect the architecture on non-macosx operating systems.
It creates a dummy C file with all the #if __ARCH_MACRO__, "compiles and runs" it, then takes the stderr
from the compiler output as the architecture.

llvm/clang may have something similar as well..

LLVM uses uname indirectly. I have dug through this a little, the calling sequence goes as follows for cmake:
Top Level CMakeLists.txt --> (includes) cmake/config-ix.cmake --> (includes) cmake/GetHostTriple.cmake --> (calls) get_host_triple() --> (calls) sh autoconf/config.guess --> (calls) uname
The variable at interest is LLVM_NATIVE_ARCH. It appears someone has already done the if() work (in autoconf/config.guess) for autoconf and LLVM uses that for non-Windows machines.

-- Johnny

Others appear to have solved this though https://github.com/petroules/solar-cmake/blob/master/TargetArch.cmake

Seems to cleanly wrap the if() statements and provide a reliable way to detect what's needed. I do slightly question if it will be perfect for PPC64, but I guess it wouldn't be too hard to patch.

I think it is a stretch to call this clean. It is slick, but it relies on predefined compiler macros to detect the architecture on non-macosx operating systems.
It creates a dummy C file with all the #if __ARCH_MACRO__, "compiles and runs" it, then takes the stderr
from the compiler output as the architecture.

#1 What's the problem with this method? autocrap and cmake are likely doing compile time tests in the background that may not be obvious (I'm curious how it's definitively finding the size of a pointer)

#2 by "clean" I meant that this cmake module abstracts away these details and just include it and done.. no need to deal with the if() spaghetti mess directly

llvm/clang may have something similar as well..

LLVM uses uname indirectly. I have dug through this a little, the calling sequence goes as follows for cmake:
Top Level CMakeLists.txt --> (includes) cmake/config-ix.cmake --> (includes) cmake/GetHostTriple.cmake --> (calls) get_host_triple() --> (calls) sh autoconf/config.guess --> (calls) uname
The variable at interest is LLVM_NATIVE_ARCH. It appears someone has already done the if() work (in autoconf/config.guess) for autoconf and LLVM uses that for non-Windows machines.

I'd need to look, but I'm a bit skeptical that cmake is calling autocrap to get this. If this is really the case then they can go to hell and we should certainly do it more cleanly. (Learning from the failures of others)

My feedback feels like I'm pushing a rock uphill, but I guess I should be happy I didn't have to look at M4...

#1 What's the problem with this method? autocrap and cmake are likely doing compile time tests in the background that may not be obvious (I'm curious how it's definitively finding the size of a pointer)

#2 by "clean" I meant that this cmake module abstracts away these details and just include it and done.. no need to deal with the if() spaghetti mess directly

Yeah, the more I think about it, this is probably the best way. One nice feature about this way is it accommodates cross-compiling in a natural way by probing the compiler itself.
I guess "clean" for me means to have CMake offer this functionality instead of each individual project hacking up their own solution (or borrowing from others). They already probe the compiler
in a similar fashion for __COMPILER_TYPE_MACROS__ and test a dummy source file to make sure your toolchain is set up correctly to produce executables.

-- Johnny