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

[Nuno Lopes]

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

Ok, thanks!
Nadav: if you're still following this thread :),  can you recommit r176407 
for me, please?
I believe it's a slightly better solution and enables future improvements.

Thanks,
Nuno