<html><body>
<p><font size="2" face="sans-serif">Hi C. Bergström,</font><br>
<br>
<font size="2" face="sans-serif">My answers below interspersed with your comments.</font><br>
<br>
<br>
<tt><font size="2">"C. Bergström" <cbergstrom@pathscale.com> wrote on 08/06/2014 12:05:24 PM:<br>
<br>
> From: "C. Bergström" <cbergstrom@pathscale.com></font></tt><br>
<tt><font size="2">> To: Carlo Bertolli/Watson/IBM@IBMUS</font></tt><br>
<tt><font size="2">> Cc: "Cownie, James H" <james.h.cownie@intel.com>, Michael Wong <br>
> <michaelw@ca.ibm.com>, "openmp-dev@dcs-maillist2.engr.illinois.edu" <br>
> <openmp-dev@dcs-maillist2.engr.illinois.edu>, Hal Finkel <hfinkel@anl.gov></font></tt><br>
<tt><font size="2">> Date: 08/06/2014 12:07 PM</font></tt><br>
<tt><font size="2">> Subject: Re: [Openmp-dev] PPC64 patch from Intel's fourth cmake patch</font></tt><br>
<tt><font size="2">> <br>
> On 08/ 6/14 10:28 PM, Carlo Bertolli wrote:<br>
> ><br>
> > Hi James,<br>
> ><br>
> ><br>
> > Attached a second version of my patch addressing C. Bergström concerns <br>
> > (again, thanks for this).<br>
> ><br>
> > /(See attached file: ppc64-second-patch-from-fourth-intel-cmake)/<br>
> ><br>
> > There were issues that remained unsolved - here are my answers:<br>
> ><br>
> > 1. I removed the use of uname from everywhere. The user has to specify <br>
> > the value of the variable "arch" on the command line (e.g. ppc64), <br>
> > otherwise 32e is assumed.<br>
> ><br>
> > 2. The old cmake has a very complex way of setting the OpenMP version <br>
> > (3 variables are used). I removed all my attempts to make them <br>
> > homogeneous, and left it as it was before I worked on it.<br>
> ><br>
> > 3. I left the implementation of kmp_invoke_microtask as Hal Finkel did <br>
> > it because there is no time to implement a better version - <br>
> > incidentally, this will work for the foreseeable future.<br>
> ><br>
> ><br>
> > Please, let me know of any comments and I'll quickly fix the thing and <br>
> > send a revised patch.<br>
> > In the meantime, thanks for holding this while I was fixing my patch.<br>
> ><br>
> <br>
> I don't see any blockers, but I do have some general feedback.<br>
> -------------<br>
> Why would we set 64bit pointer size to 32e - Seems wrong<br>
> <br>
> + if("${CMAKE_SIZEOF_VOID_P}" STREQUAL "4")<br>
> + set(arch 32 CACHE STRING "The architecture <br>
> to build for (32e/32/arm/ppc64). 32e is Intel(R) 64 architecture, 32 is <br>
> IA-32 architecture")<br>
> + else()<br>
> + set(arch 32e CACHE STRING "The architecture <br>
> to build for (32e/32/arm/ppc64). 32e is Intel(R) 64 architecture, 32 is <br>
> IA-32 architecture")<br>
</font></tt><br>
<br>
<tt><font size="2">This code was in place before I made my additions. I am surprised that it was not noticed before.</font></tt><br>
<tt><font size="2">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).</font></tt><br>
<br>
<br>
<tt><font size="2">> --------<br>
> <br>
> Is the cache size the same for Power7 and Power8. This won't block the <br>
> current patch, but something to think about<br>
> <br>
> + if(${PPC64})<br>
> + append_definitions("-D CACHE_LINE=128")<br>
> + else()<br>
> + append_definitions("-D CACHE_LINE=64")<br>
> + endif()<br>
</font></tt><br>
<tt><font size="2">Yes, this is true also for Power7.</font></tt><br>
<br>
<tt><font size="2"><br>
> -------<br>
> General comment #2 (Not your fault)<br>
> This is not professional<br>
> +if(arch)<br>
> + set(ARCH ${arch}) #acquire from command line<br>
> +else() #assume default<br>
> <br>
> </font></tt><br>
<br>
<tt><font size="2">Indeed, I am always open to more professional solutions. Are you be able to suggest one?</font></tt><br>
<br>
<tt><font size="2"><br>
> I'm not surprised an OMP runtime lib would depend on pthread on linux.. <br>
> It doesn't look good though<br>
> +if("${ARCH}" STREQUAL "ppc64")<br>
> + find_library(PTHREAD NAMES pthread)<br>
> + target_link_libraries(iomp5 ${PTHREAD})<br>
> +endif()<br>
</font></tt><br>
<tt><font size="2">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.</font></tt><br>
<br>
<tt><font size="2">About libpthread: libpthread is used also by other implementations of OpenMP.</font></tt><br>
<tt><font size="2">Now, even if I find the thing not ideal, this is the state of things in IOMP5.</font></tt><br>
<tt><font size="2">I am not sure if this is a comment about this patch.</font></tt><br>
<br>
<tt><font size="2">> <br>
> -----<br>
> __kmp_invoke_microtask is f* ugly..<br>
> <br>
> The comment in the code leads me to believe one branch is "expected" to <br>
> be used. Would __builtin_expectmaybe be something to consider in the <br>
> future if nothing more pretty is written.<br>
> <br>
> Is there a test case or anything which can help demonstrate why this is <br>
> necessary. Maybe if we have a negative test case for clang - and that <br>
> issue is one day fixed we can remove this s**t. If there's no test case <br>
> it'll likely stay there forever.<br>
> <br>
> Why I care - I may be mistaken, but a "micro" task in my mind should be <br>
> as lightweight and low overhead as possible. (Be it the task itself or <br>
> the overhead to invoke it. If it was named __kmp_invoke_slowtask or <br>
> __kmp_invoke_uglytask I'd have no problem here..)<br>
> -------------------<br>
> <br>
> Sorry to rant - I want to care about a properly written runtime, but <br>
> it's clearly outside the scope of this CR.<br>
> <br>
</font></tt><br>
<tt><font size="2">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.</font></tt><br>
<br>
<br>
<tt><font size="2">Best</font></tt><br>
<br>
<tt><font size="2">-- Carlo</font></tt><br>
</body></html>