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

[Daniel Berlin via llvm-commits]

dberlin added a subscriber: dberlin.

================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:476
@@ +475,3 @@
+/// Return true if the load instruction is hoistable.
+bool isHoistableLoad(LoadInst *LI, AAResults *AA, AliasSetTracker *CurAST);
+
----------------
1. The comment is not right or specific.
First, this is used for both hoisting and sinking.
Second, it does not say what "hoistable" means.

In fact, this function is really checking "is aliased with loop", you probably should call it something like that and use that.

================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:479
@@ +478,3 @@
+/// Return true if the call instruction is hoistable.
+bool isHoistableCall(CallInst *CI, AAResults *AA, AliasSetTracker *CurAST);
+
----------------
Ditto above

================
Comment at: include/llvm/Transforms/Utils/LoopUtils.h:482
@@ +481,3 @@
+/// Return true if the instruction is hoistable.
+bool isHoistableInst(const Instruction *I);
+}
----------------
This should be "Return true if a non-memory instruction can be handled by the hoister/sinker"

Please don't call it isHoistableInst and then use it for sinking :)


================
Comment at: lib/Transforms/Scalar/LICM.cpp:508
@@ -506,3 +507,3 @@
 
-    return false;
-  }
+/// isHoistableInst - Return true if the instruction is hoistable.
+///
----------------
This is not really right, it returns whether a non-memory instruction is hoistable. :)


================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:41
@@ +40,3 @@
+    "sink-freq-percent-threshold", cl::Hidden, cl::init(90),
+    cl::desc("If sinking would incur duplication, the reduced "
+             "frequency must be lower than this threshold in "
----------------
"Don't sink instructions that require cloning unless they execute less than this percent of the time"

(or whatever) 

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:86
@@ +85,3 @@
+/// canSinkToLoopBody - Return true if the instruction can be sinked into loop
+/// body.
+static bool canSinkToLoopBody(Instruction &I, AliasAnalysis *AA, Loop *CurLoop,
----------------
"sunk into loop body"

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:88
@@ +87,3 @@
+static bool canSinkToLoopBody(Instruction &I, AliasAnalysis *AA, Loop *CurLoop,
+                              AliasSetTracker *CurAST) {
+  // Loads have extra constraints we have to verify before we can hoist them.
----------------
CurLoop is unused?

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:89
@@ +88,3 @@
+                              AliasSetTracker *CurAST) {
+  // Loads have extra constraints we have to verify before we can hoist them.
+  if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
----------------
s/hoist/sink/

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:109
@@ +108,3 @@
+  bool HasColdBB = false;
+  for (BasicBlock *BB : L->blocks())
+    if (BFI->getBlockFreq(BB) <= PreheaderFreq) {
----------------
bool HasColdBB =llvm::any_of(L->blocks(), [&](const BasicBlock *BB) { return BFI->getBlockFreq(BB) <= PreHeaderFreq});

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:132
@@ +131,3 @@
+    if (L->hasLoopInvariantOperands(&I) && canSinkToLoopBody(I, AA, L, &CurAST))
+      INS.push_back(&I);
+  }
----------------
I'm confused.
Why is this necessary, instead of using the reverse iterators and just advancing them when necessary?

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:169
@@ +168,3 @@
+      }
+    }
+    if (SinkBB)
----------------
1. Please factor this out into FindSinkBlocks or something.

2. This is non-deterministic, because you are iterating over a denseset.


I am also confused by this placement strategy.

You are not ordering the blocks in any particular processing order, so you may not actually choose the best sink points, as once you NCA something high in the domtree and something low, NCA will always be something high in the domtree.  If you ordered it so it was the lowest things first (using the DFS numbers or whatever), you may decide multiple intermediate placements are cheaper than what you are doing here.


================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:190
@@ +189,3 @@
+                 BranchProbability(SinkFrequencyPercentThreshold, 100)))
+      continue;
+
----------------
Needs a comment

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:198
@@ +197,3 @@
+      if (i++ == 0) {
+        I->moveBefore(&*N->getFirstInsertionPt());
+      } else {
----------------
This looks ... interesting.

Instead, why not add SinkBBS.size() == 0 check above (so it skips if it's empty).

Then you should simply move the i == 0 case outside of the loop, and the loop is just doing the insertions.


================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:204
@@ +203,3 @@
+        SmallVector<Use *, 4> UV;
+        for (Use &U : I->uses()) {
+          if (DT->dominates(IC, U))
----------------
This is Local.cpp's replaceDominatedUsesWith (if you need a Instruction, Use version, make one :P)


https://reviews.llvm.org/D22778