[PATCH] D12975: [Polly] De-LICM and De-GVN (WIP)

[Michael Kruse via llvm-commits]

Meinersbur marked an inline comment as done.
Meinersbur added a comment.

Thanks for your first look.

In http://reviews.llvm.org/D12975#362672, @etherzhhb wrote:

> Can we put the algorithm to a ScopPass or another cpp file?


Explained inline.


================
Comment at: include/polly/CodeGen/BlockGenerators.h:362
@@ -357,2 +361,3 @@
   /// @param Stmt  The statement we generate code for.
   /// @param BBMap A mapping from old values to their new values in this block.
+  void generateScalarLoads(ScopStmt &Stmt, LoopToScevMapT &LTS,
----------------
etherzhhb wrote:
> Also talk about @param NewAccesses? (because the dirty code-disclaimer?)
Before the code is stable enough I don't bother to keep docs up-to-date that don't explain anything new.

Generally I think, we should not propagate such values by arguments through a lot of calls. The doxygen comments get very repetitive. Passing using as class member would be better. If the value is only available a limited time (here: during copyStmt), we could create a class that is valid during this period.

================
Comment at: include/polly/ScopInfo.h:74
@@ +73,3 @@
+///
+/// A MemAccInst gives us both: the SCEV by asking ScalarEvolution on its
+/// PointerArgument, and a Loop using LoopInfo::getLoopFor().
----------------
etherzhhb wrote:
> The MemAccInst here can only be StoreInst?
Kind of.

Yes in contexts of mapping targets. Altough it is only the result of the implementation in greedyCollapse that only StoreInsts are chosen.

No in context where we check if a LoadInst would read the same value, either to see that the value is still used, or whether the LoadInst would be redundant. 

================
Comment at: include/polly/ScopInfo.h:110
@@ -68,2 +109,3 @@
+/// to store that value there temporally there until it is overwritten.
 typedef DenseMap<ScopStmt *, Value *> OutgoingValueMapTy;
 
----------------
etherzhhb wrote:
> will the same statement map to multiple values?
No for OutgoingValueMapTy. There is a OutgoingValueMapTy per mapping location and only one value can be mapped to that location at a Stamt's exit.

================
Comment at: include/polly/ScopInfo.h:2348
@@ +2347,3 @@
+  // a load because its value is not used in the current statement -or-
+  // a store because the value is already stored at the location.
+
----------------
etherzhhb wrote:
> Stored at the location indicated by @param Addr?
Yes.

================
Comment at: include/polly/ScopInfo.h:2399
@@ +2398,3 @@
+
+  /// @brief DeLICM main algorithm.
+  ///
----------------
etherzhhb wrote:
> Can we move this to a ScopPass? Or move the algorithm implementation to another cpp file?
I already suggested this for invariant load hoisting, but I don't remember what Johannes' answer was.

It does not make sense for the algorithm in this implementation. We'd need to split ScopInfo into two parts: before and MemoryAccess creation. A ScopInfo is not useful for outsiders without its MemoryAccesses.

It may become feasible when I move the implementation that modifies exiting MemoryAccesses. If we want such a split of passes, we should also split: Invariant load hoisting, D13611, and this.


http://reviews.llvm.org/D12975