Refactor LICM to expose canSinkOrHoistInst to LoopSink pass.

[danielcdh]

Summary

LoopSink pass shares the same canSinkOrHoistInst functionality with LICM pass. This patch exposes this function in preparation of https://reviews.llvm.org/D22778

[danielcdh]

This is also obvious change, commit without review.

[chandlerc]

In D24171#532277, @danielcdh wrote:

This is also obvious change, commit without review.

As with the other patch, please avoid submitting without review after you ask folks to do the review...

Generally, I don't know that it makes sense to split this one out (and I tried to make that clear in the other thread). This is now a public function without any other users, which we try to avoid. Anyways, I would have avoided splitting this one out... Anyways, more a note for the future. I don't think there are really substantive changes needed here.

That said, I *did* have some comments about this part of the change that I hadn't made yet... Again, this is minor stuff, but stuff that indicates the change might not be across the "obvious" bar yet.

llvm/trunk/include/llvm/Transforms/Utils/LoopUtils.h

473–475 ↗

(On Diff #70098)

Please use the modern doxygen format for this comment.

Unrelated and really minor nit pick: I also would suggest using the indicative mood rather than the imperative for API level comments although this isn't super important. The difference would be "Returns true ..." rather than "Return true ...".

llvm/trunk/lib/Transforms/Scalar/LICM.cpp

435–437 ↗

(On Diff #70098)

Please don't leave a duplicate doxygen comment for this API when adding a declaration in a header with its own comment. They have a tendancy to grow stale.

In fact, I'll point out that it is *already stale*.

[danielcdh]

Oops, I saw people approving their own patch and commit without approval from others and thought it's common practice. If that's not the case, I apologize.

Revert r280429 and r280425 in r280453, and will reopen https://reviews.llvm.org/D24168#532233 to address comments.

[danielcdh]

Abondon this revision as it's supposed to be together with the main patch.

include/llvm/Transforms/Utils/LoopUtils.h

 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/EHPersonalities.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/IRBuilder.h"
 
 namespace llvm {
+class AAResults;
 class AliasSet;
 class AliasSetTracker;
 class AssumptionCache;
 class BasicBlock;
 class DataLayout;
 class DominatorTree;
+class Instruction;
 class Loop;
 class LoopInfo;
 class Pass;
 class PredicatedScalarEvolution;
 class PredIteratorCache;
 class ScalarEvolution;
 class SCEV;
 class TargetLibraryInfo;
void addStringMetadataToLoop(Loop *TheLoop, const char *MDString,
                              unsigned V = 0);
 
 /// Helper to consistently add the set of standard passes to a loop pass's \c
 /// AnalysisUsage.
 ///
 /// All loop passes should call this as part of implementing their \c
 /// getAnalysisUsage.
 void getLoopAnalysisUsage(AnalysisUsage &AU);
-}
 
+/// canSinkOrHoistInst - Return true if the hoister and sinker can handle this
+/// instruction. If SafetyInfo is not nullptr, check if the instruction can
+/// execute speculatively.
+bool canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT,
+                        Loop *CurLoop, AliasSetTracker *CurAST,
+                        LoopSafetyInfo *SafetyInfo);
+}
 #endif

lib/Transforms/Scalar/LICM.cpp

static bool isSafeToExecuteUnconditionally(const Instruction &Inst,
                                            const Instruction *CtxI = nullptr);
 static bool pointerInvalidatedByLoop(Value *V, uint64_t Size,
                                      const AAMDNodes &AAInfo,
                                      AliasSetTracker *CurAST);
 static Instruction *
 CloneInstructionInExitBlock(Instruction &I, BasicBlock &ExitBlock, PHINode &PN,
                             const LoopInfo *LI,
                             const LoopSafetyInfo *SafetyInfo);
-static bool canSinkOrHoistInst(Instruction &I, AliasAnalysis *AA,
-                               DominatorTree *DT,
-                               Loop *CurLoop, AliasSetTracker *CurAST,
-                               LoopSafetyInfo *SafetyInfo);
 
 namespace {
 struct LoopInvariantCodeMotion {
   bool runOnLoop(Loop *L, AliasAnalysis *AA, LoopInfo *LI, DominatorTree *DT,
                  TargetLibraryInfo *TLI, ScalarEvolution *SE, bool DeleteAST);
 
   DenseMap<Loop *, AliasSetTracker *> &getLoopToAliasSetMap() {
     return LoopToAliasSetMap;
if (Fn->hasPersonalityFn())
     if (Constant *PersonalityFn = Fn->getPersonalityFn())
       if (isFuncletEHPersonality(classifyEHPersonality(PersonalityFn)))
         SafetyInfo->BlockColors = colorEHFunclets(*Fn);
 }
 
 /// canSinkOrHoistInst - Return true if the hoister and sinker can handle this
 /// instruction.
 ///
-bool canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT,
+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;