<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"><base href="x-msg://518/"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><div><div>On Feb 27, 2013, at 12:19 PM, "Murali, Sriram" <<a href="mailto:sriram.murali@intel.com">sriram.murali@intel.com</a>> wrote:</div><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><div class="WordSection1" style="page: WordSection1; "><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><span style="color: rgb(31, 73, 125); ">Can someone please take a look at this patch?</span></div></div></div></blockquote><div><br></div><div>Thanks for making the effort to fix this. I agree FoldingSetNode is suboptimal in the context of SCEV. I and probably others have not tried to optimize this because duplicating the FoldingSet implementation did not seem worthwhile. We need a very strong justification. Please include the details of you experiments (full compile lines etc.), and report the improvement on clang selfhost.</div><div><br></div><div>Along those lines, you still need to address Sean Silva's comments from Feb 4:</div><div><br></div><div>> One of the most important benchmarks is self-hosting. How is that affected?</div><div>> *Please* include your raw data (including hardware, setup, and the build</div><div>> conditions). You may want to look at</div><div>> <<a href="http://thread.gmane.org/gmane.comp.compilers.llvm.cvs/131170">http://thread.gmane.org/gmane.comp.compilers.llvm.cvs/131170</a>></div><div>> for an example of a simple but convincing format for presenting results (although that</div><div>> one was showing that a build-time increase would not be so big, rather than attempting</div><div>> to demonstrate a built-time decrease).</div><div><br></div><div>More importantly, although you patch looks good, we need to be sure that the compiler output has not changed. It is easy to improve compile times by introducing an analysis bug that disables optimization. Please verify before checkin. If your patch changes SCEV behavior, someone will eventually find out the hard way. Also please add cases to unittests/Analysis/ScalarEvolutionTest.cpp if the existing tests don't cover your new code.</div><div><br></div><div>If the results are clearly convincing and well verified, then I'm not opposed to this patch. For the record, I would prefer a generic fix. For example, we could adapt DenseSet by using a different LookupKey for each signature of SCEV constructor args. The "SCEVKey" types could specialize getHashValue and isEqual. The DenseSet's key type would be a SCEV* with a cached hash value directly returned by getHashValue() specialization. DenseMap::ValueT would be unused.</div><div><br></div><div>If that's too ambitious, we can save it for later and for now just cleanup the specialized SCEVSet so it isn't as ugly as the fully generic FoldingSet.</div><div><br></div><div>In particular:</div><div><br></div><div>+ void *nextInFoldingSetBucket;</div><div>...</div><div>+ void **Buckets;</div><div><br></div><div>Avoid void* and reinterpret_cast. You can probably do what you need with PointerIntPair<SCEVSetItem*, 1> (or PointerIntPair<SCEV*,1>. If you don't want to use that template, at least make the type intptr_t. Only cast to a pointer when you know it's a pointer and don't use reinterpret_cast (I'm not sure why FoldingSet has such brutal casts).</div><div><br></div><div>+ class SCEVSetItem {</div><div>+ private:</div><div>+ void *nextInFoldingSetBucket;</div><div><br></div><div>You can probably eliminate this class and move the field into SCEV since you always immediately cast it to or from a SCEV anyway.</div><div><br></div><div>---</div><div>Minor style issues:</div><div><br></div><div>+ inline void *getNextInBucket() const { return nextInFoldingSetBucket; }</div><div><br></div><div>You don't need to declare methods "inline" within the class definition.</div><div><br></div><div>+ inline SCEVSetItem* getNextPtr(void *nextInBucketPtr) {</div><div><br></div><div>Even for return values, llvm uses this style:</div><div>SCEVSetItem *getNextPtr(...</div><div><br></div><div>+ unsigned bucketID = Hash & (numBuckets - 1);</div><div><br></div><div>Since you have no assert here, at least comment the definition of numBuckets indicating the power-of-two requirement.</div><div><br></div><div>+ if (const SCEV *S = UniqueSCEVs.findNodeOrInsertPos(LHS, RHS, IP, hashID))</div><div><br></div><div>It is confusing that this API takes a generic LHS, RHS SCEV and assumes UDiv under-the-hood. The UDiv should either be explicit in the name, or the SCEV type should be passed too.</div><div><span style="color: rgb(31, 73, 125); font-size: 11pt; font-family: Calibri, sans-serif; "><br></span></div><div>-Andy</div><div><span style="color: rgb(31, 73, 125); font-size: 11pt; font-family: Calibri, sans-serif; "> </span></div><blockquote type="cite"><div lang="EN-US" link="blue" vlink="purple" style="font-family: Helvetica; font-size: medium; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: 2; text-align: -webkit-auto; text-indent: 0px; text-transform: none; white-space: normal; widows: 2; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px; "><div class="WordSection1" style="page: WordSection1; "><div><div style="border-style: solid none none; border-top-width: 1pt; border-top-color: rgb(181, 196, 223); padding: 3pt 0in 0in; position: static; z-index: auto; "><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><b><span style="font-size: 10pt; font-family: Tahoma, sans-serif; ">From:</span></b><span style="font-size: 10pt; font-family: Tahoma, sans-serif; "><span class="Apple-converted-space"> </span>Murali, Sriram<span class="Apple-converted-space"> </span><br><b>Sent:</b><span class="Apple-converted-space"> </span>Thursday, February 07, 2013 12:45 PM<br><b>To:</b><span class="Apple-converted-space"> </span>Nick Lewycky<br><b>Cc:</b><span class="Apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu" style="color: purple; text-decoration: underline; ">llvm-commits@cs.uiuc.edu</a><br><b>Subject:</b><span class="Apple-converted-space"> </span>RE: [PATCH][Review Requested][Compilation Time] Calculate hash value and equality comparison within SCEV node itself<o:p></o:p></span></div></div></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Hi Nick,<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Using Vtune, our colleague Xiaofei, collected profile information for FoldingSet (see attached). It is observed that the hash calculation part is especially hot with benchmarks having many loops, increasing the overhead within SCEV.<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">The reason being, whenever FoldingSet is used as a hash map in different classes such as SCEV and Selection DAG, all the information is copied into FoldingSetNode, which is a clone of the original object. The aim to solve this is to calculate the hash value based on SCEV itself, rather than using the hash function of FoldingSetNode. Even though this causes rewrite of the hash calculation, simple arithmetic operations such as XOR, AND can be used instead of the complicated hash algorithms in FoldingSetnode.<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Thanks<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Ram<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">-----Original Message-----<br>From: Nick Lewycky [<a href="mailto:nicholas@mxc.ca" style="color: purple; text-decoration: underline; ">mailto:nicholas@mxc.ca</a>]<span class="Apple-converted-space"> </span><br>Sent: Wednesday, February 06, 2013 4:23 AM<br>To: Murali, Sriram<br>Cc:<span class="Apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu" style="color: purple; text-decoration: underline; ">llvm-commits@cs.uiuc.edu</a><br>Subject: Re: [PATCH][Review Requested][Compilation Time] Calculate hash value and equality comparison within SCEV node itself<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Murali, Sriram wrote:<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> SCEV is currently implemented as a FoldingSetNode to calculate the<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> hashing value and for equality comparison. This causes some<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> performance loss. This patch aims at improving the compilation time<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> performance by implementing the hashing and equality comparison within<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> SCEV class instead of inheriting FoldingSetNode. The compilation<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> performance gain for this particular refactoring is around 2%.<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">What is it about FoldingSet that causes a performance loss? It should not. I want to understand this. :)<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">Nick<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; "><o:p> </o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> This patch is part of a series of compile time improvements. Although<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> these were originally produced by our colleague Wan Xiaofei, our team<o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> consisting of<span class="Apple-converted-space"> </span><a href="mailto:tyler.nowicki@intel.com" style="color: purple; text-decoration: underline; "><span style="color: windowtext; text-decoration: none; ">tyler.nowicki@intel.com</span></a><o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> <<a href="mailto:tyler.nowicki@intel.com" style="color: purple; text-decoration: underline; "><span style="color: windowtext; text-decoration: none; ">mailto:tyler.nowicki@intel.com</span></a>>;<span class="Apple-converted-space"> </span><a href="mailto:preston.gurd@intel.com" style="color: purple; text-decoration: underline; "><span style="color: windowtext; text-decoration: none; ">preston.gurd@intel.com</span></a><o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> <<a href="mailto:preston.gurd@intel.com" style="color: purple; text-decoration: underline; "><span style="color: windowtext; text-decoration: none; ">mailto:preston.gurd@intel.com</span></a>>; and<span class="Apple-converted-space"> </span><a href="mailto:sriram.murali@intel.com" style="color: purple; text-decoration: underline; "><span style="color: windowtext; text-decoration: none; ">sriram.murali@intel.com</span></a><o:p></o:p></div><div style="margin: 0in 0in 0.0001pt; font-size: 11pt; font-family: Calibri, sans-serif; ">> <<a href="mailto:sriram.murali@intel.com" style="color: purple; text-decoration: underline; "><span style="color: windowtext; text-decoration: none; ">mailto:sriram.murali@intel.com</span></a>>, are assuming all responsibility for this work.<o:p></o:p></div></div><span><csemapSCEV.patch></span>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" style="color: purple; text-decoration: underline; ">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" style="color: purple; text-decoration: underline; ">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a></div></blockquote></div><br></body></html>