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

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

(Trying to first clarify the split-off of the patch I'm suggesting...)


================
Comment at: lib/Transforms/Scalar/LICM.cpp:438-447
@@ -441,4 +437,12 @@
 ///
-bool canSinkOrHoistInst(Instruction &I, AliasAnalysis *AA, DominatorTree *DT,
-                        TargetLibraryInfo *TLI, Loop *CurLoop,
-                        AliasSetTracker *CurAST, LoopSafetyInfo *SafetyInfo) {
+bool llvm::canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT,
+                              Loop *CurLoop, AliasSetTracker *CurAST,
+                              LoopSafetyInfo *SafetyInfo) {
+  if (!isa<LoadInst>(I) && !isa<CallInst>(I) &&
+      !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))
+    return false;
+
----------------
chandlerc wrote:
> Can you split these refactorings into a separate patch please? They seem strictly beneficial and unrelated, and will make for example reverts messier if left in as part of this patch.
> 
> I have several minor and boring comments on the refactorings, but it seems better to make them on a dedicated patch than to clutter this thread with them.
> 
> (Just to be clear, I'd would leave it a static function, and just get the API you want and the implementation improvements. Then in this patch you can just make it an external function.)
I see that we got confused here and in the other review.

The part of this refactoring I *do* think makes sense to split out and send for review independently is changing the signature (for example, removing TargetLibraryInfo) and re-organizing the implementation.

The only part I think needs to happen with this patch is making this routine be a public routine in the 'llvm' namespace.

Does that make more sense?


https://reviews.llvm.org/D22778