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

Wed Aug 23 06:28:01 PDT 2017

Hi Chandler,

I’m investigating the issue with core.c file and could not reproduce the problem you’ve mentioned.
I’ve checked out  sources for stable-1.0 branch and build it with:
$ ./configure --cc=<path-to-my-clang>
$ make

Here are my compile line and clang version:

$ clang -I/tmpdir/qemu/slirp -I. -I/tmpdir/qemu -I/tmpdir/qemu/fpu -fPIE -DPIE -m64 -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -Wstrict-prototypes -Wredundant-decls -Wall -Wundef -Wwrite-strings -Wmissing-prototypes -fno-strict-aliasing  -fstack-protector-all -Wendif-labels -Wmissing-include-dirs -Wempty-body -Wnested-externs -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wold-style-definition -Wtype-limits -I/usr/include/libpng15 -I/tmpdir/qemu/libcacard -I/usr/include/nss3 -I/usr/include/nspr4   -DTARGET_PHYS_ADDR_BITS=64 -I.. -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include   -pthread -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include   -MMD -MP -MT ide/core.o -MF ide/core.d -O2 -g  -c -o ide/core.o /tmpdir/qemu/hw/ide/core.c
$ clang -v
clang version 6.0.0 (trunk 310583)
Target: x86_64-unknown-linux-gnu
Thread model: posix

I also checked qemu trunk version, it also has no problems. Compilation time is the same for r310582 and r310583.

Could you please provide additional information on how to reproduce the timeout issue?

Thanks,
Olga


From: Bozhenov, Nikolai
Sent: Monday, 14 August, 2017 11:43 AM
To: Chandler Carruth <chandlerc at gmail.com>
Cc: llvm-commits at lists.llvm.org; Chupina, Olga <olga.chupina at intel.com>
Subject: RE: [llvm] r310583 - [ValueTracking] Enabling ValueTracking patch by default (recommit). Part 2.

Ok, we will try to reproduce it with vanilla qemu build.
As for the performance, we can see very nice and stable improvements on some EEMBC tests (up to 25%), and no regressions caused by the transformation itself. That’s why we want to remove the flag.
Anyway, thank you for letting us know about the issue with compile time. We will investigate and fix it before re-committing the patch.

-Nikolai

From: Chandler Carruth [mailto:chandlerc at gmail.com]
Sent: Monday, August 14, 2017 11:17 AM
To: Bozhenov, Nikolai <nikolai.bozhenov at intel.com<mailto:nikolai.bozhenov at intel.com>>
Cc: llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>; Chupina, Olga <olga.chupina at intel.com<mailto:olga.chupina at intel.com>>
Subject: Re: [llvm] r310583 - [ValueTracking] Enabling ValueTracking patch by default (recommit). Part 2.

On Mon, Aug 14, 2017 at 2:01 AM Bozhenov, Nikolai <nikolai.bozhenov at intel.com<mailto: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<mailto:chandlerc at gmail.com>]
Sent: Monday, August 14, 2017 9:05 AM
To: Bozhenov, Nikolai <nikolai.bozhenov at intel.com<mailto:nikolai.bozhenov at intel.com>>; llvm-commits at lists.llvm.org<mailto: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<mailto: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<mailto: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/20170823/2a6a42f3/attachment.html>
