[PATCH] D20560: [new PM] port LoopAccessAnalysis to new pass manager (part-1)

[Davide Italiano via llvm-commits]

davide added inline comments.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:684
@@ +683,3 @@
+                      ScalarEvolution *SCEV_, TargetLibraryInfo *TLI_) {
+    AAR = AAR_;
+    DT = DT_;
----------------
I'm not entirely sure what's the preferred LLVM style here, but I seldom (if ever) have seen variables ending with `_` used like this. 
I might miss something (and certainly I didn't compile), but, won't 
`this.AAR = AAR` et similia work?

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:701
@@ +700,3 @@
+  void releaseMemory() {
+    // Invalidate the cache when the pass is freed.
+    LoopAccessInfoMap.clear();
----------------
Move this outside of the function, and make it doxygen, i.e.
`\brief Invalidate the cache when this pass is freed`

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:706
@@ +705,3 @@
+private:
+  /// \brief The cache.
+  DenseMap<Loop *, std::unique_ptr<LoopAccessInfo>> LoopAccessInfoMap;
----------------
This comment is too generic to be somehow useful. I think it should be expanded a bit, while you're here.

================
Comment at: include/llvm/Analysis/LoopAccessAnalysis.h:709
@@ +708,3 @@
+
+  AliasAnalysis *AAR;
+  DominatorTree *DT;
----------------
Why `AAR` and not `AA` as it was before? Also more consistent with how other variables are named.

================
Comment at: lib/Analysis/LoopAccessAnalysis.cpp:1966
@@ -1968,1 +1965,3 @@
+                      &getAnalysis<LoopInfoWrapperPass>().getLoopInfo(),
+                      &getAnalysis<ScalarEvolutionWrapperPass>().getSE(), TLI);
 
----------------
As you're passing `getAnalysis` everywhere, can't you also inline 
`TLIP ? &TLIP->getTLI() : nullptr` instead of assigning `TLI` and passing the variable?

================
Comment at: lib/Transforms/Scalar/LoopLoadElimination.cpp:555
@@ -554,3 +554,3 @@
     for (Loop *L : Worklist) {
-      const LoopAccessInfo &LAI = LAA->getInfo(L, ValueToValueMap());
+      const LoopAccessInfo &LAI = LAA->getInfo().getInfo(L, ValueToValueMap());
       // The actual work is performed by LoadEliminationForLoop.
----------------
I agree with Adam here, the getInfo().getInfo() is just very weird


http://reviews.llvm.org/D20560