[Bug preprocessor/53525] New: Performance regression due to enabling track-macro-expansion

[Bug preprocessor/53525] New: Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

             Bug #: 53525
           Summary: Performance regression due to enabling
                    track-macro-expansion
    Classification: Unclassified
           Product: gcc
           Version: 4.8.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: preprocessor
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jimis@gmx.net


About 3% regression on cc1 -O0 compilation time has been noticed between
gcc-4.8-20120429 and gcc-4.8-20120513. The culprit seems to be enabling by
default -ftrack-macro-expansion (TME). Example time, instruction count and
memory usage for compiling reload.c follow.

    time    M Instr    KB RAM
noTME    0.744s    2081    28608
TME    0.785s    2201    31672

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

jimis <jimis at gmx dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |abel at gcc dot gnu.org,
                   |                            |amonakov at gcc dot
                   |                            |gnu.org, dodji at gcc dot
                   |                            |gnu.org, tromey at gcc dot
                   |                            |gnu.org

--- Comment #1 from jimis <jimis at gmx dot net> 2012-05-30 04:04:00 UTC ---
I'll be posting some patches that try to improve CPU usage, some simple but
most more complex that don't work properly so I'd appreciate advice. CC'ing
mentors and tromey+Dodji who I think are the owners of the initial
track-macro-expansion patch.

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #2 from jimis <jimis at gmx dot net> 2012-05-30 04:44:54 UTC ---
According to valgrind major overhead is due to numerous calls of
line-map.c:linemap_line_start() that actually allocate new line_maps. This
happens because we are resetting the max_column_hint whenever we enter macro
context in enter_macro_context()->linemap_enter_macro(). 

The following one-liner seems to resolve this, as run time is improved by about
7ms or 45 M Instr.


=== modified file 'libcpp/line-map.c'
--- libcpp/line-map.c   2012-04-30 11:42:12 +0000
+++ libcpp/line-map.c   2012-05-27 06:52:08 +0000
@@ -331,7 +331,6 @@ linemap_enter_macro (struct line_maps *s
          num_tokens * sizeof (source_location));

   LINEMAPS_MACRO_CACHE (set) = LINEMAPS_MACRO_USED (set) - 1;
-  set->max_column_hint = 0;

   return map;
 }

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #3 from jimis <jimis at gmx dot net> 2012-05-30 04:52:20 UTC ---
Another simple one that my eye caught but does not effect performance.
Generally I don't get many things in macro.c, but am I correct to assume that
the following stands?


=== modified file 'libcpp/macro.c'
--- libcpp/macro.c      2012-05-02 16:55:19 +0000
+++ libcpp/macro.c      2012-05-27 06:55:37 +0000
@@ -1897,10 +1897,9 @@ tokens_buff_new (cpp_reader *pfile, size
                 source_location **virt_locs)
 {
   size_t tokens_size = len * sizeof (cpp_token *);
-  size_t locs_size = len * sizeof (source_location);

   if (virt_locs != NULL)
-    *virt_locs = XNEWVEC (source_location, locs_size);
+    *virt_locs = XNEWVEC (source_location, len);
   return _cpp_get_buff (pfile, tokens_size);
 }

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #4 from jimis <jimis at gmx dot net> 2012-05-30 05:23:54 UTC ---
Another hotspot higlighted by valgrind is the multitude of malloc/free() calls
in comparison to the past. I'm attaching a slightly more intrusive patch that
uses obstacks to allocate some of the virt_locs. It manages to save about 15 M
instr or 2-3 ms, almost unnoticeable difference.

Even though it seems to work fine it's not ready to commit, just an RFC for
moving to obstacks. I'm also attaching some changes in libiberty.h that
introduce some more XOB* macros.

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #5 from jimis <jimis at gmx dot net> 2012-05-30 05:28:31 UTC ---
Created attachment 27520
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27520
In macro.c:collect_args() use obstacks for virt_locs instead of malloc/realloc
vectors.

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #6 from jimis <jimis at gmx dot net> 2012-05-30 05:31:03 UTC ---
Created attachment 27521
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27521
Add some new obstack macros in libiberty.h.

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #7 from jimis <jimis at gmx dot net> 2012-05-30 06:01:23 UTC ---
Now time for the most intrusive and problematic patches. I tried moving all
virt_locs, expanded, expanded_virt_locs to obstacks for allocation. After many
failures to work with obstacks as they are I had to introduce two new
functions, obstack_{mark,release}(), that allow me to continue growing an older
object by pushing and popping a special marker.

