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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:122
@@ +121,3 @@
+  // Compute alias set.
+  for (BasicBlock *BB : L->blocks())
+    CurAST.add(*BB);
----------------
This is not correct -- it should merge in SubLoop's AST too. See LICM's CollectAliasInfoForLoop -- that should be extracted as a common utility.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:131
@@ +130,3 @@
+    Instruction &I = *II++;
+    if (L->hasLoopInvariantOperands(&I) && canSinkToLoopBody(I, AA, L, &CurAST))
+      INS.push_back(&I);
----------------
Is the first check needed?

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:147
@@ +146,3 @@
+    }
+
+    // Find the set of BBs that we should insert a copy of I.
----------------
Contnue if BBs is empty

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:155
@@ +154,3 @@
+        if (BFI->getBlockFreq(CDT) >= PreheaderFreq ||
+            BFI->getBlockFreq(CDT) *
+                    BranchProbability(SinkFrequencyPercentThreshold, 100) >
----------------
This logic seems to be inverted -- Using CDT should be encouraged if its frequency equals the sum of SinkBB and N. In other words, division should be used, not multiplication 

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:183
@@ +182,3 @@
+    }
+    if (ShouldSkip || T > PreheaderFreq ||
+        (SinkBBs.size() > 1 &&
----------------
Is the formatting correct here?


https://reviews.llvm.org/D22778