[PATCH] D21772: New pass manager for LICM.

[Dehao Chen via llvm-commits]

danielcdh added inline comments.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:155
@@ -126,2 +154,3 @@
 
   bool doFinalization() override {
+    assert(LICM.getLoopToAliasSetMap().empty() &&
----------------
davide wrote:
> 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.
The doFinalization performs a legality check for the caching data structure LoopToAliasSetMap. However, as the new pass manage is lack of equivalence of the following helper functions:

cloneBasicBlockAnalysis
deleteAnalysisValue
deleteAnalysisLoop

As a result, LoopToAliasSetMap was not used in the new pass manager implementation.

I've added a FIXME to collectAliasInfoForLoop to clarify this and remind hooking it up once available.

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

================
Comment at: lib/Transforms/Scalar/LICM.cpp:1122
@@ -1097,3 +1121,3 @@
 ///
-void LICM::cloneBasicBlockAnalysis(BasicBlock *From, BasicBlock *To, Loop *L) {
-  AliasSetTracker *AST = LoopToAliasSetMap.lookup(L);
+void LegacyLICMPass::cloneBasicBlockAnalysis(BasicBlock *From, BasicBlock *To,
+                                             Loop *L) {
----------------
silvas wrote:
> This seems to be called by the LPM in the old pass manager as a notification. I don't think there is an analog in the new PM -- does there need to be? What happens if this is not called?
For LICM, these helper functions are for compile time purposes: avoid recompute the AST for the inner loop when processing the outer loop. And you are right, these notifications does not exist in new PM. It would be best if we can have that in new PM. But without it, when processing the outer loop, it will just recompute the AST for the inner loop. So it will affect compile time but not generated code.

================
Comment at: test/Transforms/LICM/hoist-deref-load.ll:2
@@ -1,2 +1,3 @@
 ; RUN: opt -S -basicaa -licm < %s | FileCheck %s
+; RUN: opt -loop-simplifycfg %s | opt -aa-pipeline=basic-aa -passes='require<aa>,require<targetir>,require<scalar-evolution>,loop(licm)' -S | FileCheck %s
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
davidxl wrote:
> Is loop-simplifycfg available in new pm?
Yes, it is. But I'm not sure how to specify to make it run prior to licm in the -passes= option. any ideas?


http://reviews.llvm.org/D21772