Previous patches are included with the following, but I'm submitting it
separately because it's WIP. Even though it is now stable, there is a bad
memory leak that I don't know how to handle since I'm kind of lost in the
macro-expansion code. I'd appreciate suggestions, just search for LEAK or TODO
in my comments. :-p

Please ignore whitespace changes (that are highlighted by emacs so I fix them)
or stylistic changes in parts that I rewrote so many times that lost completely
their initial style.

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #8 from jimis <jimis at gmx dot net> 2012-05-30 06:06:16 UTC ---
Created attachment 27522
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27522
Introduce obstack_{mark,release} functions.

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #9 from jimis <jimis at gmx dot net> 2012-05-30 06:10:38 UTC ---
Created attachment 27523
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27523
Move all location/expansion vectors to obstacks. Warning MEMLEAKS!

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #10 from jimis <jimis at gmx dot net> 2012-05-30 06:23:56 UTC ---
Here is how this last patch (macro4) compares to trunk (TME) and to completely
disabling track-macro-expansion (noTME):

           time     M instr
noTME      0.744s   2081
TME        0.785s   2201
macro4     0.769s   2114

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-05-30
     Ever Confirmed|0                           |1

--- Comment #11 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-05-30 08:38:15 UTC ---
Confirmed.

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #12 from jimis <jimis at gmx dot net> 2012-05-30 15:55:19 UTC ---
I should probably explain where the problem is and why I've left a memory leak.
In tokens_buff_new() I can't use XOBNEWVEC() instead of XNEWVEC() because it is
not guarded from the obstack_mark/release() calls in enter_macro_context() and
it messes up previously allocated objects when going back in the recursion. 

The thing I don't get and so I can't figure out a solution, is where the
virt_locs vector allocated inside tokens_buff_new() is freed. For example when
it is called from the path
cpp_get_token_1->paste_all_tokens->tokens_buff_new(), then when is the virt
locs vector actually freed?

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

jimis <jimis at gmx dot net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #27520|0                           |1
        is obsolete|                            |
  Attachment #27523|0                           |1
        is obsolete|                            |

--- Comment #13 from jimis <jimis at gmx dot net> 2012-06-04 04:49:13 UTC ---
Created attachment 27550
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=27550
Diff of all changes versus the 20120513 snapshot.

I think I'm closing to a final version of this fix. The attached patch contains
all of the above mentioned changes, plus it fixes the memory leaks. This
bootstraps fine and passes tests on x86 with no regression. 

Instruction count has been reduced from 2201M downto 2108M, which is only 30M
higher than having track-macro-expansion (TME) turned off. Unfortunately actual
runtime was improved less, so we gained back almost 50% of what we had lost by
enabling TME. In short running the same test as above, with this (macro5)
patch, gives:

2108 M instr
31692 KB RAM
0.760s

In a few words, I introduced four (!) new obstacks inside struct cpp_reader for
allocating the tokens from macro argument expansion, their virtual locations,
the virtual locations of arguments, and the virtual locations of other macros. 

I'm not sure whether this is an elegant solution but growing obstacks for
nested scopes is much more intuitive than reallocating arrays. I'd appreciate
any comments on my "TODO" notes in this patch, which mostly concern whether I
should move other allocations to obstacks as well.

