[llvm] r338671 - [LICM] Factor out fault legality from canHoistOrSinkInst [NFC]
Philip Reames via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 1 21:08:05 PDT 2018
Author: reames
Date: Wed Aug 1 21:08:04 2018
New Revision: 338671
URL: http://llvm.org/viewvc/llvm-project?rev=338671&view=rev
Log:
[LICM] Factor out fault legality from canHoistOrSinkInst [NFC]
This method has three callers, each of which wanted distinct handling:
1) Sinking into a loop is moving an instruction known to execute before a loop into the loop. We don't need to worry about introducing a fault at all in this case.
2) Hoisting from a loop into a preheader already duplicated the check in the caller.
3) Sinking from the loop into an exit block was the only true user of the code within the routine. For the moment, this has just been lifted into the caller, but up next is examining the logic more carefully. Whitelisting of loads and calls - while consistent with the previous code - is rather suspicious. Either way, a behavior change is worthy of it's own patch.
Modified:
llvm/trunk/include/llvm/Transforms/Utils/LoopUtils.h
llvm/trunk/lib/Transforms/Scalar/LICM.cpp
llvm/trunk/lib/Transforms/Scalar/LoopSink.cpp
Modified: llvm/trunk/include/llvm/Transforms/Utils/LoopUtils.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Utils/LoopUtils.h?rev=338671&r1=338670&r2=338671&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Transforms/Utils/LoopUtils.h (original)
+++ llvm/trunk/include/llvm/Transforms/Utils/LoopUtils.h Wed Aug 1 21:08:04 2018
@@ -497,16 +497,18 @@ Optional<unsigned> getLoopEstimatedTripC
/// getAnalysisUsage.
void getLoopAnalysisUsage(AnalysisUsage &AU);
-/// Returns true if the hoister and sinker can handle this instruction.
-/// If SafetyInfo is null, we are checking for sinking instructions from
-/// preheader to loop body (no speculation).
-/// If SafetyInfo is not null, we are checking for hoisting/sinking
-/// instructions from loop body to preheader/exit. Check if the instruction
-/// can execute speculatively.
+/// Returns true if is legal to hoist or sink this instruction disregarding the
+/// possible introduction of faults. Reasoning about potential faulting
+/// instructions is the responsibility of the caller since it is challenging to
+/// do efficiently from within this routine.
+/// \p TargetExecutesOncePerLoop is true only when it is guaranteed that the
+/// target executes at most once per execution of the loop body. This is used
+/// to assess the legality of duplicating atomic loads. Generally, this is
+/// true when moving out of loop and not true when moving into loops.
/// If \p ORE is set use it to emit optimization remarks.
bool canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT,
Loop *CurLoop, AliasSetTracker *CurAST,
- LoopSafetyInfo *SafetyInfo,
+ bool TargetExecutesOncePerLoop,
OptimizationRemarkEmitter *ORE = nullptr);
/// Generates an ordered vector reduction using extracts to reduce the value.
Modified: llvm/trunk/lib/Transforms/Scalar/LICM.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LICM.cpp?rev=338671&r1=338670&r2=338671&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/LICM.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/LICM.cpp Wed Aug 1 21:08:04 2018
@@ -412,7 +412,14 @@ bool llvm::sinkRegion(DomTreeNode *N, Al
//
bool FreeInLoop = false;
if (isNotUsedOrFreeInLoop(I, CurLoop, SafetyInfo, TTI, FreeInLoop) &&
- canSinkOrHoistInst(I, AA, DT, CurLoop, CurAST, SafetyInfo, ORE)) {
+ canSinkOrHoistInst(I, AA, DT, CurLoop, CurAST, true, ORE) &&
+ // FIXME: Remove the special casing here. Why do we need the
+ // execution check at all? We're sinking from a dominating location
+ // to a dominated location.
+ (isa<LoadInst>(I) || isa<CallInst>(I) ||
+ // TODO: Plumb the context instruction through to make sinking
+ // more powerful.
+ isSafeToExecuteUnconditionally(I, DT, CurLoop, SafetyInfo, ORE))) {
if (sink(I, LI, DT, CurLoop, SafetyInfo, ORE, FreeInLoop)) {
if (!FreeInLoop) {
++II;
@@ -482,7 +489,7 @@ bool llvm::hoistRegion(DomTreeNode *N, A
// if it is safe to hoist the instruction.
//
if (CurLoop->hasLoopInvariantOperands(&I) &&
- canSinkOrHoistInst(I, AA, DT, CurLoop, CurAST, SafetyInfo, ORE) &&
+ canSinkOrHoistInst(I, AA, DT, CurLoop, CurAST, true, ORE) &&
(IsMustExecute ||
isSafeToExecuteUnconditionally(
I, DT, CurLoop, SafetyInfo, ORE,
@@ -592,15 +599,12 @@ bool isHoistableAndSinkableInst(Instruct
bool llvm::canSinkOrHoistInst(Instruction &I, AAResults *AA, DominatorTree *DT,
Loop *CurLoop, AliasSetTracker *CurAST,
- LoopSafetyInfo *SafetyInfo,
+ bool TargetExecutesOncePerLoop,
OptimizationRemarkEmitter *ORE) {
// If we don't understand the instruction, bail early.
if (!isHoistableAndSinkableInst(I))
return false;
- // SafetyInfo is nullptr if we are checking for sinking from preheader to
- // loop body.
- const bool SinkingToLoopBody = !SafetyInfo;
// Loads have extra constraints we have to verify before we can hoist them.
if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
if (!LI->isUnordered())
@@ -613,8 +617,8 @@ bool llvm::canSinkOrHoistInst(Instructio
if (LI->getMetadata(LLVMContext::MD_invariant_load))
return true;
- if (LI->isAtomic() && SinkingToLoopBody)
- return false; // Don't sink unordered atomic loads to loop body.
+ if (LI->isAtomic() && !TargetExecutesOncePerLoop)
+ return false; // Don't risk duplicating unordered loads
// This checks for an invariant.start dominating the load.
if (isLoadInvariantInLoop(LI, DT, CurLoop))
@@ -685,15 +689,9 @@ bool llvm::canSinkOrHoistInst(Instructio
return false;
}
- // If we are checking for sinking from preheader to loop body it will be
- // always safe as there is no speculative execution.
- if (SinkingToLoopBody)
- return true;
-
- // TODO: Plumb the context instruction through to make hoisting and sinking
- // more powerful. Hoisting of loads already works due to the special casing
- // above.
- return isSafeToExecuteUnconditionally(I, DT, CurLoop, SafetyInfo, nullptr);
+ // We've established mechanical ability and aliasing, it's up to the caller
+ // to check fault safety
+ return true;
}
/// Returns true if a PHINode is a trivially replaceable with an
Modified: llvm/trunk/lib/Transforms/Scalar/LoopSink.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/LoopSink.cpp?rev=338671&r1=338670&r2=338671&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/LoopSink.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/LoopSink.cpp Wed Aug 1 21:08:04 2018
@@ -290,7 +290,7 @@ static bool sinkLoopInvariantInstruction
// No need to check for instruction's operands are loop invariant.
assert(L.hasLoopInvariantOperands(I) &&
"Insts in a loop's preheader should have loop invariant operands!");
- if (!canSinkOrHoistInst(*I, &AA, &DT, &L, &CurAST, nullptr))
+ if (!canSinkOrHoistInst(*I, &AA, &DT, &L, &CurAST, false))
continue;
if (sinkInstruction(L, *I, ColdLoopBBs, LoopBlockNumber, LI, DT, BFI))
Changed = true;
More information about the llvm-commits
mailing list