[llvm] r185843 - Fix a SCEV update problem.

[Nick Lewycky]

Shuxin Yang wrote:
> 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.

Got it. Thanks!

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

Sorry, I was confusing. I wrote it as a question to indicate that I 
consider my suggested change optional. The suggested change is to use 
constructor initializer lists, as in, "FindInvalidSCEVUnknown() : 
FindOne(false) {}". It's semantically identical in this context.

>>
>>> + 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.
>