[PATCH][Review Requested][Compilation Time] Calculate hash value and equality comparison within SCEV node itself

[Andrew Trick]

On Feb 27, 2013, at 12:19 PM, "Murali, Sriram" <sriram.murali at intel.com> wrote:
> Can someone please take a look at this patch?

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.

Along those lines, you still need to address Sean Silva's comments from Feb 4:

> One of the most important benchmarks is self-hosting. How is that affected?
> *Please* include your raw data (including hardware, setup, and the build
>  conditions). You may want to look at
> <http://thread.gmane.org/gmane.comp.compilers.llvm.cvs/131170>
> for an example of a simple but convincing format for presenting results (although that
> one was showing that a build-time increase would not be so big, rather than attempting
> to demonstrate a built-time decrease).

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.

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.

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.

In particular:

+    void *nextInFoldingSetBucket;
...
+    void **Buckets;

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).

+  class SCEVSetItem {
+  private:
+    void *nextInFoldingSetBucket;

You can probably eliminate this class and move the field into SCEV since you always immediately cast it to or from a SCEV anyway.

---
Minor style issues:

+    inline void *getNextInBucket() const { return nextInFoldingSetBucket; }

You don't need to declare methods "inline" within the class definition.

+    inline SCEVSetItem* getNextPtr(void *nextInBucketPtr) {

Even for return values, llvm uses this style:
SCEVSetItem *getNextPtr(...

+      unsigned bucketID = Hash & (numBuckets - 1);

Since you have no assert here, at least comment the definition of numBuckets indicating the power-of-two requirement.

+  if (const SCEV *S = UniqueSCEVs.findNodeOrInsertPos(LHS, RHS, IP, hashID))

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.

-Andy
 
> From: Murali, Sriram 
> Sent: Thursday, February 07, 2013 12:45 PM
> To: Nick Lewycky
> Cc: llvm-commits at cs.uiuc.edu
> Subject: RE: [PATCH][Review Requested][Compilation Time] Calculate hash value and equality comparison within SCEV node itself
>  
> Hi Nick,
> 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.
>  
> 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.
>  
> Thanks
> Ram
>  
> -----Original Message-----
> From: Nick Lewycky [mailto:nicholas at mxc.ca] 
> Sent: Wednesday, February 06, 2013 4:23 AM
> To: Murali, Sriram
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH][Review Requested][Compilation Time] Calculate hash value and equality comparison within SCEV node itself
>  
> Murali, Sriram wrote:
> > SCEV is currently implemented as a FoldingSetNode to calculate the
> > hashing value and for equality comparison. This causes some
> > performance loss. This patch aims at improving the compilation time
> > performance by implementing the hashing and equality comparison within
> > SCEV class instead of inheriting FoldingSetNode. The compilation
> > performance gain for this particular refactoring is around 2%.
>  
> What is it about FoldingSet that causes a performance loss? It should not. I want to understand this. :)
>  
> Nick
>  
> > This patch is part of a series of compile time improvements. Although
> > these were originally produced by our colleague Wan Xiaofei, our team
> > consisting of tyler.nowicki at intel.com
> > <mailto:tyler.nowicki at intel.com>; preston.gurd at intel.com
> > <mailto:preston.gurd at intel.com>; and sriram.murali at intel.com
> > <mailto:sriram.murali at intel.com>, are assuming all responsibility for this work.
> <csemapSCEV.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130304/55ccd181/attachment.html>