[PATCH] [Refactor] Replace RegionPasses by FunctionPasses

[Tobias Grosser]

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/