[Openmp-dev] PPC64 patch from Intel's fourth cmake patch

["C. Bergström"]

On 08/ 6/14 01:26 AM, Carlo Bertolli wrote:
>
> 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.
>
Hi Carlo!

Thanks a lot for sending this

Here's my off the cuff review
---------
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 )


Please don't use !not when it can be avoided - It can be slightly 
confusing at a glance
# Is this a work-around to something? Why not just test for ppc64 and 
handle that condition?
if(NOT "${DETECT_ARCH}" STREQUAL "ppc64")

why change the minimum cmake version?
+cmake_minimum_required(VERSION 3.0)

--------
We should find a correct solution instead of "ugly work-around" - I hope 
Intel can chime in here
+#ugly workaround because these variables are not really set
// For PathScale on BGQ - we are only able to support OMP2.5 for now. 
Advertising support all the way to 4.x would possibly lead to weird 
things. This should be done with care
--------
I don't know if this is limited to linux only, but it isn't a correct 
assumption for Solaris and possibly FreeBSD
-  set(CMAKE_SHARED_LINKER_FLAGS 
"-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/exports_so.txt")
+  set(CMAKE_SHARED_LINKER_FLAGS 
"-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/exports_so.txt -ldl")

--------
I think this sneaked in
-#define KMP_VERSION_BUILD    00000000
+//#define KMP_VERSION_BUILD    00000000

--------
I don't have the time to review makefile.mk/perl module - someone else 
should do that..
# Personally, I think it should be removed
----------

// This looks really weird and I'd need more details before accepting it
// I also think we may want to limit this damage to very specifically 
"clang" instead of just PPC64
// Please provide more details
+__kmp_invoke_microtask


How do you plan to test this?
Does it pass the testsuite which has been made available?
I believe ANL did some work on this (Jeff Hammond) - is any of his work 
used in this?