[PATCH] D22778: Add Loop Sink pass to reverse the LICM based of basic block frequency.

[David Li via llvm-commits]

davidxl added a comment.

Need to add more smaller test cases (do not need to enable LICM) to cover various scenarios: sinking to one bb, multiple bbs. When there are multiple use bbs, best candidate bb selection;  also negative tests (e.g. aliasing blocking sinking etc).


================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:37
@@ +36,3 @@
+
+static bool SinkLoop(Loop *L, AliasAnalysis *AA, LoopInfo *LI,
+                     DominatorTree *DT, BlockFrequencyInfo *BFI,
----------------
Add statistics for number of instructions sinked etc.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:78
@@ +77,3 @@
+/// Returns true if Child is in subloop of Parent.
+static bool IsInSubLoop(const Loop *Parent, const Loop *Child) {
+  for (; Child; Child = Child->getParentLoop()) {
----------------
LoopBase class has a member method 'contains' which can be used.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:98
@@ +97,3 @@
+  }
+/*  // Loads have extra constraints we have to verify before we can sink them.
+  if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
----------------
Remove dead code

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:133
@@ +132,3 @@
+
+bool SinkLoop(Loop *L, AliasAnalysis *AA, LoopInfo *LI, DominatorTree *DT,
+              BlockFrequencyInfo *BFI, ScalarEvolution *SE) {
----------------
add documentation for the method.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:139
@@ +138,3 @@
+
+  bool Changed = false;
+  AliasSetTracker CurAST(*AA);
----------------
To speed up the pass, perhaps it is better to do a quick scan of the BB of the loop to see if there are any blocks that are really cold (colder than preheader). If there are not, early return.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:143
@@ +142,3 @@
+  SmallVector<Instruction *, 10> INS;
+
+  // Compute alias set.
----------------
add a comment for this variable.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:159
@@ +158,3 @@
+      Instruction *UI = cast<Instruction>(U.getUser());
+      // If the use if phi node, we can not sink I to this BB.
+      if (dyn_cast<PHINode>(UI) ||
----------------
if -> is

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:178
@@ +177,3 @@
+        } else {
+          SinkBB = CDT;
+        }
----------------
Perhaps compare cdt's frequency with the sum of bb and sink bb? If it is greater than the sum, do not use cdt.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:199
@@ +198,3 @@
+    }
+    if (T < PreheaderFreq) {
+      int i = 0;
----------------
if T >= ... early return

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:204
@@ +203,3 @@
+        if (i++ == 0) {
+          I->moveBefore(&*N->getFirstInsertionPt());
+        } else {
----------------
Add debug trace here


https://reviews.llvm.org/D22778