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

[Hongbin Zheng via llvm-commits]

etherzhhb added a comment.

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


================
Comment at: include/polly/CodeGen/BlockGenerators.h:134-136
@@ -133,1 +133,5 @@
 
+  Value *getImplicitAddress(MemoryAccess &Access, Loop *L, LoopToScevMapT &LTS,
+                            ValueMapT &BBMap,
+                            __isl_keep isl_id_to_ast_expr *NewAccesses);
+
----------------
Comments to explain what is going on here? (because the dirty code-disclaimer?)

================
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,
----------------
Also talk about @param NewAccesses? (because the dirty code-disclaimer?)

================
Comment at: include/polly/CodeGen/BlockGenerators.h:381
@@ -375,1 +380,3 @@
+                                    ValueMapT &BBMap,
+                                    __isl_keep isl_id_to_ast_expr *NewAccesses);
 
----------------
Here too

================
Comment at: include/polly/CodeGen/BlockGenerators.h:496-499
@@ -486,1 +495,6 @@
 
+  Value *generateLocationAccessed(ScopStmt &Stmt, Loop *L, Value *Pointer,
+                                  ValueMapT &BBMap, LoopToScevMapT &LTS,
+                                  isl_id_to_ast_expr *NewAccesses,
+                                  __isl_take isl_id *Id, Type *ExpectedType);
+
----------------
Comments to explain what is the different between this one and the above one?

isl_id_to_ast_expr *NewAccesses should be an __isl_keep?

================
Comment at: include/polly/CodeGen/BlockGenerators.h:826
@@ -812,1 +825,3 @@
+  generateScalarStores(ScopStmt &Stmt, LoopToScevMapT &LTS, ValueMapT &BBMAp,
+                       __isl_keep isl_id_to_ast_expr *NewAccesses) override;
 
----------------
@param NewAccesses

================
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().
----------------
The MemAccInst here can only be StoreInst?

================
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;
 
----------------
will the same statement map to multiple values?

================
Comment at: include/polly/ScopInfo.h:2295
@@ -2225,1 +2294,3 @@
+                  ArrayRef<const SCEV *> Sizes, ScopArrayInfo::MemoryKind Kind,
+                  MemAccInst AddrAlias);
 
----------------
It would be good to put explanation for the difference between Inst and AddrAlias.

================
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.
+
----------------
Stored at the location indicated by @param Addr?

================
Comment at: include/polly/ScopInfo.h:2399
@@ +2398,3 @@
+
+  /// @brief DeLICM main algorithm.
+  ///
----------------
Can we move this to a ScopPass? Or move the algorithm implementation to another cpp file?

================
Comment at: lib/Analysis/ScopInfo.cpp:3839
@@ -3824,3 +3838,3 @@
 
   if (isa<GetElementPtrInst>(Address) || isa<BitCastInst>(Address)) {
     auto *NewAddress = Address;
----------------
[Unrelated to this patch] Can we put a early return here to reduce the indent levels?

```
if (!(isa<GetElementPtrInst>(Address) || isa<BitCastInst>(Address)))
  return nullptr;

...
```


http://reviews.llvm.org/D12975