<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
<meta name="Generator" content="Microsoft Word 14 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:Helvetica;
panose-1:2 11 6 4 2 2 2 2 2 4;}
@font-face
{font-family:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@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:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
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.MsoAcetate, li.MsoAcetate, div.MsoAcetate
{mso-style-priority:99;
mso-style-link:"Balloon Text Char";
margin:0in;
margin-bottom:.0001pt;
font-size:8.0pt;
font-family:"Tahoma","sans-serif";}
span.apple-converted-space
{mso-style-name:apple-converted-space;}
span.BalloonTextChar
{mso-style-name:"Balloon Text Char";
mso-style-priority:99;
mso-style-link:"Balloon Text";
font-family:"Tahoma","sans-serif";}
span.EmailStyle20
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.25in 1.0in 1.25in;}
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-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Hi, Nadav:<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Yes, there is one extreme case quake, in which getVTList() consumes a lot of compilation time(~2%).<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Actually, this patch was sent out by my colleague for review in this Feb(I didn’t have commit access at that time) and it was approved by Chris and Bill Wendling .<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130225/167075.html"><span style="color:windowtext">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20130225/167075.html</span></a><o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">BTW, several other patches aiming at improving compilation time were reviewed and approved but were not submitted due to some changes in our internal org; I will continue
this jobs since I am the author of these patches.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">This patch is just a very minor one and it has a very obvious defect in getVTList since it use linear search.<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Thanks<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Wan Xiaofei<o:p></o:p></span></p>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> Nadav Rotem [mailto:nrotem@apple.com]
<br>
<b>Sent:</b> Friday, October 18, 2013 1:10 PM<br>
<b>To:</b> Wan, Xiaofei<br>
<b>Cc:</b> Owen Anderson; llvm-commits@cs.uiuc.edu LLVM<br>
<b>Subject:</b> Re: [PATCH] [Review Request] Improve getVTList() in SelectionDAG<o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
<p class="MsoNormal">Hi Wan, <o:p></o:p></p>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Have you measured the performance impact of your change (compile time) ? <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
<div>
<p class="MsoNormal">Thanks,<o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">Nadav <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></p>
<div>
<div>
<p class="MsoNormal">On Oct 17, 2013, at 4:40 AM, Wan, Xiaofei <<a href="mailto:xiaofei.wan@intel.com">xiaofei.wan@intel.com</a>> wrote:<o:p></o:p></p>
</div>
<p class="MsoNormal"><br>
<br>
<o:p></o:p></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Ping again.</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Owen, is it OK to submit?</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D"> </span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Thanks</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D">Wan Xiaofei</span><o:p></o:p></p>
</div>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<div>
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span class="apple-converted-space"><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> </span></span><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""><a href="mailto:llvm-commits-bounces@cs.uiuc.edu">llvm-commits-bounces@cs.uiuc.edu</a>
[<a href="mailto:llvm-commits-bounces@cs.uiuc.edu">mailto:llvm-commits-bounces@cs.uiuc.edu</a>]<span class="apple-converted-space"> </span><b>On Behalf Of<span class="apple-converted-space"> </span></b>Wan, Xiaofei<br>
<b>Sent:</b><span class="apple-converted-space"> </span>Sunday, October 13, 2013 12:50 AM<br>
<b>To:</b><span class="apple-converted-space"> </span>Owen Anderson<br>
<b>Cc:</b><span class="apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a> LLVM<br>
<b>Subject:</b><span class="apple-converted-space"> </span>RE: [PATCH] [Review Request] Improve getVTList() in SelectionDAG</span><o:p></o:p></p>
</div>
</div>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Owen:</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">FoldingSet can implement similar concept, thanks.</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Patch updated<span class="apple-converted-space"> </span><a href="http://llvm-reviews.chandlerc.com/D1127"><span style="color:windowtext">http://llvm-reviews.chandlerc.com/D1127</span></a></span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">BTW, FoldingSet is a good data structure, but it copies many contents when implementing hash and cause a big performance penalty since it is wildly used in SelectionDAG;
meanwhile the hash value is calculated many times for same object. Our profiling data shows it consume about ~3-~5% of total time in llc; We may need to optimize it.</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif""> </span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Thanks</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif"">Wan Xiaofei</span><o:p></o:p></p>
</div>
<div>
<div style="border:none;border-top:solid #B5C4DF 1.0pt;padding:3.0pt 0in 0in 0in">
<div>
<p class="MsoNormal"><b><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">From:</span></b><span class="apple-converted-space"><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif""> </span></span><span style="font-size:10.0pt;font-family:"Tahoma","sans-serif"">Owen
Anderson [<a href="mailto:resistor@mac.com"><span style="color:purple">mailto:resistor@mac.com</span></a>]<span class="apple-converted-space"> </span><br>
<b>Sent:</b><span class="apple-converted-space"> </span>Saturday, October 12, 2013 2:08 AM<br>
<b>To:</b><span class="apple-converted-space"> </span>Wan, Xiaofei<br>
<b>Cc:</b><span class="apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu"><span style="color:purple">llvm-commits@cs.uiuc.edu</span></a><span class="apple-converted-space"> </span>LLVM<br>
<b>Subject:</b><span class="apple-converted-space"> </span>Re: [PATCH] [Review Request] Improve getVTList() in SelectionDAG</span><o:p></o:p></p>
</div>
</div>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<div>
<p class="MsoNormal">OK, I can see that it works. It seems like you’re basically implementing the same idiom as the existing FoldingSet class (<a href="http://llvm.org/docs/ProgrammersManual.html#llvm-adt-foldingset-h"><span style="color:purple">http://llvm.org/docs/ProgrammersManual.html#llvm-adt-foldingset-h</span></a>).
Can you see if you could make use of that rather than rolling your own solution?<o:p></o:p></p>
</div>
<div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal">—Owen<o:p></o:p></p>
</div>
</div>
<div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
<div>
<div>
<div>
<p class="MsoNormal">On Oct 10, 2013, at 10:26 PM, Wan, Xiaofei <<a href="mailto:xiaofei.wan@intel.com"><span style="color:purple">xiaofei.wan@intel.com</span></a>> wrote:<o:p></o:p></p>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"> <o:p></o:p></p>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:9.0pt">Owen:<br>
<br>
Thanks for your comments.<span class="apple-converted-space"> </span><br>
Patch updated<span class="apple-converted-space"> </span><a href="http://llvm-reviews.chandlerc.com/D1127"><span style="color:purple">http://llvm-reviews.chandlerc.com/D1127</span></a><br>
<br>
SDVTList SelectionDAG::getVTList(EVT VT1, EVT VT2) {<br>
const EVT tmpVTs[2] = {VT1, VT2};<br>
return getVTList(tmpVTs, 2);<br>
}<br>
How does the lifetime of this array work out? It looks like SDVTListInfo captures the pointer to the EVT array, which was stack allocated. After this method returns, you're going to have a dangling pointer in the SDVTListInfo.<br>
[Xiaofei] Yes, tmpVTs is an array on the stack, but it is just used temporally to determine whether this SDVTList exist in the DenseMap, it won't be referenced anymore after that.<br>
<br>
SDVTList SelectionDAG::getVTList(const EVT *VTs, unsigned NumVTs) {<br>
SDVTListInfo Key(VTs, NumVTs);<br>
if (VTListMap.count(Key))<br>
return VTListMap[Key];<br>
<br>
EVT *Array = Allocator.Allocate<EVT>(NumVTs);<br>
std::copy(VTs, VTs + NumVTs, Array); -------------------------------------------------VTs is copied out to Array which will be stored in the DenseMap<br>
SDVTList Result = makeVTList(Array, NumVTs);<br>
Key.pVTs = Array; --------------------------------------------------VTs is not used any more.<br>
VTListMap.insert(std::make_pair(Key, Result));<br>
<br>
return Result;<br>
}<br>
<br>
Thanks<br>
Wan Xiaofei<br>
<br>
-----Original Message-----<br>
From: Owen Anderson [<a href="mailto:resistor@mac.com"><span style="color:purple">mailto:resistor@mac.com</span></a>]<span class="apple-converted-space"> </span><br>
Sent: Friday, October 11, 2013 1:30 AM<br>
To: Wan, Xiaofei<br>
Cc:<span class="apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu"><span style="color:purple">llvm-commits@cs.uiuc.edu</span></a><span class="apple-converted-space"> </span>LLVM<br>
Subject: Re: [PATCH] [Review Request] Improve getVTList() in SelectionDAG<br>
<br>
Some feedback:<br>
<br>
<br>
</span><o:p></o:p></p>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt">+ /// getEmptyKey() - A private constructor that returns an unknown<span class="apple-converted-space"> </span><br>
+ that is /// not equal to the tombstone key or SDVTListInfo().<br>
+ static SDVTListInfo getEmptyKey() {<br>
+ SDVTListInfo SDVTInfo;<br>
+ SDVTInfo.NumVTs = 0;<br>
+ return SDVTInfo;<br>
+ }<br>
+<br>
+ /// getTombstoneKey() - A private constructor that returns an<span class="apple-converted-space"> </span><br>
+ unknown that /// is not equal to the empty key or SDVTListInfo().<br>
+ static SDVTListInfo getTombstoneKey() {<br>
+ SDVTListInfo SDVTInfo;<br>
+ SDVTInfo.NumVTs = 0;<br>
+ return SDVTInfo;<br>
+ }</span><o:p></o:p></p>
</div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><span style="font-size:9.0pt"><br>
I don't think this is a valid implementation of the DenseMapInfo callbacks. getEmptyKey() and getTombstoneKey() have to return values that don't compare equal to each other.<br>
<br>
<br>
</span><o:p></o:p></p>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt">+ const EVT pVTs[2] = {VT1, VT2};<br>
+ return getVTList(pVTs, 2);</span><o:p></o:p></p>
</div>
<div>
<p class="MsoNormal"><span style="font-size:9.0pt"><br>
How does the lifetime of this array work out? It looks like SDVTListInfo captures the pointer to the EVT array, which was stack allocated. After this method returns, you're going to have a dangling pointer in the SDVTListInfo.<br>
<br>
-Owen<br>
<br>
<D1127.5.patch></span><o:p></o:p></p>
</div>
</div>
</div>
<div>
<p class="MsoNormal"> <o:p></o:p></p>
</div>
</div>
<p class="MsoNormal"><span style="font-size:9.0pt;font-family:"Helvetica","sans-serif"">_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><o:p></o:p></span></p>
</div>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</body>
</html>