[PATCH] [ScalarEvolution] Change data structure to memorize computed result

[Wan, Xiaofei]

Hi, Hal:

No need to sort them,  the reason why it is expensive is that, most of the time, SmallVector only has 1-2 elements since a SCEV only belongs very little loops, and SCEV only belongs to one BB.

Thanks
Wan Xiaofei
-----Original Message-----
From: Hal Finkel [mailto:hfinkel at anl.gov] 
Sent: Thursday, November 07, 2013 11:18 PM
To: reviews+D2117+public+50946a28790208ad at llvm-reviews.chandlerc.com
Cc: llvm-commits at cs.uiuc.edu; atrick at apple.com; Wan, Xiaofei
Subject: Re: [PATCH] [ScalarEvolution] Change data structure to memorize computed result

Wan,

Did you try keeping the SmallVector sorted (and using std::binary_search for the lookup)? I wonder whether that would make this first option a little bit faster.

Thanks again,
Hal

----- Original Message -----
> Hi atrick,
> 
> In ScalarEvolution, the computed loop disposition and block 
> disposition are stored in a map to avoid repeated calculation.
> 
> /// LoopDispositions - Memoized computeLoopDisposition results.
> DenseMap<const SCEV *, std::map<const Loop *, LoopDisposition> > 
> LoopDispositions; /// BlockDispositions - Memoized 
> computeBlockDisposition results.
> DenseMap<const SCEV *, std::map<const BasicBlock *, BlockDisposition>
> > BlockDispositions;
> 
> The profiling data shows It is expensive to look up a result in such 
> two-dimensional map container, even more expensive than calculating 
> them directly each time.
> 
> There are two solutions to cost-down the look-up.
> 1. Usually, a SCEV belongs to very little loops/blocks, so in the 
> upper container, std::map only has 1-2 elements, linear search is 
> faster than map here.
>     DenseMap<const SCEV *, SmallVector<std::pair<const Loop *,
>     LoopDisposition>, 2> > LoopDispositions;
>     DenseMap<const SCEV *, SmallVector<std::pair<const BasicBlock *,
>     BlockDisposition>, 2> > BlockDispositions; 2. Remove the memorized 
> container and calculate them directly each time.
> 
> Both solutions could introduce 1~3% improvement for cases with many 
> scalar expressions (470.lbm in CPUSpec2006); Solution 2 has better 
> improvement than solution1, but I prefer solution 1 since it may 
> benefit some extreme cases.
> 
> 
> http://llvm-reviews.chandlerc.com/D2117
> 
> Files:
>   lib/Analysis/ScalarEvolution.cpp
>   include/llvm/Analysis/ScalarEvolution.h
> 
> Index: lib/Analysis/ScalarEvolution.cpp 
> ===================================================================
> --- lib/Analysis/ScalarEvolution.cpp
> +++ lib/Analysis/ScalarEvolution.cpp
> @@ -5059,15 +5059,15 @@
>  /// original value V is returned.
>  const SCEV *ScalarEvolution::getSCEVAtScope(const SCEV *V, const  
> Loop *L) {
>    // Check to see if we've folded this expression at this loop
>    before.
> -  std::map<const Loop *, const SCEV *> &Values = ValuesAtScopes[V];
> -  std::pair<std::map<const Loop *, const SCEV *>::iterator, bool> 
> Pair =
> -    Values.insert(std::make_pair(L, static_cast<const SCEV *>(0)));
> -  if (!Pair.second)
> -    return Pair.first->second ? Pair.first->second : V;
> +  SmallVector<std::pair<const Loop *, const SCEV *>, 2> &Values =
> ValuesAtScopes[V];
> +  for (unsigned u = 0; u < Values.size(); u++) {
> +    if (Values[u].first == L)
> +      return Values[u].second ? Values[u].second : V;  }
>  
>    // Otherwise compute it.
>    const SCEV *C = computeSCEVAtScope(V, L);
> -  ValuesAtScopes[V][L] = C;
> +  ValuesAtScopes[V].push_back(std::make_pair(L, C));
>    return C;
>  }
>  
> @@ -6727,7 +6727,7 @@
>  
> //===-----------------------------------------------------------------
> -----===//
>  
>  ScalarEvolution::ScalarEvolution()
> -  : FunctionPass(ID), FirstUnknown(0) {
> +  : FunctionPass(ID), ValuesAtScopes(64), LoopDispositions(64),
> BlockDispositions(64), FirstUnknown(0) {
>    initializeScalarEvolutionPass(*PassRegistry::getPassRegistry());
>  }
>  
> @@ -6865,14 +6865,14 @@
>  
>  ScalarEvolution::LoopDisposition
>  ScalarEvolution::getLoopDisposition(const SCEV *S, const Loop *L) {
> -  std::map<const Loop *, LoopDisposition> &Values = 
> LoopDispositions[S];
> -  std::pair<std::map<const Loop *, LoopDisposition>::iterator, bool> 
> Pair =
> -    Values.insert(std::make_pair(L, LoopVariant));
> -  if (!Pair.second)
> -    return Pair.first->second;
> -
> +  SmallVector<std::pair<const Loop *, LoopDisposition>, 2> &Values =
> LoopDispositions[S];
> +  for (unsigned u = 0; u < Values.size(); u++) {
> +    if (Values[u].first == L)
> +      return Values[u].second;
> +  }
>    LoopDisposition D = computeLoopDisposition(S, L);
> -  return LoopDispositions[S][L] = D;
> +  LoopDispositions[S].push_back(std::make_pair(L, D));  return D;
>  }
>  
>  ScalarEvolution::LoopDisposition
> @@ -6964,14 +6964,14 @@
>  
>  ScalarEvolution::BlockDisposition
>  ScalarEvolution::getBlockDisposition(const SCEV *S, const BasicBlock
>  *BB) {
> -  std::map<const BasicBlock *, BlockDisposition> &Values =
> BlockDispositions[S];
> -  std::pair<std::map<const BasicBlock *,
> BlockDisposition>::iterator, bool>
> -    Pair = Values.insert(std::make_pair(BB, DoesNotDominateBlock));
> -  if (!Pair.second)
> -    return Pair.first->second;
> -
> +  SmallVector<std::pair<const BasicBlock *, BlockDisposition>, 2>
> &Values = BlockDispositions[S];
> +  for (unsigned u = 0; u < Values.size(); u++) {
> +    if (Values[u].first == BB)
> +      return Values[u].second;
> +  }
>    BlockDisposition D = computeBlockDisposition(S, BB);
> -  return BlockDispositions[S][BB] = D;
> +  BlockDispositions[S].push_back(std::make_pair(BB, D));
> +  return D;
>  }
>  
>  ScalarEvolution::BlockDisposition
> Index: include/llvm/Analysis/ScalarEvolution.h
> ===================================================================
> --- include/llvm/Analysis/ScalarEvolution.h
> +++ include/llvm/Analysis/ScalarEvolution.h
> @@ -361,18 +361,18 @@
>      /// that we attempt to compute getSCEVAtScope information for,
>      which can
>      /// be expensive in extreme cases.
>      DenseMap<const SCEV *,
> -             std::map<const Loop *, const SCEV *> > ValuesAtScopes;
> +             SmallVector<std::pair<const Loop *, const SCEV *>, 2> >
> ValuesAtScopes;
>  
>      /// LoopDispositions - Memoized computeLoopDisposition results.
>      DenseMap<const SCEV *,
> -             std::map<const Loop *, LoopDisposition> >
> LoopDispositions;
> +             SmallVector<std::pair<const Loop *, LoopDisposition>,
> 2> > LoopDispositions;
>  
>      /// computeLoopDisposition - Compute a LoopDisposition value.
>      LoopDisposition computeLoopDisposition(const SCEV *S, const Loop
>      *L);
>  
>      /// BlockDispositions - Memoized computeBlockDisposition
>      results.
>      DenseMap<const SCEV *,
> -             std::map<const BasicBlock *, BlockDisposition> >
> BlockDispositions;
> +             SmallVector<std::pair<const BasicBlock *,
> BlockDisposition>, 2> > BlockDispositions;
>  
>      /// computeBlockDisposition - Compute a BlockDisposition value.
>      BlockDisposition computeBlockDisposition(const SCEV *S, const
>      BasicBlock *BB);
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory