[PATCH] D13611: [Polly] Create virtual independent blocks

[Tobias Grosser via llvm-commits]

grosser added a comment.

Hi Johannes,

great to see we will finally get rid of the IndependentBlocks pass. I just had a very first skim over this patch, but did not manage to do a full review yet.

The two main items I would still like to look into:

1. This seems to conflict with http://reviews.llvm.org/D12975 (Michael's DE-LICM)

I need to get a better understanding in which ways these patches possibly conflict (as well as the overall design).

2. Post-processing statements vs. instruction-list ScopStmts

Your current approach of first building all ScopStmts and then post-process them to eliminate scalar dependences is interesting. However, it requires a special post-processing phase in ScopInfo and additional special-purpose code in ScopStmt and BlockGenerator. I wonder if it may make sense to build ScopStmts from the beginning just as a list of instructions (instead of a basic block) which could then be uniformly processed in the BlockGenerator.

Best,
Tobias


================
Comment at: include/polly/ScopInfo.h:964
@@ -943,1 +963,3 @@
 
+  /// @brief Add a scalar that need to be recomputed in this statement.
+  void addDependentScalar(Instruction *Inst) { DependentScalars.insert(Inst); }
----------------
that needs to be recomputed

================
Comment at: lib/Analysis/ScopInfo.cpp:1410
@@ -1361,3 +1409,3 @@
   // with all scalar accesses that were caused by them. The tricky iteration
   // order uses is needed because the MemAccs is a vector and the order in
   // which the accesses of each memory access list (MAL) are stored in this
----------------
use is / uses are

================
Comment at: lib/Analysis/ScopInfo.cpp:2708
@@ +2707,3 @@
+  // decide if we can recompute the scalar definition to another statement.
+  // The latter to add read-only scalar accesses to the statement in which the
+  // scalar is recomputed. That allows us to identify values needed e.g., for
----------------
"The latter to add" does not seem to make sense grammatically.

================
Comment at: lib/Analysis/ScopInfo.cpp:2833
@@ +2832,3 @@
+  // access as all users will recompute the value. As we currently cannot simply
+  // use this logic to recompute values at the exit of the SCoP we will not
+  // remove scalars that escape the SCoP.
----------------
grammar

================
Comment at: lib/Analysis/ScopInfo.cpp:2838
@@ +2837,3 @@
+  //       can recompute escaping values in this exit statement and remove
+  //       them from others.
+
----------------
I am not yet convinced an exit ScopStmt is something we would like to have. Maybe leave this for another patch review.


http://reviews.llvm.org/D13611