[PATCH] [Refactor] Replace RegionPasses by FunctionPasses

Johannes Doerfert doerfert at cs.uni-saarland.de
Tue Mar 3 05:14:23 PST 2015 

Hey Tobias,

let me give you a short answer now and a more detailed one later:
  
  - Compile time:
    We perform __much__ better on large test with a lot of regions. Here
    are some examples after one lnt run for a release build.

      MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4  -89.42%   345.5400  36.5533   -     -
      SingleSource/Benchmarks/Misc-C++-EH/spirit  -78.71%   33.4433   7.1200  -     -
      MultiSource/Applications/kimwitu++/kc   -71.26%   77.1700   22.1766   -     -
      MultiSource/Benchmarks/MiBench/consumer-typeset/consumer-typeset  -67.55%   65.2370   21.1667   -     -
      SingleSource/UnitTests/DefaultInitDynArrays   -50.00%   0.0200  0.0100  -     -
      SingleSource/Benchmarks/Adobe-C++/simple_types_constant_folding   -42.39%   8.6267  4.9700  -     -
      MultiSource/Applications/sqlite3/sqlite3  -42.16%   60.4734   34.9800   -     -
      MultiSource/Benchmarks/ASCI_Purple/SMG2000/smg2000  -39.79%   19.6970   11.8601   -     -
      SingleSource/Regression/C++/EH/inlined_cleanup  -38.34%   0.0433  0.0267  -     -
      MultiSource/Applications/JM/ldecod/ldecod   -37.42%   23.0533   14.4267   -     -
      MultiSource/Applications/oggenc/oggenc  -33.81%   24.6900   16.3433   -     -
      SingleSource/Benchmarks/Stanford/Treesort   -31.79%   0.0733  0.0500  -     -
      MultiSource/Applications/lemon/lemon  -30.80%   2.8467  1.9700  -     -
      SingleSource/Benchmarks/Shootout/strcat   -30.72%   0.0433  0.0300  -     -
      MultiSource/Applications/JM/lencod/lencod   -28.96%   39.8598   28.3167   -     -
      MultiSource/Benchmarks/PAQ8p/paq8p  -26.53%   4.6367  3.4067

    Note that for a debug build I more or less always time out on some of these
    benchmarks after 500s with the old pass system!  I admit the changes
    (especially in the DependenceInfo pass) can be made even more efficient but
    that is an easy patch I can write afterwards.

  - I don't know enough about the new pass manager to say how this change will
    affect it.

  - SCoPs affecting each other are a misconception by design (at least in my
    opinion). We should make the code generation aware of that (e.g., do not
    split the exit block of a region at 3 different locations in Polly!).
    However, this change passes lnt and all unit tests and we detect exactly the
    same number of SCoPs, that is at least a hint that we can make function
    passes work. With scalar/phi code generation and without independent blocks
    and code prepare the impact of one SCoP on another should be even smaller.

I hope you reconsider this change, at least after I provide more accurate
numbers.

Best regards,
  Johannes


On 03/03, Tobias Grosser wrote:
> Hi Johannes,
> 
> what is the motivation and the impact of this change? The only motivation of this change you give in the commit message is compile time. Did you measure any significant improvements here? I personally think there are very good reasons to change/improve our pass infrastructure, but am surprised compile-time is the one and only that motivates your change.
> 
> I see two main reasons to work on the pass infrastructure:
> 
> 1. Make it work with Chandler's new pass manager
> 
> Chandler's new pass manager uses caching to keep multiple analysis results. I believe when we perform changes to our pass infrastructure, we should try to make sure it will work with Chandler's new pass manager. 
> Besides his last PassManager talk, commits such as https://llvm.org/svn/llvm-project/llvm/trunk@226560 show the idea of using caching results.
> 
> 2. Fix the LoopInfo/RegionInfo misconception
> 
> We currently assume ScopPasses on different regions do not affect each other. However, they indeed
> affect each other, which means code-generating one scop may invalidate the next scop. Hence, we have some hacks in place to detect this invalidated scops. The (implicit) pass order change your patch brings does not seem to improve this in any way.
> 
> I am not saying your patch should solve those two issues, but maybe it is a good time to reason about this and at least understand if they will not negatively affect these changes (or possibly even help to solve some).
> 
> I added a couple of first comments to your patch, but before going further I would like to understand your intentions.
> 
> Cheers,
> Tobias
> 
> 
> ================
> Comment at: include/polly/DependenceInfo.h:1
> @@ -1,2 +1,2 @@
>  //===------ polly/Dependences.h - Polyhedral dependency analysis *- C++ -*-===//
>  //
> ----------------
> Needs update, if renamed.
> 
> ================
> Comment at: include/polly/DependenceInfo.h:24
> @@ -23,3 +23,3 @@
>  #ifndef POLLY_DEPENDENCES_H
>  #define POLLY_DEPENDENCES_H
>  
> ----------------
> Needs update, if renamed.
> 
> ================
> Comment at: include/polly/DependenceInfo.h:140
> @@ +139,3 @@
> +  /// @brief Struct type to remember all kinds of dependences for a SCoP.
> +  struct Dependences {
> +    /// @brief The different kinds of dependences we calculate.
> ----------------
> The idea of introducing an analysis result for each individual Scop
> is very much in line with Chandler's new analysis infrastructure. However, to me it seem you only went half wayby  leaving most functions on the pass itself. Looking at this, introducing per-scop-dependence objects seems to be an almost independent change.
> 
> ================
> Comment at: include/polly/LinkAllPasses.h:31
> @@ -30,3 +30,3 @@
>  llvm::Pass *createDeadCodeElimPass();
> -llvm::Pass *createDependencesPass();
> +llvm::Pass *createDependenceInfoPass();
>  llvm::Pass *createDOTOnlyPrinterPass();
> ----------------
> I would personally not perform such renaming as part this patch, as it causes noise all over the place.
> 
> ================
> Comment at: lib/Analysis/DependenceInfo.cpp:1
> @@ -1,2 +1,2 @@
>  //===- Dependences.cpp - Calculate dependency information for a Scop. -----===//
>  //
> ----------------
> Needs updated if you want to perform renaming.
> 
> ================
> Comment at: lib/Analysis/DependenceInfo.cpp:351
> @@ -351,1 +350,3 @@
> +    D.RAW = isl_union_map_subtract(D.RAW, isl_union_map_copy(D.RED));
> +    D.WAW = isl_union_map_subtract(D.WAW, isl_union_map_copy(D.RED));
>  
> ----------------
> These are a lot of D.*? Would it make sense to make all this functionality part of the Dependences object/class.
> 
> ================
> Comment at: lib/Analysis/DependenceInfo.cpp:453
> @@ -453,1 +452,3 @@
> +  isl_union_map *Dependences =
> +      getDependences(S, TYPE_RAW | TYPE_WAW | TYPE_WAR);
>    isl_space *Space = S.getParamSpace();
> ----------------
> I have the feeling passing the scop to each of this functions unnecessarily complicates the interface. Could we not just once ask for the dependences object of this scop and then work with it?
> 
> http://reviews.llvm.org/D7986
> 
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
> 
> 

-- 

Johannes Doerfert
Researcher / PhD Student

Compiler Design Lab (Prof. Hack)
Saarland University, Computer Science
Building E1.3, Room 4.31

Tel. +49 (0)681 302-57521 : doerfert at cs.uni-saarland.de
Fax. +49 (0)681 302-3065  : http://www.cdl.uni-saarland.de/people/doerfert
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150303/b5911534/attachment.sig>

More information about the llvm-commits mailing list