<div dir="ltr">Hi Phillip,<div><br></div><div>I am seeing some failures when building with PGO in our internal benchmarks using a compiler built at r317088 (so before this revert went in). They had the following (partial) stack trace, does this sound like the miscompile you saw?</div><div><br></div><div><div>#0 0x0000563e1afd5fc6 llvm::sys::RunSignalHandlers() </div><div>#1 0x0000563e1afd7cf4 SignalHandler(int) </div><div>#2 0x00007efcbedf89a0 __restore_rt</div><div>#3 0x0000563e1b551ed7 llvm::PHINode::getIncomingValueForBlock(llvm::BasicBlock const*) const (</div><div>#4 0x0000563e1ac44593 llvm::simplifyUsersOfIV(llvm::PHINode*, llvm::ScalarEvolution*, llvm::DominatorTree*, llvm::LoopInfo*, llvm::SmallVectorImpl<llvm::WeakTrackingVH>&, llvm::SCEVExpander&, llvm::IVVisitor*) </div><div>#5 0x0000563e1a96e423 (anonymous namespace)::IndVarSimplify::run(llvm::Loop*) </div><div>#6 0x0000563e1a96cd9e llvm::IndVarSimplifyPass::run(llvm::Loop&, llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>&, llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&) </div></div><div>...</div><div><br></div><div>Thanks,</div><div>Teresa</div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Nov 1, 2017 at 12:49 PM, Philip Reames via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: reames<br>
Date: Wed Nov 1 12:49:20 2017<br>
New Revision: 317116<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=317116&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=317116&view=rev</a><br>
Log:<br>
Revert 317016 and 317048<br>
<br>
The former appears to have introduced a miscompile in a stage2 clang build. Revert so I can investigate offline.<br>
<br>
<br>
Modified:<br>
llvm/trunk/lib/Transforms/<wbr>Utils/SimplifyIndVar.cpp<br>
<br>
Modified: llvm/trunk/lib/Transforms/<wbr>Utils/SimplifyIndVar.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyIndVar.cpp?rev=317116&r1=317115&r2=317116&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Transforms/Utils/<wbr>SimplifyIndVar.cpp?rev=317116&<wbr>r1=317115&r2=317116&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Transforms/<wbr>Utils/SimplifyIndVar.cpp (original)<br>
+++ llvm/trunk/lib/Transforms/<wbr>Utils/SimplifyIndVar.cpp Wed Nov 1 12:49:20 2017<br>
@@ -83,6 +83,7 @@ namespace {<br>
<br>
bool eliminateOverflowIntrinsic(<wbr>CallInst *CI);<br>
bool eliminateIVUser(Instruction *UseInst, Instruction *IVOperand);<br>
+ bool makeIVComparisonInvariant(<wbr>ICmpInst *ICmp, Value *IVOperand);<br>
void eliminateIVComparison(ICmpInst *ICmp, Value *IVOperand);<br>
void simplifyIVRemainder(<wbr>BinaryOperator *Rem, Value *IVOperand,<br>
bool IsSigned);<br>
@@ -92,16 +93,6 @@ namespace {<br>
bool eliminateSDiv(BinaryOperator *SDiv);<br>
bool strengthenOverflowingOperation<wbr>(BinaryOperator *OBO, Value *IVOperand);<br>
bool strengthenRightShift(<wbr>BinaryOperator *BO, Value *IVOperand);<br>
-<br>
-<br>
- private:<br>
- // Wrapped around ScalarEvolution::<wbr>isLoopInvariantPredicate which returns<br>
- // true only when a free expansion is available.<br>
- bool isCheapLoopInvariantPredicate(<wbr>ICmpInst::Predicate Pred,<br>
- const SCEV *LHS, const SCEV *RHS, const Loop *L,<br>
- const SmallDenseMap<const SCEV*, Value*> &FreeExpansions,<br>
- ICmpInst::Predicate &InvariantPred,<br>
- Value *&LHSV, Value *& RHSV);<br>
};<br>
}<br>
<br>
@@ -171,24 +162,62 @@ Value *SimplifyIndvar::foldIVUser(<wbr>Instru<br>
return IVSrc;<br>
}<br>
<br>
-bool SimplifyIndvar::<wbr>isCheapLoopInvariantPredicate(<wbr>ICmpInst::Predicate Pred,<br>
- const SCEV *LHS, const SCEV *RHS, const Loop *L,<br>
- const SmallDenseMap<const SCEV*, Value*> &FreeExpansions,<br>
- ICmpInst::Predicate &InvariantPred,<br>
- Value *&LHSV, Value *& RHSV) {<br>
+bool SimplifyIndvar::<wbr>makeIVComparisonInvariant(<wbr>ICmpInst *ICmp,<br>
+ Value *IVOperand) {<br>
+ unsigned IVOperIdx = 0;<br>
+ ICmpInst::Predicate Pred = ICmp->getPredicate();<br>
+ if (IVOperand != ICmp->getOperand(0)) {<br>
+ // Swapped<br>
+ assert(IVOperand == ICmp->getOperand(1) && "Can't find IVOperand");<br>
+ IVOperIdx = 1;<br>
+ Pred = ICmpInst::getSwappedPredicate(<wbr>Pred);<br>
+ }<br>
+<br>
+ // Get the SCEVs for the ICmp operands (in the specific context of the<br>
+ // current loop)<br>
+ const Loop *ICmpLoop = LI->getLoopFor(ICmp-><wbr>getParent());<br>
+ const SCEV *S = SE->getSCEVAtScope(ICmp-><wbr>getOperand(IVOperIdx), ICmpLoop);<br>
+ const SCEV *X = SE->getSCEVAtScope(ICmp-><wbr>getOperand(1 - IVOperIdx), ICmpLoop);<br>
<br>
+ ICmpInst::Predicate InvariantPredicate;<br>
const SCEV *InvariantLHS, *InvariantRHS;<br>
- if (!SE-><wbr>isLoopInvariantPredicate(Pred, LHS, RHS, L, InvariantPred,<br>
+<br>
+ auto *PN = dyn_cast<PHINode>(IVOperand);<br>
+ if (!PN)<br>
+ return false;<br>
+ if (!SE-><wbr>isLoopInvariantPredicate(Pred, S, X, L, InvariantPredicate,<br>
InvariantLHS, InvariantRHS))<br>
return false;<br>
<br>
// Rewrite the comparison to a loop invariant comparison if it can be done<br>
// cheaply, where cheaply means "we don't need to emit any new<br>
// instructions".<br>
- LHSV = FreeExpansions.lookup(<wbr>InvariantLHS);<br>
- RHSV = FreeExpansions.lookup(<wbr>InvariantRHS);<br>
<br>
- return (LHSV && RHSV);<br>
+ SmallDenseMap<const SCEV*, Value*> CheapExpansions;<br>
+ CheapExpansions[S] = ICmp->getOperand(IVOperIdx);<br>
+ CheapExpansions[X] = ICmp->getOperand(1 - IVOperIdx);<br>
+<br>
+ // TODO: Support multiple entry loops? (We currently bail out of these in<br>
+ // the IndVarSimplify pass)<br>
+ if (auto *BB = L->getLoopPredecessor()) {<br>
+ Value *Incoming = PN->getIncomingValueForBlock(<wbr>BB);<br>
+ const SCEV *IncomingS = SE->getSCEV(Incoming);<br>
+ CheapExpansions[IncomingS] = Incoming;<br>
+ }<br>
+ Value *NewLHS = CheapExpansions[InvariantLHS];<br>
+ Value *NewRHS = CheapExpansions[InvariantRHS];<br>
+<br>
+ if (!NewLHS || !NewRHS)<br>
+ // We could not find an existing value to replace either LHS or RHS.<br>
+ // Generating new instructions has subtler tradeoffs, so avoid doing that<br>
+ // for now.<br>
+ return false;<br>
+<br>
+ DEBUG(dbgs() << "INDVARS: Simplified comparison: " << *ICmp << '\n');<br>
+ ICmp->setPredicate(<wbr>InvariantPredicate);<br>
+ ICmp->setOperand(0, NewLHS);<br>
+ ICmp->setOperand(1, NewRHS);<br>
+ return true;<br>
}<br>
<br>
/// SimplifyIVUsers helper for eliminating useless<br>
@@ -210,23 +239,6 @@ void SimplifyIndvar::<wbr>eliminateIVComparis<br>
const SCEV *S = SE->getSCEVAtScope(ICmp-><wbr>getOperand(IVOperIdx), ICmpLoop);<br>
const SCEV *X = SE->getSCEVAtScope(ICmp-><wbr>getOperand(1 - IVOperIdx), ICmpLoop);<br>
<br>
- SmallDenseMap<const SCEV*, Value*> CheapExpansions;<br>
- CheapExpansions[S] = ICmp->getOperand(IVOperIdx);<br>
- CheapExpansions[X] = ICmp->getOperand(1 - IVOperIdx);<br>
-<br>
- // TODO: Support multiple entry loops? (We currently bail out of these in<br>
- // the IndVarSimplify pass)<br>
- auto *PN = dyn_cast<PHINode>(IVOperand);<br>
- if (PN)<br>
- if (auto *BB = L->getLoopPredecessor()) {<br>
- Value *Incoming = PN->getIncomingValueForBlock(<wbr>BB);<br>
- const SCEV *IncomingS = SE->getSCEV(Incoming);<br>
- CheapExpansions[IncomingS] = Incoming;<br>
- }<br>
-<br>
- ICmpInst::Predicate NewPred;<br>
- Value *NewLHS = nullptr, *NewRHS = nullptr;<br>
-<br>
// If the condition is always true or always false, replace it with<br>
// a constant value.<br>
if (SE->isKnownPredicate(Pred, S, X)) {<br>
@@ -237,14 +249,8 @@ void SimplifyIndvar::<wbr>eliminateIVComparis<br>
ICmp->replaceAllUsesWith(<wbr>ConstantInt::getFalse(ICmp-><wbr>getContext()));<br>
DeadInsts.emplace_back(ICmp);<br>
DEBUG(dbgs() << "INDVARS: Eliminated comparison: " << *ICmp << '\n');<br>
- } else if (PN &&<br>
- isCheapLoopInvariantPredicate(<wbr>Pred, S, X, L, CheapExpansions,<br>
- NewPred, NewLHS, NewRHS)) {<br>
- DEBUG(dbgs() << "INDVARS: Simplified comparison: " << *ICmp << '\n');<br>
- ICmp->setPredicate(NewPred);<br>
- assert(NewLHS && NewRHS);<br>
- ICmp->setOperand(0, NewLHS);<br>
- ICmp->setOperand(1, NewRHS);<br>
+ } else if (makeIVComparisonInvariant(<wbr>ICmp, IVOperand)) {<br>
+ // fallthrough to end of function<br>
} else if (ICmpInst::isSigned(<wbr>OriginalPred) &&<br>
SE->isKnownNonNegative(S) && SE->isKnownNonNegative(X)) {<br>
// If we were unable to make anything above, all we can is to canonicalize<br>
<br>
<br>
______________________________<wbr>_________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><span style="font-family:Times;font-size:medium"><table cellspacing="0" cellpadding="0"><tbody><tr style="color:rgb(85,85,85);font-family:sans-serif;font-size:small"><td nowrap style="border-top-style:solid;border-top-color:rgb(213,15,37);border-top-width:2px">Teresa Johnson |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(51,105,232);border-top-width:2px"> Software Engineer |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(0,153,57);border-top-width:2px"> <a href="mailto:tejohnson@google.com" target="_blank">tejohnson@google.com</a> |</td><td nowrap style="border-top-style:solid;border-top-color:rgb(238,178,17);border-top-width:2px"> 408-460-2413</td></tr></tbody></table></span></div>
</div>