From 5ceef263505d13e5394efacdf1fd9d8949737c15 Mon Sep 17 00:00:00 2001 From: Juneyoung Lee Date: Thu, 23 Apr 2020 08:06:53 +0900 Subject: [PATCH] Revert "RFC: [ValueTracking] Let analyses assume a value cannot be partially poison" This reverts commit 80faa8c3af856df93faf909f21cdcc397bed068f. --- llvm/include/llvm/Analysis/ValueTracking.h | 24 ++++++++++++++-------- llvm/lib/Analysis/ScalarEvolution.cpp | 6 +++--- llvm/lib/Analysis/ValueTracking.cpp | 12 +++++------ .../Transforms/Instrumentation/PoisonChecking.cpp | 6 +++--- llvm/lib/Transforms/Scalar/IndVarSimplify.cpp | 2 +- .../Scalar/SeparateConstOffsetFromGEP.cpp | 4 ++-- .../Analysis/ScalarEvolution/flags-from-poison.ll | 2 +- 7 files changed, 32 insertions(+), 24 deletions(-) diff --git a/llvm/include/llvm/Analysis/ValueTracking.h b/llvm/include/llvm/Analysis/ValueTracking.h index d3729df..fa0da0e 100644 --- a/llvm/include/llvm/Analysis/ValueTracking.h +++ b/llvm/include/llvm/Analysis/ValueTracking.h @@ -563,26 +563,34 @@ class Value; const Loop *L); /// Return true if this function can prove that I is guaranteed to yield - /// poison if at least one of its operands is poison. - bool propagatesPoison(const Instruction *I); + /// full-poison (all bits poison) if at least one of its operands are + /// full-poison (all bits poison). + /// + /// The exact rules for how poison propagates through instructions have + /// not been settled as of 2015-07-10, so this function is conservative + /// and only considers poison to be propagated in uncontroversial + /// cases. There is no attempt to track values that may be only partially + /// poison. + bool propagatesFullPoison(const Instruction *I); /// Return either nullptr or an operand of I such that I will trigger - /// undefined behavior if I is executed and that operand has a poison - /// value. - const Value *getGuaranteedNonPoisonOp(const Instruction *I); + /// undefined behavior if I is executed and that operand has a full-poison + /// value (all bits poison). + const Value *getGuaranteedNonFullPoisonOp(const Instruction *I); /// Return true if the given instruction must trigger undefined behavior. /// when I is executed with any operands which appear in KnownPoison holding - /// a poison value at the point of execution. + /// a full-poison value at the point of execution. bool mustTriggerUB(const Instruction *I, const SmallSet& KnownPoison); /// Return true if this function can prove that if PoisonI is executed - /// and yields a poison value, then that will trigger undefined behavior. + /// and yields a full-poison value (all bits poison), then that will + /// trigger undefined behavior. /// /// Note that this currently only considers the basic block that is /// the parent of I. - bool programUndefinedIfPoison(const Instruction *PoisonI); + bool programUndefinedIfFullPoison(const Instruction *PoisonI); /// Return true if I can create poison from non-poison operands. /// For vectors, canCreatePoison returns true if there is potential poison in diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp index 75891ae..690acb2 100644 --- a/llvm/lib/Analysis/ScalarEvolution.cpp +++ b/llvm/lib/Analysis/ScalarEvolution.cpp @@ -6072,7 +6072,7 @@ bool ScalarEvolution::isSCEVExprNeverPoison(const Instruction *I) { return false; // Only proceed if we can prove that I does not yield poison. - if (!programUndefinedIfPoison(I)) + if (!programUndefinedIfFullPoison(I)) return false; // At this point we know that if I is executed, then it does not wrap @@ -6152,7 +6152,7 @@ bool ScalarEvolution::isAddRecNeverPoison(const Instruction *I, const Loop *L) { SmallVector PoisonStack; // We start by assuming \c I, the post-inc add recurrence, is poison. Only - // things that are known to be poison under that assumption go on the + // things that are known to be fully poison under that assumption go on the // PoisonStack. Pushed.insert(I); PoisonStack.push_back(I); @@ -6162,7 +6162,7 @@ bool ScalarEvolution::isAddRecNeverPoison(const Instruction *I, const Loop *L) { const Instruction *Poison = PoisonStack.pop_back_val(); for (auto *PoisonUser : Poison->users()) { - if (propagatesPoison(cast(PoisonUser))) { + if (propagatesFullPoison(cast(PoisonUser))) { if (Pushed.insert(cast(PoisonUser)).second) PoisonStack.push_back(cast(PoisonUser)); } else if (auto *BI = dyn_cast(PoisonUser)) { diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp index 5ef5abc..be5d18f 100644 --- a/llvm/lib/Analysis/ValueTracking.cpp +++ b/llvm/lib/Analysis/ValueTracking.cpp @@ -4720,7 +4720,7 @@ bool llvm::isGuaranteedNotToBeUndefOrPoison(const Value *V, } if (auto I = dyn_cast(V)) { - if (programUndefinedIfPoison(I) && I->getType()->isIntegerTy(1)) + if (programUndefinedIfFullPoison(I) && I->getType()->isIntegerTy(1)) // Note: once we have an agreement that poison is a value-wise concept, // we can remove the isIntegerTy(1) constraint. return true; @@ -4846,7 +4846,7 @@ bool llvm::isGuaranteedToExecuteForEveryIteration(const Instruction *I, llvm_unreachable("Instruction not contained in its own parent basic block."); } -bool llvm::propagatesPoison(const Instruction *I) { +bool llvm::propagatesFullPoison(const Instruction *I) { // TODO: This should include all instructions apart from phis, selects and // call-like instructions. switch (I->getOpcode()) { @@ -4880,7 +4880,7 @@ bool llvm::propagatesPoison(const Instruction *I) { } } -const Value *llvm::getGuaranteedNonPoisonOp(const Instruction *I) { +const Value *llvm::getGuaranteedNonFullPoisonOp(const Instruction *I) { switch (I->getOpcode()) { case Instruction::Store: return cast(I)->getPointerOperand(); @@ -4918,12 +4918,12 @@ const Value *llvm::getGuaranteedNonPoisonOp(const Instruction *I) { bool llvm::mustTriggerUB(const Instruction *I, const SmallSet& KnownPoison) { - auto *NotPoison = getGuaranteedNonPoisonOp(I); + auto *NotPoison = getGuaranteedNonFullPoisonOp(I); return (NotPoison && KnownPoison.count(NotPoison)); } -bool llvm::programUndefinedIfPoison(const Instruction *PoisonI) { +bool llvm::programUndefinedIfFullPoison(const Instruction *PoisonI) { // We currently only look for uses of poison values within the same basic // block, as that makes it easier to guarantee that the uses will be // executed given that PoisonI is executed. @@ -4956,7 +4956,7 @@ bool llvm::programUndefinedIfPoison(const Instruction *PoisonI) { if (YieldsPoison.count(&I)) { for (const User *User : I.users()) { const Instruction *UserI = cast(User); - if (propagatesPoison(UserI)) + if (propagatesFullPoison(UserI)) YieldsPoison.insert(User); } } diff --git a/llvm/lib/Transforms/Instrumentation/PoisonChecking.cpp b/llvm/lib/Transforms/Instrumentation/PoisonChecking.cpp index 6815900..bb90c7b 100644 --- a/llvm/lib/Transforms/Instrumentation/PoisonChecking.cpp +++ b/llvm/lib/Transforms/Instrumentation/PoisonChecking.cpp @@ -283,7 +283,7 @@ static bool rewrite(Function &F) { // Note: There are many more sources of documented UB, but this pass only // attempts to find UB triggered by propagation of poison. - if (Value *Op = const_cast(getGuaranteedNonPoisonOp(&I))) + if (Value *Op = const_cast(getGuaranteedNonFullPoisonOp(&I))) CreateAssertNot(B, getPoisonFor(ValToPoison, Op)); if (LocalCheck) @@ -294,7 +294,7 @@ static bool rewrite(Function &F) { } SmallVector Checks; - if (propagatesPoison(&I)) + if (propagatesFullPoison(&I)) for (Value *V : I.operands()) Checks.push_back(getPoisonFor(ValToPoison, V)); @@ -344,7 +344,7 @@ PreservedAnalyses PoisonCheckingPass::run(Function &F, - shufflevector - It would seem reasonable for an out of bounds mask element to produce poison, but the LangRef does not state. - and/or - It would seem reasonable for poison to propagate from both - arguments, but LangRef doesn't state and propagatesPoison doesn't + arguments, but LangRef doesn't state and propagatesFullPoison doesn't include these two. - all binary ops w/vector operands - The likely interpretation would be that any element overflowing should produce poison for the entire result, but diff --git a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp index 6b3db66..832385a 100644 --- a/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp +++ b/llvm/lib/Transforms/Scalar/IndVarSimplify.cpp @@ -1813,7 +1813,7 @@ static bool mustExecuteUBIfPoisonOnPathTo(Instruction *Root, // If we can't analyze propagation through this instruction, just skip it // and transitive users. Safe as false is a conservative result. - if (!propagatesPoison(I) && I != Root) + if (!propagatesFullPoison(I) && I != Root) continue; if (KnownPoison.insert(I).second) diff --git a/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp b/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp index 0d375bb..deff56b 100644 --- a/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp +++ b/llvm/lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp @@ -1214,13 +1214,13 @@ bool SeparateConstOffsetFromGEP::reuniteExts(Instruction *I) { // Add I to DominatingExprs if it's an add/sub that can't sign overflow. if (match(I, m_NSWAdd(m_Value(LHS), m_Value(RHS)))) { - if (programUndefinedIfPoison(I)) { + if (programUndefinedIfFullPoison(I)) { const SCEV *Key = SE->getAddExpr(SE->getUnknown(LHS), SE->getUnknown(RHS)); DominatingAdds[Key].push_back(I); } } else if (match(I, m_NSWSub(m_Value(LHS), m_Value(RHS)))) { - if (programUndefinedIfPoison(I)) { + if (programUndefinedIfFullPoison(I)) { const SCEV *Key = SE->getAddExpr(SE->getUnknown(LHS), SE->getUnknown(RHS)); DominatingSubs[Key].push_back(I); diff --git a/llvm/test/Analysis/ScalarEvolution/flags-from-poison.ll b/llvm/test/Analysis/ScalarEvolution/flags-from-poison.ll index caf372f..8733a85 100644 --- a/llvm/test/Analysis/ScalarEvolution/flags-from-poison.ll +++ b/llvm/test/Analysis/ScalarEvolution/flags-from-poison.ll @@ -205,7 +205,7 @@ exit: ret void } -; Demonstrate why we need a Visited set in llvm::programUndefinedIfPoison. +; Demonstrate why we need a Visited set in llvm::programUndefinedIfFullPoison. define void @test-add-not-header5(float* %input, i32 %offset) { ; CHECK-LABEL: @test-add-not-header5 entry: -- 2.7.4