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

["C. Bergström"]

On 08/ 6/14 10:28 PM, Carlo Bertolli wrote:
>
> 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.
>

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")
--------

Is the cache size the same for Power7 and Power8. This won't block the 
current patch, but something to think about

+    if(${PPC64})
+        append_definitions("-D CACHE_LINE=128")
+    else()
+        append_definitions("-D CACHE_LINE=64")
+    endif()

-------
General comment #2 (Not your fault)
This is not professional
+if(arch)
+    set(ARCH ${arch}) #acquire from command line
+else() #assume default


I'm not surprised an OMP runtime lib would depend on pthread on linux.. 
It doesn't look good though
+if("${ARCH}" STREQUAL "ppc64")
+          find_library(PTHREAD NAMES pthread)
+          target_link_libraries(iomp5 ${PTHREAD})
+endif()

-----
__kmp_invoke_microtask is f* ugly..

The comment in the code leads me to believe one branch is "expected" to 
be used. Would __builtin_expectmaybe be something to consider in the 
future if nothing more pretty is written.

Is there a test case or anything which can help demonstrate why this is 
necessary. Maybe if we have a negative test case for clang - and that 
issue is one day fixed we can remove this s**t. If there's no test case 
it'll likely stay there forever.

Why I care - I may be mistaken, but a "micro" task in my mind should be 
as lightweight and low overhead as possible. (Be it the task itself or 
the overhead to invoke it. If it was named __kmp_invoke_slowtask or 
__kmp_invoke_uglytask I'd have no problem here..)
-------------------

Sorry to rant - I want to care about a properly written runtime, but 
it's clearly outside the scope of this CR.