[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