[PATCH] D17203: [LICM] Hoist and sink entire inner loops.

[Chris Diamand via llvm-commits]

chrisdiamand_arm added inline comments.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:208
@@ -207,11 +207,3 @@
   for (Loop *InnerL : L->getSubLoops()) {
-    AliasSetTracker *InnerAST = LoopToAliasSetMap[InnerL];
-    assert(InnerAST && "Where is my AST?");
-
-    // What if InnerLoop was modified by other passes ?
-    CurAST->add(*InnerAST);
-
-    // Once we've incorporated the inner loop's AST into ours, we don't need the
-    // subloop's anymore.
-    delete InnerAST;
-    LoopToAliasSetMap.erase(InnerL);
+    auto ASTI = LoopToAliasSetMap.find(InnerL);
+    // Subloop alias info may have already been merged into another subloop's
----------------
This conflicts with r260892 committed yesterday - I'll fix the merge conflicts in the next version.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:659
@@ +658,3 @@
+                          SafetyInfo);
+        SubLoop->verifyLoop();
+        CurLoop->verifyLoop();
----------------
jmolloy wrote:
> These should be inside DEBUG() macros for release builds.
Isn't everything in `verifyLoop()` wrapped in `#ifndef NDEBUG` anyway?

================
Comment at: lib/Transforms/Scalar/LICM.cpp:1437
@@ -1070,4 +1436,3 @@
 void LICM::cloneBasicBlockAnalysis(BasicBlock *From, BasicBlock *To, Loop *L) {
-  AliasSetTracker *AST = LoopToAliasSetMap.lookup(L);
-  if (!AST)
-    return;
+  auto ASTI = LoopToAliasSetMap.find(L);
+  if (ASTI != LoopToAliasSetMap.end()) {
----------------
jmolloy wrote:
> I'm not sure I understand why these have been changed?
The `lookup()` method inserts a null value into `LoopToAliasSetMap` when the key isn't found. This means that the `assert(LoopToAliasSetMap.empty())` statement in `doFinalization()` fails.

`find()` doesn't add an entry when one doesn't already exist, so avoids this.

================
Comment at: test/Analysis/ScalarEvolution/2012-03-26-LoadConstant.ll:1
@@ -1,2 +1,2 @@
-; RUN: opt < %s -basicaa -globalopt -instcombine -loop-rotate -licm -instcombine -indvars -loop-deletion -constmerge -S | FileCheck %s
+; RUN: opt < %s -basicaa -globalopt -instcombine -loop-rotate -licm -instcombine -indvars -constmerge -loop-deletion -S | FileCheck %s
 ; PR11882: ComputeLoadConstantCompareExitLimit crash.
----------------
jmolloy wrote:
> Why have these orders changed?
When I first wrote it I had to switch them for some reason, but I've just tried it again and it's no longer needed. Will put them back.


http://reviews.llvm.org/D17203