[llvm] r185843 - Fix a SCEV update problem.

[Shuxin Yang]

On 7/11/13 11:54 PM, Nick Lewycky wrote:
> 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?
No dangling pointer.
"SCEV(Op1) is invalidated" by resetting SCEV(Op1)'s field (i.e. 
SCEVUnknown::ValPtr) to null.
The SCEV(Op1) instance is still there.

> 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?

Not sure you question.  There is list involved here.
>
>> +    bool follow(const SCEV *S) {
>> +      switch (S->getSCEVType()) {
>> +      case scConstant:
>> +        return false;
>> +      case scUnknown:
>> +        if(!cast<SCEVUnknown>(S)->getValue())
>
> Missing space after 'if'.
Will fix stylistic problems soon.