<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:x="urn:schemas-microsoft-com:office:excel" xmlns:p="urn:schemas-microsoft-com:office:powerpoint" xmlns:a="urn:schemas-microsoft-com:office:access" xmlns:dt="uuid:C2F41010-65B3-11d1-A29F-00AA00C14882" xmlns:s="uuid:BDC6E3F0-6DA3-11d1-A2A3-00AA00C14882" xmlns:rs="urn:schemas-microsoft-com:rowset" xmlns:z="#RowsetSchema" xmlns:b="urn:schemas-microsoft-com:office:publisher" xmlns:ss="urn:schemas-microsoft-com:office:spreadsheet" xmlns:c="urn:schemas-microsoft-com:office:component:spreadsheet" xmlns:odc="urn:schemas-microsoft-com:office:odc" xmlns:oa="urn:schemas-microsoft-com:office:activation" xmlns:html="http://www.w3.org/TR/REC-html40" xmlns:q="http://schemas.xmlsoap.org/soap/envelope/" xmlns:rtc="http://microsoft.com/officenet/conferencing" xmlns:D="DAV:" xmlns:Repl="http://schemas.microsoft.com/repl/" xmlns:mt="http://schemas.microsoft.com/sharepoint/soap/meetings/" xmlns:x2="http://schemas.microsoft.com/office/excel/2003/xml" xmlns:ppda="http://www.passport.com/NameSpace.xsd" xmlns:ois="http://schemas.microsoft.com/sharepoint/soap/ois/" xmlns:dir="http://schemas.microsoft.com/sharepoint/soap/directory/" xmlns:ds="http://www.w3.org/2000/09/xmldsig#" xmlns:dsp="http://schemas.microsoft.com/sharepoint/dsp" xmlns:udc="http://schemas.microsoft.com/data/udc" xmlns:xsd="http://www.w3.org/2001/XMLSchema" xmlns:sub="http://schemas.microsoft.com/sharepoint/soap/2002/1/alerts/" xmlns:ec="http://www.w3.org/2001/04/xmlenc#" xmlns:sp="http://schemas.microsoft.com/sharepoint/" xmlns:sps="http://schemas.microsoft.com/sharepoint/soap/" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns:udcs="http://schemas.microsoft.com/data/udc/soap" xmlns:udcxf="http://schemas.microsoft.com/data/udc/xmlfile" xmlns:udcp2p="http://schemas.microsoft.com/data/udc/parttopart" xmlns:wf="http://schemas.microsoft.com/sharepoint/soap/workflow/" xmlns:dsss="http://schemas.microsoft.com/office/2006/digsig-setup" xmlns:dssi="http://schemas.microsoft.com/office/2006/digsig" xmlns:mdssi="http://schemas.openxmlformats.org/package/2006/digital-signature" xmlns:mver="http://schemas.openxmlformats.org/markup-compatibility/2006" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns:mrels="http://schemas.openxmlformats.org/package/2006/relationships" xmlns:spwp="http://microsoft.com/sharepoint/webpartpages" xmlns:ex12t="http://schemas.microsoft.com/exchange/services/2006/types" xmlns:ex12m="http://schemas.microsoft.com/exchange/services/2006/messages" xmlns:pptsl="http://schemas.microsoft.com/sharepoint/soap/SlideLibrary/" xmlns:spsl="http://microsoft.com/webservices/SharePointPortalServer/PublishedLinksService" xmlns:Z="urn:schemas-microsoft-com:" xmlns:st="" 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 12 (filtered medium)">
<base href="x-msg://2320/">
<style>
<!--
/* Font Definitions */
@font-face
{font-family:Helvetica;
panose-1:2 11 6 4 2 2 2 2 2 4;}
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@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;}
/* 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.MsoListParagraph, li.MsoListParagraph, div.MsoListParagraph
{mso-style-priority:34;
margin-top:0cm;
margin-right:0cm;
margin-bottom:0cm;
margin-left:36.0pt;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
span.apple-style-span
{mso-style-name:apple-style-span;}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-size:10.0pt;}
@page Section1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.Section1
{page:Section1;}
/* List Definitions */
@list l0
{mso-list-id:1080059553;
mso-list-type:hybrid;
mso-list-template-ids:449376144 134807567 134807577 134807579 134807567 134807577 134807579 134807567 134807577 134807579;}
@list l0:level1
{mso-level-tab-stop:none;
mso-level-number-position:left;
text-indent:-18.0pt;}
ol
{margin-bottom:0cm;}
ul
{margin-bottom:0cm;}
-->
</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 style='word-wrap: break-word;
-webkit-nbsp-mode: space;-webkit-line-break: after-white-space'>
<div class=Section1>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>Hi Chris,<o:p></o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>Thanks for the quick review! </span><span style='font-size:11.0pt;
font-family:Wingdings;color:#1F497D'>J</span><span style='font-size:11.0pt;
font-family:"Calibri","sans-serif";color:#1F497D'><o:p></o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<p class=MsoListParagraph style='text-indent:-18.0pt;mso-list:l0 level1 lfo1'><![if !supportLists]><span
style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><span
style='mso-list:Ignore'>1.<span style='font:7.0pt "Times New Roman"'>
</span></span></span><![endif]><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>I’ve used SmallPtrSet with a size of 64 as a wild
finger-in-the-air guess. Do you reckon that’s about right?<o:p></o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<p class=MsoListParagraph style='text-indent:-18.0pt;mso-list:l0 level1 lfo1'><![if !supportLists]><span
style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><span
style='mso-list:Ignore'>2.<span style='font:7.0pt "Times New Roman"'>
</span></span></span><![endif]><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>Fixed, thanks.<o:p></o:p></span></p>
<p class=MsoListParagraph><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<p class=MsoListParagraph style='text-indent:-18.0pt;mso-list:l0 level1 lfo1'><![if !supportLists]><span
style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><span
style='mso-list:Ignore'>3.<span style='font:7.0pt "Times New Roman"'>
</span></span></span><![endif]><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>Omission on my part – removed during my changes and forgot
to re-add it when I found it was actually rather important </span><span
style='font-size:11.0pt;font-family:Wingdings;color:#1F497D'>L</span><span
style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p></o:p></span></p>
<p class=MsoListParagraph><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<p class=MsoListParagraph style='text-indent:-18.0pt;mso-list:l0 level1 lfo1'><![if !supportLists]><span
style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><span
style='mso-list:Ignore'>4.<span style='font:7.0pt "Times New Roman"'>
</span></span></span><![endif]><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>I’ve modified the comments. They still compare the
algorithm versus a naive/obvious approach because I feel that I have to justify
the extra complexity. Are they OK now?<o:p></o:p></span></p>
<p class=MsoListParagraph><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>Cheers,<o:p></o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'><o:p> </o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";
color:#1F497D'>James<o:p></o:p></span></p>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","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"'> Chris Lattner [mailto:clattner@apple.com] <br>
<b>Sent:</b> 15 February 2012 11:50<br>
<b>To:</b> James Molloy<br>
<b>Cc:</b> llvm-commits@cs.uiuc.edu<br>
<b>Subject:</b> Re: [llvm-commits] [PATCH] Reduce complexity of DAGCombiner<o:p></o:p></span></p>
</div>
</div>
<p class=MsoNormal><o:p> </o:p></p>
<p class=MsoNormal><o:p> </o:p></p>
<div>
<div>
<p class=MsoNormal>On Feb 15, 2012, at 2:57 AM, James Molloy 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"'>Hi,<o:p></o:p></span></p>
</div>
<div>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'> <o:p></o:p></span></p>
</div>
<div>
<p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif"'>One
of PlumHall’s testcases generates a function with half a million SDNodes.
This brings out the O(n) complexity in DAGCombiner::AddToWorkList() and means
that in debug mode the test has not been seen to complete compilation, and in
release mode it takes several hours. KCacheGrind showed 99% of the time is
being spent in std::remove, doing linear scans of the DAGCombiner’s
worklist during AddToWorkList.<o:p></o:p></span></p>
</div>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<p class=MsoNormal>Wow, that's bad :), I'm surprised that this terrible
algorithm lasted so long.<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal><br>
<br>
<o:p></o:p></p>
<p class=MsoNormal><span class=apple-style-span><span style='font-size:11.5pt;
font-family:"Calibri","sans-serif"'>My solution was to allow duplicate items in
the vector, and on every pop to query the set to see if the item should
actually be in the vector. If not, discard and grab the next item. This means
that the vector could potentially grow larger than it would previously.</span></span><span
class=apple-style-span><span style='font-size:13.5pt;font-family:"Helvetica","sans-serif"'><o:p></o:p></span></span></p>
</div>
<p class=MsoNormal><o:p> </o:p></p>
<div>
<p class=MsoNormal>The approach makes sense. Some requests on the patch:<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>1. Please use SmallPtrSet<> instead of std::set, it
will be much more efficient (and may help your timings even more). See
also: <a href="http://llvm.org/docs/ProgrammersManual.html#ds_set">http://llvm.org/docs/ProgrammersManual.html#ds_set</a><o:p></o:p></p>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>2. In removeFromWorkList, you can just call
WorkListContents.erase(N) instead of find+erase.<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>3. You removed the comment from AddToWorkList about making
sure that the thing added is next-to-be-processed. As you discovered,
this is important, please keep the comment.<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>4. The comments you added are great, but should be written
explaining the algorithm your adding, not explaining a delta from the old code.
When this goes in the new algorithm will be truth, and the comments
should explain why it works this way.<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>Thanks for finding and fixing this James!<o:p></o:p></p>
</div>
<div>
<p class=MsoNormal><o:p> </o:p></p>
</div>
<div>
<p class=MsoNormal>-Chris<o:p></o:p></p>
</div>
</div>
</body>
</html>