[PATCH] D21772: New pass manager for LICM.

[Davide Italiano via llvm-commits]

davide added a comment.

Some comments.


================
Comment at: include/llvm/Transforms/Scalar/LICM.h:1
@@ +1,2 @@
+//===- LoopSimplifyCFG.cpp - Loop CFG Simplification Pass -------*- C++ -*-===//
+//
----------------
LoopSimplifyCFG.cpp -> LICM.h ?

================
Comment at: include/llvm/Transforms/Scalar/LICM.h:41
@@ +40,3 @@
+
+/// Performs basic CFG simplifications to assist other loop passes.
+class LICMPass : public PassInfoMixin<LICMPass> {
----------------
This comment is wrong, no?

================
Comment at: lib/Transforms/Scalar/LICM.cpp:155
@@ -126,2 +154,3 @@
 
   bool doFinalization() override {
+    assert(LICM.getLoopToAliasSetMap().empty() &&
----------------
There's no `doFinalization()` helper in the new PM (but the logic should be either removed or moved to the destructor). Can you make sure this happen? I think this check is kinda important here.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:187
@@ +186,3 @@
+  auto *SE = FAM.getCachedResult<ScalarEvolutionAnalysis>(*F);
+  assert((AA && LI && DT && TLIi && SE) && "Analyses for LICM not available");
+
----------------
I assume `TLIi` is `TLI`? Does this code even build?

================
Comment at: lib/Transforms/Scalar/LICM.cpp:193
@@ +192,3 @@
+    return PreservedAnalyses::all();
+  return getLoopPassPreservedAnalyses();
+}
----------------
The old PM preserves the CFG add a fix for that please.


http://reviews.llvm.org/D21772