[llvm] r179111 - Revert r176408 and r176408 to address PR15540.

[Shuxin Yang]

On 4/14/13 12:11 AM, Nuno Lopes wrote:
>> On 4/13/13 11:43 PM, Nuno Lopes wrote:
>>>> On 4/13/13 11:19 PM, Nuno Lopes wrote:
>>>>> Hi,
>>>>>
>>>>> In summary, I would like to reapply r176407, yes.
>>>>> The reason is that it is the correct fix for PR15540. This new 
>>>>> function, getUnderlyingObjectSize(), gives exactly the semantics 
>>>>> that BasicAA wants (which is to get the size of the object pointed 
>>>>> by a given pointer).
>>>> To be pedantic, BasicAA need min-size-of-all-potential-object 
>>>> pointed by the pointer in question.
>>>
>>> Sure, the precision of the analysis could be improved. The same goes 
>>> with constant folding expressions involving __builtin_object_size().
>> If you care the __builtin_object_size(), you certainly can improve it 
>> without impairing alias's cost.
>> Our team(s) are *VERY* picky at compile time,  please don't increase 
>> alias's cost without any good reason.
>>
>>> At this point, the only analysis we have is this one, that compute 
>>> exact object sizes instead of ranges.
>>>
>>>
>>>>> The function getObjectSize(), on the other hand, gives the size of 
>>>>> an object minus the offset of the given pointer. Using 
>>>>> getObjectSize() is therefore wrong (I wrote that code, but later I 
>>>>> realized my mistake).
>>>> Before the r176407, the compiler avoid the problem by *NOT* 
>>>> evaluating the size if the pointer does not points to a object.
>>>
>>> Analyzing phi nodes is orthogonal to getUnderlyingObjectSize(). 
>>> Please don't mix the two.
>> I recall the getUnderlyingObjectSize() call get.*Offset(), which walk 
>> along the U-D chain. No?
>
> Yes. But getObjectSize() does exactly the same thing.  The problem is 
> that it became noticeable after introducing the analysis of phi nodes, 
> and before it was just bailing out sooner.
>
>
>>> What you can say is that the analysis was improved to be more 
>>> aggressive by analyzing phi nodes, which triggered the *latent* bug 
>>> in the LTO build.
>>>
>>>
>>>>> I believe r176407 is the correct fix, and it should be reapplied.
>>>>>
>>>> Then come up an example to prove r176407-1 dose not work.
>>>
>>> I believe your patch does work, since you restrict the analysis to 
>>> pointers pointing to a single object.  But that's overly 
>>> restrictive, and inhibits further improvements.
>>
>> I'm wondering how many times we come across a case where a pointer 
>> points to difficult "objects" having the same size.
>> If you can show us data to justify this cost, that is certainly great.
>>
>> The missed the opportunity can picked up by other analyses, whose 
>> result are cached, and can be queried with low cost.
>>
>> However, calling get.*...Size() is different story, each time alias 
>> query is called, it has invoke this function. We should keep it cheap.
>
> Yes, agreed.  But I want to extend the analysis to provide ranges.  
> For AA it's probably irrelevant, but it is a huge help for 
> __builtin_object_size() folding.
> The lesson (for me) is that first I need to convert this whole thing 
> into a proper analysis so that the results can be cached across 
> passes.  And it may make sense to have different precisions for 
> different clients.
>
Yeah. Currently basicAA dose a lot of on-the-fly computation.

If you really want r176407 back,  I don't have strong objection. If you 
are seeking a better way, I think it would better hold it for a while:-).
Have a nice weekend!