[llvm] r185843 - Fix a SCEV update problem.

[Nick Lewycky]

Shuxin Yang wrote:
> Author: shuxin_yang
> Date: Mon Jul  8 12:33:13 2013
> New Revision: 185843
>
> URL: http://llvm.org/viewvc/llvm-project?rev=185843&view=rev
> Log:
>   Fix a SCEV update problem.
>
>   The symptom is seg-fault, and the root cause is that a SCEV contains a SCEVUnknown
> which has null-pointer to a llvm::Value.
>
>   This is how the problem take place:
>   ===================================
>    1). In the pristine input IR, there are two relevant instrutions Op1 and Op2,
>       Op1's corresponding SCEV (denoted as SCEV(op1)) is a SCEVUnknown, and
>       SCEV(Op2) contains SCEV(Op1).  None of these instructions are dead.
>
>       Op1 : V1 = ...
>       ...
>       Op2 : V2 = ... // directly or indirectly (data-flow) depends on Op1
>
>    2) Optimizer (LSR in my case) generates an instruction holding the equivalent
>       value of Op1, making Op1 dead.
>       Op1': V1' = ...
>       Op1: V1 = ... ; now dead)
>       Op2 : V2 = ... //Now deps on Op1', but the SCEV(Op2) still contains SCEV(Op1)
>
>    3) Op1 is deleted, and call-back function is called to reset
>       SCEV(Op1) to indicate it is invalid. However, SCEV(Op2) is not
>       invalidated as well.
>
>    4) Following pass get the cached, invalid SCEV(Op2), and try to manipulate it,
>       and cause segfault.
>
>   The fix:
>   ========
>   It seems there is no clean yet inexpensive fix. I write to dev-list
> soliciting good solution, unforunately no ack. So, I decide to fix this
> problem in a brute-force way:
>
>    When ScalarEvolution::getSCEV is called, check if the cached SCEV
> contains a invalid SCEVUnknow, if yes, remove the cached SCEV, and
> re-evaluate the SCEV from scratch.
>
>    I compile buch of big *.c and *.cpp, fortunately, I don't see any increase
> in compile time.
>
>   Misc:
> =====
>   The reduced test-case has 2357 lines of code+other-stuff, too big to commit.
>
>   rdar://14283433

I have a question about the symptom. SCEV(Op1) is invalidated, does that 
mean that SCEV(Op2) holds a dangling pointer? If so, don't we still have 
a bug if a new SCEV happens to be allocated at the old address?

>
> Modified:
>      llvm/trunk/include/llvm/Analysis/ScalarEvolution.h
>      llvm/trunk/lib/Analysis/ScalarEvolution.cpp
>
> Modified: llvm/trunk/include/llvm/Analysis/ScalarEvolution.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/ScalarEvolution.h?rev=185843&r1=185842&r2=185843&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/Analysis/ScalarEvolution.h (original)
> +++ llvm/trunk/include/llvm/Analysis/ScalarEvolution.h Mon Jul  8 12:33:13 2013
> @@ -545,6 +545,10 @@ namespace llvm {
>       /// forgetMemoizedResults - Drop memoized information computed for S.
>       void forgetMemoizedResults(const SCEV *S);
>
> +    /// return false iff given SCEV contains a SCEVUnknown with NULL value-

Capital 'R' please.

> +    /// pointer.
> +    bool checkValidity(const SCEV *S) const;
> +
>     public:
>       static char ID; // Pass identification, replacement for typeid
>       ScalarEvolution();
>
> Modified: llvm/trunk/lib/Analysis/ScalarEvolution.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=185843&r1=185842&r2=185843&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
> +++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Mon Jul  8 12:33:13 2013
> @@ -2715,13 +2715,51 @@ const SCEV *ScalarEvolution::getCouldNot
>     return&CouldNotCompute;
>   }
>
> +namespace {
> +  // Helper class working with SCEVTraversal to figure out if a SCEV contains
> +  // a SCEVUnknown with null value-pointer. FindInvalidSCEVUnknown::FindOne
> +  // is set iff if find such SCEVUnknown.
> +  //
> +  struct FindInvalidSCEVUnknown {
> +    bool FindOne;
> +    FindInvalidSCEVUnknown() { FindOne = false; }

Constructor initializer list?

> +    bool follow(const SCEV *S) {
> +      switch (S->getSCEVType()) {
> +      case scConstant:
> +        return false;
> +      case scUnknown:
> +        if(!cast<SCEVUnknown>(S)->getValue())

Missing space after 'if'.

> +          FindOne = true;
> +        return false;
> +      default:
> +        return true;
> +      }
> +    }
> +    bool isDone() const { return FindOne; }
> +  };
> +}
> +
> +bool ScalarEvolution::checkValidity(const SCEV *S) const {
> +  FindInvalidSCEVUnknown F;
> +  SCEVTraversal<FindInvalidSCEVUnknown>  ST(F);
> +  ST.visitAll(S);
> +
> +  return !F.FindOne;
> +}
> +
>   /// getSCEV - Return an existing SCEV if it exists, otherwise analyze the
>   /// expression and create a new one.
>   const SCEV *ScalarEvolution::getSCEV(Value *V) {
>     assert(isSCEVable(V->getType())&&  "Value is not SCEVable!");
>
> -  ValueExprMapType::const_iterator I = ValueExprMap.find_as(V);
> -  if (I != ValueExprMap.end()) return I->second;
> +  ValueExprMapType::iterator I = ValueExprMap.find_as(V);
> +  if (I != ValueExprMap.end()) {
> +    const SCEV *S = I->second;
> +    if(checkValidity(S))

Missing space after 'if'.

Nick

> +      return S;
> +    else
> +      ValueExprMap.erase(I);
> +  }
>     const SCEV *S = createSCEV(V);
>
>     // The process of creating a SCEV for V may have caused other SCEVs
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>