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

[Dehao Chen via llvm-commits]

danielcdh added inline comments.

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

1. Build a base class ASTLoopTransformation, and make LookInvariantCodeMotion and LoopSink sub class of it. LoopToAliasSetMap and collectAliasInfoForLoop should be protected member of the base class. The logic in the sub class will also be shared with the new pass manager.
2. Build a base class ASTLoopLegacyPass which inherits from LoopPass, and LICMLegacyPass and LoopSinkLegacyPass subclass of it. cloneBasicBlockAnalysis, deleteAnalysisValue and deleteAnalysisLoop should be private member of the base class. Base class also need to have a ASTLoopTransformation pointer to invoke the actual logic shared with new pass manager.

This is definitely doable, but seems an over kill to me because:
1. collectAliasInfoForLoop is an optimization for compile time. And it is not yet available in the new pass manager.
2. The refactoring mainly focuses on abstraction of the old pass manager, which will be replaced by new pass manager soon.
3. There is much complexity involved because new pass manage does not support this optimization, and we need to make it fall back to what we do right now (add all basic blocks to AST) without introducing memory leak.

Comments?

================
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);
----------------
davidxl wrote:
> Is the first check needed?
I think yes because if there is loop variant in its operand, sinking it into the loop may change the value every iteration.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:155
@@ +154,3 @@
+        if (BFI->getBlockFreq(CDT) >= PreheaderFreq ||
+            BFI->getBlockFreq(CDT) *
+                    BranchProbability(SinkFrequencyPercentThreshold, 100) >
----------------
davidxl wrote:
> 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 
It's actually expected: if CDT's frequency is equal or only a little larger than SinkBB+N's frequency, then the check will fail and goes to "else" branch: picking the CDT instead SinkBB.

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


https://reviews.llvm.org/D22778