[PATCH] [Refactor] Replace RegionPasses by FunctionPasses

Wed Mar 4 02:13:20 PST 2015

Hey Tobias,

I attached 2 lnt reports to this mail. The first (report_region.json)
was created using the current polly/master. The second
(report_function.json) was created with the region->function pass patch.
Note that this patch is not yet at its full capacity, however over the 3
runs I measured I got:
  Performance Regressions  16
  Performance Improvements  159
  Unchanged Tests   817
  Total Tests   992
Where as the execution time changes (only 8 in total) should be jitter.

The point I try to make here is, 3 runs with the region pass manager take
  2:35:27 (3 runs region passes)
but 3 runs with the function pass manager only
  2:07:48 (3 runs function passes)
hence we save ~18% of the lnt time (if I didn't do a stupid math mistake).

To be precise, we save compile time "only" for larger functions with small SCoP
coverage but we do not pay for it in any other case.

Does this convince you that region passes have a bad impact on compile time?

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: report_region.json
Type: application/json
Size: 561322 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150304/8de44d81/attachment.json>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: report_function.json
Type: application/json
Size: 561269 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150304/8de44d81/attachment-0001.json>
-------------- 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/20150304/8de44d81/attachment.sig>