[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: include/llvm/Transforms/Utils/LoopUtils.h:482
@@ +481,3 @@
+/// Return true if the instruction is hoistable.
+bool isHoistableInst(const Instruction *I);
+}
----------------
Refactored code to remove these functions.

================
Comment at: lib/Transforms/Scalar/LICM.cpp:508-516
@@ -506,4 +507,11 @@
 
-    return false;
-  }
+/// isHoistableInst - Return true if the instruction is hoistable.
+///
+bool llvm::isHoistableInst(const Instruction *I) {
+  return isa<BinaryOperator>(*I) || isa<CastInst>(*I) || isa<SelectInst>(*I) ||
+      isa<GetElementPtrInst>(*I) || isa<CmpInst>(*I) ||
+      isa<InsertElementInst>(*I) || isa<ExtractElementInst>(*I) ||
+      isa<ShuffleVectorInst>(*I) || isa<ExtractValueInst>(*I) ||
+      isa<InsertValueInst>(*I);
+}
 
----------------
refactored code to remove this.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:133
@@ +132,3 @@
+      INS.push_back(&I);
+  }
+  for (auto I : INS) {
----------------
We need reverse iterator because instructions in the back of the BB may depend on the instructions in the front, thus it needs to be sunk first before other instructions can be sunk.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:170
@@ +169,3 @@
+          SinkBB = CDT;
+        }
+      } else {
----------------
Good point.

Refactored the code and updated the algorithm to iterate from cold blocks top ensure optimal.

================
Comment at: lib/Transforms/Scalar/LoopSink.cpp:199
@@ +198,3 @@
+
+    int i = 0;
+    for (BasicBlock *N : SinkBBs) {
----------------
SinkBBs.size() == 0 check is already moved above the total frequency check, so it will not reach here.

The i==0 check is to distinguish between the first SinkBB (that we use move instead of insert) and the later SinkBB (that we make a copy for each insert).


https://reviews.llvm.org/D22778