Finally, my patch still contains the additions to obstacks (obstack_mark,
obstack_release). I'll try submitting them to glibc but since they are in the
obstack.h header file, they work even when using the libc obstacks so I guess
they can be committed in the gcc tree.

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[jimis at gmx dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #14 from jimis <jimis at gmx dot net> 2012-06-27 22:58:50 UTC ---
Ping? Can someone review my last patch? I think it's clean enough to be applied
(minus the TODO notes) and extra fixes can come separately later.

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #15 from Richard Guenther <rguenth at gcc dot gnu.org> 2012-06-28 09:52:43 UTC ---
(In reply to comment #14)
> Ping? Can someone review my last patch? I think it's clean enough to be applied
> (minus the TODO notes) and extra fixes can come separately later.

Did you post it to gcc-patches?  If so, ping there.

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[mathias at gaunard dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

Mathias Gaunard <mathias at gaunard dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mathias at gaunard dot com

--- Comment #16 from Mathias Gaunard <mathias at gaunard dot com> ---
I opened bug #56746 a while ago which is somewhat related to this.
ftrack-macro-expansion is causing 50% increased memory usage in my case (C++
code with heavy usage of macros and templates).

Please also consider memory usage and not just performance.

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |manu at gcc dot gnu.org

--- Comment #17 from Manuel López-Ibáñez <manu at gcc dot gnu.org> ---
(In reply to Mathias Gaunard from comment #16)
> I opened bug #56746 a while ago which is somewhat related to this.
> ftrack-macro-expansion is causing 50% increased memory usage in my case (C++
> code with heavy usage of macros and templates).
> 
> Please also consider memory usage and not just performance.

Perhaps you could try the patches posted here and report whether they
reduce/increase memory usage? I am afraid that at the moment there is none with
enough free time to work on this, so all the help we can get is appreciated.

If you want me, i can close the other bug as a duplicate of this to have all
info in a single bug, and update the description mentioning also memory usage.
>From gcc-bugs-return-424323-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Thu Jun 13 21:07:01 2013
Return-Path: <gcc-bugs-return-424323-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 25420 invoked by alias); 13 Jun 2013 21:07:01 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 25388 invoked by uid 48); 13 Jun 2013 21:06:58 -0000
From: "dhazeghi at yahoo dot com" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug target/57608] New: wrong code for expression at -O3 on x86_64-linux-gnu with -m32
Date: Thu, 13 Jun 2013 21:07:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: new
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: target
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: dhazeghi at yahoo dot com
X-Bugzilla-Status: UNCONFIRMED
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields: bug_id short_desc product version bug_status bug_severity priority component assigned_to reporter
Message-ID: <bug-57608-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2013-06/txt/msg00702.txt.bz2
Content-length: 1417

http://gcc.gnu.org/bugzilla/show_bug.cgi?idW608

            Bug ID: 57608
           Summary: wrong code for expression at -O3 on x86_64-linux-gnu
                    with -m32
           Product: gcc
           Version: 4.9.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: dhazeghi at yahoo dot com

Current gcc trunk and 4.8.x produces wrong code for the following testcase on
x86_64-linux when compiled at -O3 in 32-bit mode.  This is a regression from
4.7.x.

$ gcc-trunk -v
gcc version 4.9.0 20130613 (experimental) [trunk revision 200065] (GCC)
$ gcc-trunk -O2 -m32 wrong.c
$ ./a.out
2
$ gcc-4.7 -O3 -m32 wrong.c
$ ./a.out
2
$ gcc-trunk -O3 -m32 wrong.c
$ ./a.out
3
$
----------------------------------
int printf(const char *, ...);

int a, b, d, e, *f = &b;
long long l, *pl = &l;
char c, *pc = &c;

int bar(int r, long long s) {
  b++;
  a = 0;
  if (b) {
    b = r + s;
    b++;
    return c;
  }
  {
    int arr[] = {};
    return 0;
  }
}

void foo(int j) {
  int k;
  for (; j != 5; ++j) {
    *pc = *f;
    for (; e != 1; e++) {
      *pl = 0;
      {
        int i = 0;
        for (; i < 1; i++)
          a = 0;
      }
      k = bar(d, 1);
      d = bar(1, k);
    }
  }
}

void baz() { foo(0); }

int main() {
  foo(0);
  printf("%d\n", c);
  return 0;
}

[Bug preprocessor/53525] Performance regression due to enabling track-macro-expansion

[mathias at gaunard dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53525

--- Comment #18 from Mathias Gaunard <mathias at gaunard dot com> ---
I'm not competent enough to make my own builds of GCC with patches, and I
unfortunately do not have much time to contribute to this either.

If someone can give me binaries for debian x86-64 I can do some tests though.