[llvm] r317116 - Revert 317016 and 317048
Teresa Johnson via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 2 06:53:56 PDT 2017
Hi Phillip,
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?
#0 0x0000563e1afd5fc6 llvm::sys::RunSignalHandlers()
#1 0x0000563e1afd7cf4 SignalHandler(int)
#2 0x00007efcbedf89a0 __restore_rt
#3 0x0000563e1b551ed7
llvm::PHINode::getIncomingValueForBlock(llvm::BasicBlock const*) const (
#4 0x0000563e1ac44593 llvm::simplifyUsersOfIV(llvm::PHINode*,
llvm::ScalarEvolution*, llvm::DominatorTree*, llvm::LoopInfo*,
llvm::SmallVectorImpl<llvm::WeakTrackingVH>&, llvm::SCEVExpander&,
llvm::IVVisitor*)
#5 0x0000563e1a96e423 (anonymous
namespace)::IndVarSimplify::run(llvm::Loop*)
#6 0x0000563e1a96cd9e llvm::IndVarSimplifyPass::run(llvm::Loop&,
llvm::AnalysisManager<llvm::Loop, llvm::LoopStandardAnalysisResults&>&,
llvm::LoopStandardAnalysisResults&, llvm::LPMUpdater&)
...
Thanks,
Teresa
On Wed, Nov 1, 2017 at 12:49 PM, Philip Reames via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Author: reames
> Date: Wed Nov 1 12:49:20 2017
> New Revision: 317116
>
> URL: http://llvm.org/viewvc/llvm-project?rev=317116&view=rev
> Log:
> Revert 317016 and 317048
>
> The former appears to have introduced a miscompile in a stage2 clang
> build. Revert so I can investigate offline.
>
>
> Modified:
> llvm/trunk/lib/Transforms/Utils/SimplifyIndVar.cpp
>
> Modified: llvm/trunk/lib/Transforms/Utils/SimplifyIndVar.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/
> SimplifyIndVar.cpp?rev=317116&r1=317115&r2=317116&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Transforms/Utils/SimplifyIndVar.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/SimplifyIndVar.cpp Wed Nov 1
> 12:49:20 2017
> @@ -83,6 +83,7 @@ namespace {
>
> bool eliminateOverflowIntrinsic(CallInst *CI);
> bool eliminateIVUser(Instruction *UseInst, Instruction *IVOperand);
> + bool makeIVComparisonInvariant(ICmpInst *ICmp, Value *IVOperand);
> void eliminateIVComparison(ICmpInst *ICmp, Value *IVOperand);
> void simplifyIVRemainder(BinaryOperator *Rem, Value *IVOperand,
> bool IsSigned);
> @@ -92,16 +93,6 @@ namespace {
> bool eliminateSDiv(BinaryOperator *SDiv);
> bool strengthenOverflowingOperation(BinaryOperator *OBO, Value
> *IVOperand);
> bool strengthenRightShift(BinaryOperator *BO, Value *IVOperand);
> -
> -
> - private:
> - // Wrapped around ScalarEvolution::isLoopInvariantPredicate which
> returns
> - // true only when a free expansion is available.
> - bool isCheapLoopInvariantPredicate(ICmpInst::Predicate Pred,
> - const SCEV *LHS, const SCEV *RHS, const Loop *L,
> - const SmallDenseMap<const SCEV*, Value*> &FreeExpansions,
> - ICmpInst::Predicate &InvariantPred,
> - Value *&LHSV, Value *& RHSV);
> };
> }
>
> @@ -171,24 +162,62 @@ Value *SimplifyIndvar::foldIVUser(Instru
> return IVSrc;
> }
>
> -bool SimplifyIndvar::isCheapLoopInvariantPredicate(ICmpInst::Predicate
> Pred,
> - const SCEV *LHS, const SCEV *RHS, const Loop *L,
> - const SmallDenseMap<const SCEV*, Value*> &FreeExpansions,
> - ICmpInst::Predicate &InvariantPred,
> - Value *&LHSV, Value *& RHSV) {
> +bool SimplifyIndvar::makeIVComparisonInvariant(ICmpInst *ICmp,
> + Value *IVOperand) {
> + unsigned IVOperIdx = 0;
> + ICmpInst::Predicate Pred = ICmp->getPredicate();
> + if (IVOperand != ICmp->getOperand(0)) {
> + // Swapped
> + assert(IVOperand == ICmp->getOperand(1) && "Can't find IVOperand");
> + IVOperIdx = 1;
> + Pred = ICmpInst::getSwappedPredicate(Pred);
> + }
> +
> + // Get the SCEVs for the ICmp operands (in the specific context of the
> + // current loop)
> + const Loop *ICmpLoop = LI->getLoopFor(ICmp->getParent());
> + const SCEV *S = SE->getSCEVAtScope(ICmp->getOperand(IVOperIdx),
> ICmpLoop);
> + const SCEV *X = SE->getSCEVAtScope(ICmp->getOperand(1 - IVOperIdx),
> ICmpLoop);
>
> + ICmpInst::Predicate InvariantPredicate;
> const SCEV *InvariantLHS, *InvariantRHS;
> - if (!SE->isLoopInvariantPredicate(Pred, LHS, RHS, L, InvariantPred,
> +
> + auto *PN = dyn_cast<PHINode>(IVOperand);
> + if (!PN)
> + return false;
> + if (!SE->isLoopInvariantPredicate(Pred, S, X, L, InvariantPredicate,
> InvariantLHS, InvariantRHS))
> return false;
>
> // Rewrite the comparison to a loop invariant comparison if it can be
> done
> // cheaply, where cheaply means "we don't need to emit any new
> // instructions".
> - LHSV = FreeExpansions.lookup(InvariantLHS);
> - RHSV = FreeExpansions.lookup(InvariantRHS);
>
> - return (LHSV && RHSV);
> + SmallDenseMap<const SCEV*, Value*> CheapExpansions;
> + CheapExpansions[S] = ICmp->getOperand(IVOperIdx);
> + CheapExpansions[X] = ICmp->getOperand(1 - IVOperIdx);
> +
> + // TODO: Support multiple entry loops? (We currently bail out of these
> in
> + // the IndVarSimplify pass)
> + if (auto *BB = L->getLoopPredecessor()) {
> + Value *Incoming = PN->getIncomingValueForBlock(BB);
> + const SCEV *IncomingS = SE->getSCEV(Incoming);
> + CheapExpansions[IncomingS] = Incoming;
> + }
> + Value *NewLHS = CheapExpansions[InvariantLHS];
> + Value *NewRHS = CheapExpansions[InvariantRHS];
> +
> + if (!NewLHS || !NewRHS)
> + // We could not find an existing value to replace either LHS or RHS.
> + // Generating new instructions has subtler tradeoffs, so avoid doing
> that
> + // for now.
> + return false;
> +
> + DEBUG(dbgs() << "INDVARS: Simplified comparison: " << *ICmp << '\n');
> + ICmp->setPredicate(InvariantPredicate);
> + ICmp->setOperand(0, NewLHS);
> + ICmp->setOperand(1, NewRHS);
> + return true;
> }
>
> /// SimplifyIVUsers helper for eliminating useless
> @@ -210,23 +239,6 @@ void SimplifyIndvar::eliminateIVComparis
> const SCEV *S = SE->getSCEVAtScope(ICmp->getOperand(IVOperIdx),
> ICmpLoop);
> const SCEV *X = SE->getSCEVAtScope(ICmp->getOperand(1 - IVOperIdx),
> ICmpLoop);
>
> - SmallDenseMap<const SCEV*, Value*> CheapExpansions;
> - CheapExpansions[S] = ICmp->getOperand(IVOperIdx);
> - CheapExpansions[X] = ICmp->getOperand(1 - IVOperIdx);
> -
> - // TODO: Support multiple entry loops? (We currently bail out of these
> in
> - // the IndVarSimplify pass)
> - auto *PN = dyn_cast<PHINode>(IVOperand);
> - if (PN)
> - if (auto *BB = L->getLoopPredecessor()) {
> - Value *Incoming = PN->getIncomingValueForBlock(BB);
> - const SCEV *IncomingS = SE->getSCEV(Incoming);
> - CheapExpansions[IncomingS] = Incoming;
> - }
> -
> - ICmpInst::Predicate NewPred;
> - Value *NewLHS = nullptr, *NewRHS = nullptr;
> -
> // If the condition is always true or always false, replace it with
> // a constant value.
> if (SE->isKnownPredicate(Pred, S, X)) {
> @@ -237,14 +249,8 @@ void SimplifyIndvar::eliminateIVComparis
> ICmp->replaceAllUsesWith(ConstantInt::getFalse(ICmp->getContext()));
> DeadInsts.emplace_back(ICmp);
> DEBUG(dbgs() << "INDVARS: Eliminated comparison: " << *ICmp << '\n');
> - } else if (PN &&
> - isCheapLoopInvariantPredicate(Pred, S, X, L,
> CheapExpansions,
> - NewPred, NewLHS, NewRHS)) {
> - DEBUG(dbgs() << "INDVARS: Simplified comparison: " << *ICmp << '\n');
> - ICmp->setPredicate(NewPred);
> - assert(NewLHS && NewRHS);
> - ICmp->setOperand(0, NewLHS);
> - ICmp->setOperand(1, NewRHS);
> + } else if (makeIVComparisonInvariant(ICmp, IVOperand)) {
> + // fallthrough to end of function
> } else if (ICmpInst::isSigned(OriginalPred) &&
> SE->isKnownNonNegative(S) && SE->isKnownNonNegative(X)) {
> // If we were unable to make anything above, all we can is to
> canonicalize
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
--
Teresa Johnson | Software Engineer | tejohnson at google.com | 408-460-2413
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171102/4bb5b065/attachment.html>
More information about the llvm-commits
mailing list