<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
@font-face
{font-family:Verdana;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p
{mso-style-priority:99;
mso-margin-top-alt:auto;
margin-right:0cm;
mso-margin-bottom-alt:auto;
margin-left:0cm;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
tt
{mso-style-priority:99;
font-family:"Courier New";}
span.EmailStyle19
{mso-style-type:personal-reply;
font-family:"Verdana","sans-serif";
color:#1F497D;
font-weight:normal;
font-style:normal;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-GB" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:#1F497D">Carlo,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:#1F497D">If you send a new patch soon, I’ll commit it *<b>before</b>* I push to the 3.5 branch.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<div>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:#1F497D">-- Jim<br>
<br>
James Cownie <james.h.cownie@intel.com><br>
SSG/DPD/TCAR (Technical Computing, Analyzers and Runtimes)<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:#1F497D">Tel: +44 117 9071438</span><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"><o:p></o:p></span></p>
</div>
<p class="MsoNormal"><span style="font-size:10.0pt;font-family:"Verdana","sans-serif";color:#1F497D"><o:p> </o:p></span></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0cm 0cm 0cm">
<p class="MsoNormal"><b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span lang="EN-US" style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> openmp-dev-bounces@cs.uiuc.edu [mailto:openmp-dev-bounces@cs.uiuc.edu]
<b>On Behalf Of </b>Carlo Bertolli<br>
<b>Sent:</b> Tuesday, August 05, 2014 11:52 PM<br>
<b>To:</b> C. Bergström<br>
<b>Cc:</b> openmp-dev@dcs-maillist2.engr.illinois.edu; Michael Wong<br>
<b>Subject:</b> Re: [Openmp-dev] PPC64 patch from Intel's fourth cmake patch<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p style="margin-bottom:12.0pt"><span style="font-size:10.0pt;font-family:"Arial","sans-serif"">Hi C. Bergstrom,</span><br>
<br>
<span style="font-size:10.0pt;font-family:"Arial","sans-serif"">Thanks *very much* for the review. Below, my detailed comments on this, interspersed with your comments:</span><br>
<br>
<br>
<br>
<br>
<tt><span style="font-size:10.0pt">"C. Bergström" <<a href="mailto:cbergstrom@pathscale.com">cbergstrom@pathscale.com</a>> wrote on 08/05/2014 05:11:10 PM:</span></tt><span style="font-size:10.0pt;font-family:"Courier New""><br>
<br>
<tt>> From: "C. Bergström" <<a href="mailto:cbergstrom@pathscale.com">cbergstrom@pathscale.com</a>></tt></span><br>
<tt><span style="font-size:10.0pt">> To: Carlo Bertolli/Watson/IBM@IBMUS</span></tt><br>
<tt><span style="font-size:10.0pt">> Cc: <a href="mailto:openmp-dev@dcs-maillist2.engr.illinois.edu">
openmp-dev@dcs-maillist2.engr.illinois.edu</a>, Michael Wong </span></tt><span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>> <<a href="mailto:michaelw@ca.ibm.com">michaelw@ca.ibm.com</a>></tt></span><br>
<tt><span style="font-size:10.0pt">> Date: 08/05/2014 05:13 PM</span></tt><br>
<tt><span style="font-size:10.0pt">> Subject: Re: [Openmp-dev] PPC64 patch from Intel's fourth cmake patch</span></tt><br>
<tt><span style="font-size:10.0pt">> </span></tt><span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>> On 08/ 6/14 01:26 AM, Carlo Bertolli wrote:</tt><br>
<tt>> ></tt><br>
<tt>> > Hi all,</tt><br>
<tt>> ></tt><br>
<tt>> > I managed to create a git patch on top of latest Intel's cmake patch </tt>
<br>
<tt>> > from Jonathan Peyton.</tt><br>
<tt>> ></tt><br>
<tt>> > /(See attached file: ppc64-patch-from-fourth-intel-cmake)/</tt><br>
<tt>> ></tt><br>
<tt>> > This patch adds the ppc64 architecture and enables the current </tt><br>
<tt>> > Makefile system, the old cmake system, and the new cmake system on </tt><br>
<tt>> > that architecture.</tt><br>
<tt>> > It completely replaces the previous patch I sent on the openmp-commits </tt>
<br>
<tt>> > list.</tt><br>
<tt>> ></tt><br>
<tt>> ></tt><br>
<tt>> > Please let me know of any comments.</tt><br>
<tt>> ></tt><br>
<tt>> Hi Carlo!</tt><br>
<tt>> </tt><br>
<tt>> Thanks a lot for sending this</tt><br>
<tt>> </tt><br>
<tt>> Here's my off the cuff review</tt><br>
<tt>> ---------</tt><br>
<tt>> using uname to detect the host/target was rejected before. Please use </tt>
<br>
<tt>> the "cmake" way of detecting things.</tt><br>
<tt>> +EXECUTE_PROCESS( COMMAND uname -m COMMAND tr -d '\n' OUTPUT_VARIABLE </tt>
<br>
<tt>> DETECT_ARCH )</tt><br>
<tt>> +EXECUTE_PROCESS( COMMAND uname -m COMMAND tr -d '\n' OUTPUT_VARIABLE ARCH )</tt><br>
<tt>> </tt></span><br>
<br>
<tt><span style="font-size:10.0pt">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.</span></tt><br>
<tt><span style="font-size:10.0pt">Actually, I found comments about how cmake does not support this natively (may be an old cmake version).</span></tt><br>
<tt><span style="font-size:10.0pt">Can you suggest how to do this? (a link to a web page with an example is fine too).</span></tt><br>
<br>
<span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>> </tt><br>
<tt>> Please don't use !not when it can be avoided - It can be slightly </tt><br>
<tt>> confusing at a glance</tt><br>
<tt>> # Is this a work-around to something? Why not just test for ppc64 and </tt>
<br>
<tt>> handle that condition?</tt><br>
<tt>> if(NOT "${DETECT_ARCH}" STREQUAL "ppc64")</tt><br>
</span><br>
<tt><span style="font-size:10.0pt">I agree this is just a stupid way of coding things - I will fix it.</span></tt><br>
<br>
<tt><span style="font-size:10.0pt">> </span></tt><span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>> why change the minimum cmake version?</tt><br>
<tt>> +cmake_minimum_required(VERSION 3.0)</tt><br>
</span><br>
<tt><span style="font-size:10.0pt">I got an annoying warning from cmake when running on one of the machines here - I added it to get rid to the warning, but I will delete this.</span></tt><br>
<br>
<tt><span style="font-size:10.0pt">> </span></tt><span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>> --------</tt><br>
<tt>> We should find a correct solution instead of "ugly work-around" - I hope </tt>
<br>
<tt>> Intel can chime in here</tt><br>
<tt>> +#ugly workaround because these variables are not really set</tt><br>
<tt>> // For PathScale on BGQ - we are only able to support OMP2.5 for now. </tt>
<br>
<tt>> Advertising support all the way to 4.x would possibly lead to weird </tt><br>
<tt>> things. This should be done with care</tt><br>
</span><br>
<tt><span style="font-size:10.0pt">This is a quick fixup that I made to the old cmake to have it work again on the machine I was using. I can get rid of those and ask who runs cmake to set them up. Would that be fine by you?</span></tt><br>
<br>
<br>
<tt><span style="font-size:10.0pt">> --------</span></tt><span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>> I don't know if this is limited to linux only, but it isn't a correct </tt>
<br>
<tt>> assumption for Solaris and possibly FreeBSD</tt><br>
<tt>> - set(CMAKE_SHARED_LINKER_FLAGS </tt><br>
<tt>> "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/exports_so.txt")</tt><br>
<tt>> + set(CMAKE_SHARED_LINKER_FLAGS </tt><br>
<tt>> "-Wl,--version-script=${CMAKE_CURRENT_SOURCE_DIR}/exports_so.txt -ldl")</tt><br>
<tt>> </tt></span><br>
<br>
<tt><span style="font-size:10.0pt">I think this only triggered an error on ppc64 linux. x86_64 linux did not raise any issue. What is your suggestion? Should I guard it as "if linux && ppc64"?</span></tt><br>
<span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>> --------</tt><br>
<tt>> I think this sneaked in</tt><br>
<tt>> -#define KMP_VERSION_BUILD 00000000</tt><br>
<tt>> +//#define KMP_VERSION_BUILD 00000000</tt><br>
<tt>></tt></span><br>
<br>
<tt><span style="font-size:10.0pt">Yep, it did!</span></tt><br>
<tt><span style="font-size:10.0pt"> </span></tt><span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>> --------</tt><br>
<tt>> I don't have the time to review makefile.mk/perl module - someone else </tt>
<br>
<tt>> should do that..</tt><br>
<tt>> # Personally, I think it should be removed</tt><br>
</span><br>
<tt><span style="font-size:10.0pt">runtime/src/makefile.mk is used by the current Makefile (not sure about cmakes). In general, I am happier if Makefile stays in its place.</span></tt><br>
<br>
<br>
<tt><span style="font-size:10.0pt">> ----------</span></tt><span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>> </tt><br>
<tt>> // This looks really weird and I'd need more details before accepting it</tt><br>
<tt>> // I also think we may want to limit this damage to very specifically </tt>
<br>
<tt>> "clang" instead of just PPC64</tt><br>
<tt>> // Please provide more details</tt><br>
<tt>> +__kmp_invoke_microtask</tt><br>
<tt>> </tt><br>
</span><br>
<tt><span style="font-size:10.0pt">About this: I am not aware of any compiler that will target PPC64 using IOMP, except CLANG (I am happy to be told I am wrong). CLANG does currently gather all arguments to the microtask into a struct of pointers. That is the
reason for my comment. There is ongoing discussion at Intel, I think, about why this is/is not bad.</span></tt><br>
<br>
<tt><span style="font-size:10.0pt">From PPC64 perspective, we are happy the way it is. This implementation is coming from Hal Finkel (I am cc-ing him) that actually implemented that function. I had some discussion with my colleagues here and, if eventually
this implementation gets in the way of getting better performance, we shall be able to provide an assembly version of this.</span></tt><br>
<tt><span style="font-size:10.0pt">For this patch, we feel comfortable as it is.</span></tt><br>
<br>
<br>
<tt><span style="font-size:10.0pt">> </span></tt><span style="font-size:10.0pt;font-family:"Courier New""><br>
<tt>> How do you plan to test this?</tt><br>
<tt>> Does it pass the testsuite which has been made available?</tt><br>
<tt>> I believe ANL did some work on this (Jeff Hammond) - is any of his work </tt>
<br>
<tt>> used in this?</tt><br>
</span><br>
<tt><span style="font-size:10.0pt">Yes, it passes the testsuite with similar results as with gcc on x86_64. If you need them, I can provide detailed information.</span></tt><br>
<tt><span style="font-size:10.0pt">Once we have clarified the remaining details, I will provide a new patch.</span></tt><br>
<br>
<br>
<tt><span style="font-size:10.0pt">Thanks again for your detailed review and apologies about my inexperience in llvm-related projects.</span></tt><br>
<br>
<br>
<br>
<tt><span style="font-size:10.0pt">Cheers</span></tt><br>
<br>
<tt><span style="font-size:10.0pt">-- Carlo</span></tt><br>
<br>
<o:p></o:p></p>
</div>
<p>---------------------------------------------------------------------<br>
Intel Corporation (UK) Limited<br>
Registered No. 1134945 (England)<br>
Registered Office: Pipers Way, Swindon SN3 1RJ<br>
VAT No: 860 2173 47</p>
<p>This e-mail and any attachments may contain confidential material for<br>
the sole use of the intended recipient(s). Any review or distribution<br>
by others is strictly prohibited. If you are not the intended<br>
recipient, please contact the sender and delete all copies.</p></body>
</html>