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

Nuno Lopes nunoplopes at sapo.pt
Sat Apr 13 23:51:57 PDT 2013


> On 4/13/13 11:33 PM, Nuno Lopes wrote:
>> Shuxin,
>>
>> My vision of the events is slightly different than yours.
>> It's true that I introduced the bug in the first place. I then 
>> acknowledged your work in tracking the underlying root cause of it, which 
>> was a misinterpretation on my end of the required semantics of Basic AA.
>> You committed a fix, but in my understanding that was a hack, and not a 
>> real solution.  I then introduced getUnderlyingObjectSize(), which has 
>> the semantics that BasicAA requires. It's a superior solution than your 
>> patch that tries to work around the mismatch in the semantics of 
>> getObjectSize() and what Basic AA requires.
>> And btw, r176407 is not at fault in the compile time regression.
>>
>> So far so good. The problem is that you kept insisting my solution was 
>> wrong without giving any technical explanation. You became extremely 
>> aggressive, and therefore I stop paying attention.
>> If you give me a technical reason why r176407 is wrong, then I'll surely 
>> back off and apologize.  Otherwise I'll keep insisting that r176407 is 
>> the real fix.
>
> I think your wording is bit interesting. When did I say r176407 is wrong? 
> Please paste my mail to the list.
>
> I did have some little concern about the 176407 at the beginning you 
> commit your change. As it, combined with your next revision,
> it walk along the SSA, each time alias is queried.  Yi's report prove my 
> concerns.
>
> It seems you are insist on the 176407. You state it is "required" in your 
> previous mail.
> Now that you are so sure about your change, why not show us some data 
> proving how much
> speedup you achieve at what cost?

Ok, let me link to the patch to make sure we are all on the same page:
http://llvm.org/viewvc/llvm-project?view=revision&revision=176407

This patch is not the one that analyzes phi nodes.  That one (r176408) I 
agreed to be reverted. That's the patch that introduced the performance 
penalty.

r176407 only introduces the getUnderlyingObjectSize() function that does 
*exactly* the same thing as getObjectSize(), but does not subtract the 
pointer offset.

Nuno 




More information about the llvm-commits mailing list