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

[Nuno Lopes]

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.

Nuno


-----Original Message----- 
From: Shuxin Yang
Sent: Sunday, April 14, 2013 4:42 AM
Subject: Re: [llvm] r179111 - Revert r176408 and r176407 to address PR15540.


Hi, Nuno:

  Let me go through  the events about your changes.

  1.  Your change originally triggered a bug about LTO bootstrap build.
       Bill revert your change and file a bug.

  2.  My manager asked me to figure out the root cause. '
      I fixed the problem and reiterate (in many places) that your 
problematic should be applied with my change.
      Unfortunately, you silently let your code creep in without even a 
single word addressing my concerns.

  3. Yi Jiang later on realized that your change increase GVN compile-time 
by 10%.
      You try to "reproduce" with debug-built compiler and claim you cannot 
reproduce the problem,
      and you claim you are too busy to dig into the problem. (Why the hack 
you have time to write
      the code in the 1st place?).

   4. Nadav asked you to revert your change,  you seems to be quite 
reluctant, and ask Nadav to
       "judiciously" revert "some code", "not too much".

       This is really ridiculous! I have never heard before that a original 
author of the problematic
       patch asked other people to "judiciously" revert part of patch.

   5. Also you promise you bring the comment to "object" back, you never 
did!

  It seems it is very difficult to work w you. go back to this mail.

> You should not remove  getUnderlyingObjectSize(), since it's required for 
> correctness of BasicAA.
Which bug did you fixed in the 176407?
And, why we cannot remove getUnderlyingObjectSize()?

In 176407, you just redo I made previously in order to enable your 2nd 
change.
Check your emails, 176407 was similar to my original proposal, after discuss 
with Arnold,
I ditch this proposal, and go for his proposal.

If you want to revert 176407, you first need to reproduce the defect Yi 
report, and convince us with
data you collect.



On 4/13/13 7:55 PM, Nadav Rotem wrote:


On Apr 13, 2013, at 6:22 PM, Nuno Lopes <nunoplopes at sapo.pt> wrote:


Hi Nadav,

I'm sorry to say but you reverted too much. In particular:
- You should not remove  getUnderlyingObjectSize(), since it's required for 
correctness of BasicAA.
- BasicAA changes shouldn't be reverted for the same reason.
- Tests 18 and 19 have nothing to do with this, and should stay.

I can perform these changes myself if you prefer.



Hi Nuno,

Thanks for looking at this. Are you going to re-apply r176407 ?

Thanks,
Nadav