[llvm] r310583 - [ValueTracking] Enabling ValueTracking patch by default (recommit). Part 2.

[Chandler Carruth via llvm-commits]

On Mon, Aug 14, 2017 at 2:01 AM Bozhenov, Nikolai <
nikolai.bozhenov at intel.com> wrote:

> Hi Chandler,
>
>
>
> > This is the *wrong* code review link, and only links to a test update.
>
>
>
> The review link is correct. However, Phabricator stopped displaying the
> patch properly after some point. Here is an “almost fixed” link:
>
>
> https://reviews.llvm.org/D34101?vs=106237&id=110556&whitespace=ignore-most#toc
>
>
>
> > https://github.com/qemu/qemu/blob/stable-1.0/hw/ide/core.c
>
>
>
> Could you suggest what options one should pass to compiler for core.c to
> reproduce the problem?
>
We build QEMU in about 3 or 4 configurations and they all timed out -- I
would try a vanilla configure/make build? If that doesn't work I can try to
get more specific repro steps.


>
>
> > Where are the numbers?
>
>
>
> I’m not sure what numbers you are asking us for. When the feature was
> first implemented, we saw some regressions on important benchmarks. That’s
> why it was disabled with this quite ugly flag. With newer versions of the
> compiler we cannot reproduce those regressions, and with the old compiler
> the regressions were found to be random fluctuations that cannot be
> addressed in the compiler.
>

I mean, the original patch was disabled by a flag because of some
regressions, and now the flag is being removed. I'd love to see any
discussion of performance implications at all? Did anything in the test
suite improve? Was it just the one previously discussed TSVC benhcmark
regression that you checked for?

I guess I'm confused whenever there is a specific mention of performance
numbers being better, but then those numbers aren't provided or even really
explained or discussed...


>
>
> -Nikolai
>
>
>
>
>
> *From:* Chandler Carruth [mailto:chandlerc at gmail.com]
> *Sent:* Monday, August 14, 2017 9:05 AM
> *To:* Bozhenov, Nikolai <nikolai.bozhenov at intel.com>;
> llvm-commits at lists.llvm.org
> *Subject:* Re: [llvm] r310583 - [ValueTracking] Enabling ValueTracking
> patch by default (recommit). Part 2.
>
>
>
> Revert landed in r310816. Let me know if you need any help getting the
> compile issues to reproduce!
>
>
>
> -Chandler
>
>
>
> On Sun, Aug 13, 2017 at 11:53 PM Chandler Carruth <chandlerc at gmail.com>
> wrote:
>
> On Thu, Aug 10, 2017 at 4:25 AM Nikolai Bozhenov via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
> Author: n.bozhenov
> Date: Thu Aug 10 04:24:57 2017
> New Revision: 310583
>
> URL: http://llvm.org/viewvc/llvm-project?rev=310583&view=rev
> Log:
> [ValueTracking] Enabling ValueTracking patch by default (recommit). Part 2.
>
> The original patch was an improvement to IR ValueTracking on non-negative
> integers. It has been checked in to trunk (D18777, r284022). But was
> disabled by
> default due to performance regressions.
> Perf impact has improved. The patch would be enabled by default.
>
>
>
> Where are the numbers? I couldn't track anything down because...
>
>
>
> Differential Revision: https://reviews.llvm.org/D34101
>
>
>
> This is the *wrong* code review link, and only links to a test update.
>
>
>
> Also:
>
>
> ==============================================================================
> --- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)
> +++ llvm/trunk/lib/Analysis/ValueTracking.cpp Thu Aug 10 04:24:57 2017
> @@ -54,12 +54,6 @@ const unsigned MaxDepth = 6;
>  static cl::opt<unsigned> DomConditionsMaxUses("dom-conditions-max-uses",
>                                                cl::Hidden, cl::init(20));
>
> -// This optimization is known to cause performance regressions is some
> cases,
> -// keep it under a temporary flag for now.
> -static cl::opt<bool>
> -DontImproveNonNegativePhiBits("dont-improve-non-negative-phi-bits",
> -                              cl::Hidden, cl::init(true));
> -
>
>
>
> Given that this has already been disabled before (and had to be reverted a
> few times) it seems very prudent to leave the flag in place and just change
> the default value at first.
>
>
>
> Indeed, we've found it still causes serious compile time issues with code
> such as:
>
> https://github.com/qemu/qemu/blob/stable-1.0/hw/ide/core.c
>
>
>
>
> I'm going to revert this for now. If you can't reproduce, we can get a
> test case together, but building qemu is likely sufficient.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170814/48bdd946/attachment.html>