[llvm] r215343 - In LVI(Lazy Value Info), originally value on a BB can only be caculated once,

[Hans Wennborg]

(Forgot to +thakis in my previous email.)

On Thu, Nov 20, 2014 at 11:10 AM, Hans Wennborg <hans at chromium.org> wrote:
> Sorry for coming late to this thread.
>
> On Fri, Oct 10, 2014 at 6:31 AM, Hal Finkel <hfinkel at anl.gov> wrote:
>> ----- Original Message -----
>>> From: "Jiangning Liu" <liujiangning1 at gmail.com>
>>> To: "Hal Finkel" <hfinkel at anl.gov>
>>> Cc: "llvm-commits at cs.uiuc.edu for LLVM" <llvm-commits at cs.uiuc.edu>, "Nick Lewycky" <nicholas at mxc.ca>, "Daniel Berlin"
>>> <dberlin at dberlin.org>
>>> Sent: Friday, October 10, 2014 3:29:25 AM
>>> Subject: Re: [llvm] r215343 - In LVI(Lazy Value Info), originally value on a BB can only be caculated once,
>>> If you have any other empirical evidence, could you please share it?
>>
>> No, but I think it became clear that there were serious algorithmic concerns, justified by the fact that we needed a cut-off in the first place. The fact remains that, initially, we did not think that this commit would change the asymptotic complexity of the algorithm. I remember thinking that it would only cause additional processing to happen for blocks we had already decided to visit, not cause any additional visits, and so I expected, at most, a constant-factor increase. This turned out to be wrong, and I think that Danny and Nick have done a good job explaining the error in my reasoning.
>>
>> So at this point we're having a design discussion about a proper solution, and that's great. Nevertheless, the reasoning behind approving the current implementation was that it did not change the complexity, which was wrong, and based on Danny's and Nick's notes, I'm also not convinced that the cut-offs we introduced were proper (meaning minimal). As a matter of general policy, we don't keep commits in tree for which there are serious algorithmic concerns, and so reverting was appropriate.
>>
>> Please do continue to work on this!
>
> Is there any update on this patch? When it got reverted, we saw a 110
> k binary size increase in Chromium.
>
>  - Hans