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

[Carlo Bertolli]

Hi C. Bergström,

My answers below interspersed with your comments.


"C. Bergström" <cbergstrom at pathscale.com> wrote on 08/06/2014 12:05:24 PM:

> From: "C. Bergström" <cbergstrom at pathscale.com>
> To: Carlo Bertolli/Watson/IBM at IBMUS
> Cc: "Cownie, James H" <james.h.cownie at intel.com>, Michael Wong
> <michaelw at ca.ibm.com>, "openmp-dev at dcs-maillist2.engr.illinois.edu"
> <openmp-dev at dcs-maillist2.engr.illinois.edu>, Hal Finkel
<hfinkel at anl.gov>
> Date: 08/06/2014 12:07 PM
> Subject: Re: [Openmp-dev] PPC64 patch from Intel's fourth cmake patch
>
> 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")


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


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

Yes, this is true also for Power7.


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

Indeed, I am always open to more professional solutions. Are you be able to
suggest one?


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

I think this is misleading: this is a problem in linking for PPC64 linux
architecture. If the ordering is not enforced (as it was not in this old
cmake version) then we run into linking problems. I added a link to a page
explaining this just on top of the thing so it is clear.

About libpthread: libpthread is used also by other implementations of
OpenMP.
Now, even if I find the thing not ideal, this is the state of things in
IOMP5.
I am not sure if this is a comment about this patch.

>
> -----
> __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.
>

Again, I don't have a quick fix to this. I do agree it should be taken care
of, but I believe there is just no time today.


Best

-- Carlo
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/openmp-dev/attachments/20140806/6a1680cd/attachment.html>