Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
Hi,
On Tue, 3 Sep 2013, David Malcolm wrote:
I can't really say I find this shorter, easier to read, more
expressive or even safer than what was there before. And the
repetition for adding the helpers for const and non-const types
all the time doesn't make it better.
Part of this is the verbose struct names. I mentioned getting rid of
the _statement part of the typenames, I think I'll do that.
Yep, IMO makes sense.
The other part is that the accessor functions become redundant, and that
you'd be able to do the cast once, and then use all of the various
fields of a gimple_whatever, bypassing the getters/setters.
Well, you can do that today with unions too, it's just not prevalent
style; but you could do:
if (gimple_has_mem_ops (g))
{
struct gimple_statement_with_memory_ops_base *gm = g-gsmembase;
gm-vuse = ...;
}
Obviously the naming of the struct here also is a bit excessive. Using
accessors has one large advantage over accessing the fields directly, you
can change the semantics of them. One reason why the above style isn't
used. But if that is true one of your reasons doing the change (downcast
once, access fields directly, obsoleting the accessors) becomes moot,
because we don't _want_ to access the fields directly. That is, until you
add member functions doing the accesses, which has its own problems (of
stylistic nature, because then we'd have a very weird and clumsy mix in
GCC sources of some data structures having member function accessors and
others using the traditional C style).
Hmm. After some nights sleeping over this, I'm oscillating again between
not liking the change and being indifferent; as in, I do see some of the
advantages you mentioned but I don't regard them outweighing the
disadvantages.
Ciao,
Michael.
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Mon, 2013-09-02 at 14:35 +0200, Martin Jambor wrote: Hi, On Fri, Aug 30, 2013 at 03:21:22PM -0400, David Malcolm wrote: Apart from the GTY aspect, how do people feel about the patch series? FWIW I have vague thoughts about doing something similar for tree - doing so *might* give an easier route to the type vs expression separation that Andrew spoke about at the Cauldron rearchitecture BoF. (I started looking at doing a similar C++-ification of rtx, but... gah) I like it but before you start looking at the biger things, could you perhpas proceed with the symtab? It has much fewer classes, will probably affect private development of fewer people, the accessor macros/functions of symtab are less developed so it will immediately really make code nicer, Honza has approved it and I'm really looking forward to it. Also, perhaps it will show us at much saller scale potential problems with the general scheme. Sorry about the delay. I wasn't aware that it had been approved; there seemed to be a lot of caveats and objections in that thread. On re-reading, http://gcc.gnu.org/ml/gcc-patches/2013-08/msg01147.html could be seen as approval, but I guess I was making a conservative reading of that post. I hope to refresh the patches and reboostrap/repost them at some point this week. I think the patch was generally in right direction. I would welcome if you got rid of symtab_node_base (and made symtab_node the basetype) and possibly also did the suggested grand renaming. But the second can be probably done incrementally. I'm only writing this because the development there seems a bit stalled and it it a shame. Of course, you ay want to simplify the manual markings first. I'd perfectly understand that. I've been poking at gengtype (and running benchmarks; see other post), which would affect the symtab patch, though it's something of a quagmire... Making gengtype to generate ggc_mark for each type would make hand writting easier - you can use C++ overloading instead of trying to guess the funny names gengtype uses right now. But that is independent of this change. I am slowly getting used to the world of hand written gengtype markings. Honza
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Sep 4, 2013, at 7:09 AM, Jan Hubicka hubi...@ucw.cz wrote: Making gengtype to generate ggc_mark for each type would make hand writting easier - you can use C++ overloading instead of trying to guess the funny names gengtype uses right now. But that is independent of this change. I am slowly getting used to the world of hand written gengtype markings. I'd rather plugin generated code that applies general rules to the transitive closure of all types needed. The markings would be for exceptions to those rules, which should be very, very rare. The load for someone writing and maintaining code is then reduced and the fear of GTY doesn't play a role in activities such as refactoring, as there would be nothing to do. I know it, and I have a healthy fear of it. :-)
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Mon, 2013-09-02 at 13:44 +0200, Michael Matz wrote: Hi, On Fri, 30 Aug 2013, David Malcolm wrote: Here's the result of a pair of builds of r202029 without and with the patches, configured with --enable-checking=release, running make, then stripping debuginfo [1] So the overall sizes of such binaries are essentially unchanged. Yep, cool. Any suggestions on what to compile to compare performance? By 60 seconds, do you mean 60s for one TU, or do you mean a large build e.g. the linux kernel? For one TU, so as to not measure any significant execve, as or IO overhead. Some people compile e.g preprocessed variants of some GCC source file, myself I'm measuring such speeds since some years with unchanged versions of the attached (big-code.c, several large functions with arithmetics and one-deep loops) which can be customized by hand to produce different largeness and kdecore.cc [1] (real world C++ library code from 2009). You can customize big-code.c to match some compile time goal by commenting out some FUNC invocation for some types, or by changing the body size of the functions by fiddling in the FUNC macro itself to invoke more of the L2's or even L3's, but that's really slow. Thanks. Everyone seems to have their own benchmarking test files - I'd like to gather those that I can into a common repository. What license is big-code.c under? Presumably kdecore.cc is from a compile of KDE and thus covered by the license of that (plus of all the headers that were included, I guess). I scripted compilation of each of big-code.c and kdecore.cc at -O3, using the cc1plus from each of the builds described above, 10 times (i.e. the build of r202029 without (control) and with the patches (experiment), both configured with --enable-checking=release, running make, then stripping debuginfo). I wrote a script to parse the TOTAL timevar data from the logs, extracting the total timings for user, sys, and wallclock, and the ggc memory total. I used some code from Python's benchmarking suite to determine whether the observed difference is significant, and to draw comparative graphs of the data (actually, to generate URLs to Google's Chart API). There were no significant differences for these data between the cc1plus with the patch and the cc1plus without. The script uses Student's two-tailed T test on the benchmark results at the 95% confidence level to determine significance, but it also has a cutoff in which when the averages are within 1% of each other they are treated as insignificant. If I remove the cutoff, then the kdecore.cc wallclock time is reported as *faster* with the patch (t=2.41) - but by a tiny amount. FWIW this benchmarking code can be seen at: https://github.com/davidmalcolm/gcc-build/commit/4581f080be2ed92179bfc1bc12e0ba9e923adaae Data and URLs to graphs follow: kdecore.cc -O3: usr control: [63.01, 63.11, 62.9, 63.14, 63.22, 63.0, 63.21, 63.03, 63.07, 63.09] experiment: [62.94, 63.05, 62.99, 63.03, 63.25, 62.91, 63.14, 62.96, 63.0, 63.04] Min: 62.90 - 62.91: 1.00x slower Avg: 63.078000 - 63.031000: 1.00x faster Not significant Stddev: 0.09852 - 0.10049: 1.0200x larger Timeline: http://goo.gl/8P9mFr kdecore.cc -O3: sys control: [5.85, 5.74, 5.91, 5.79, 5.79, 5.85, 5.81, 5.9, 5.83, 5.8] experiment: [5.8, 5.81, 5.88, 5.88, 5.77, 5.87, 5.69, 5.8, 5.75, 5.84] Min: 5.74 - 5.69: 1.01x faster Avg: 5.827000 - 5.809000: 1.00x faster Not significant Stddev: 0.05229 - 0.06154: 1.1769x larger Timeline: http://goo.gl/xfWoUL kdecore.cc -O3: wall control: [69.01, 69.01, 68.94, 69.03, 69.13, 68.99, 69.18, 69.07, 69.01, 69.01] experiment: [68.88, 68.97, 68.99, 69.03, 69.12, 68.88, 68.96, 68.86, 68.85, 68.99] Min: 68.94 - 68.85: 1.00x faster Avg: 69.038000 - 68.953000: 1.00x faster Not significant Stddev: 0.07052 - 0.08616: 1.2217x larger Timeline: http://goo.gl/Pf9BL8 kdecore.cc -O3: ggc control: [988589.0, 988591.0, 988585.0, 988582.0, 988584.0, 988589.0, 988585.0, 988582.0, 988584.0, 988586.0] experiment: [988593.0, 988585.0, 988589.0, 988585.0, 988591.0, 988588.0, 988591.0, 988585.0, 988589.0, 988590.0] Mem max: 988591.000 - 988593.000: 1.x larger Usage over time: http://goo.gl/4pxB8Y big-code.c -O3: usr control: [60.06, 60.31, 60.33, 60.29, 60.28, 60.26, 60.41, 60.28, 60.29, 60.29] experiment: [59.9, 60.14, 60.27, 60.33, 60.22, 60.33, 60.31, 60.33, 60.16, 60.26] Min: 60.06 - 59.90: 1.00x faster Avg: 60.28 - 60.225000: 1.00x faster Not significant Stddev: 0.08781 - 0.13360: 1.5215x larger Timeline: http://goo.gl/gkFbgi big-code.c -O3: sys control: [1.32, 1.37, 1.34, 1.41, 1.35, 1.34, 1.29, 1.36, 1.33, 1.35] experiment: [1.38, 1.35, 1.32, 1.32, 1.33, 1.33, 1.34, 1.32, 1.41, 1.35] Min: 1.29 - 1.32: 1.02x slower Avg: 1.346000 - 1.345000: 1.00x faster Not significant Stddev: 0.03169 - 0.02953: 1.0731x smaller Timeline: http://goo.gl/DPijei
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Mon, 2013-09-02 at 13:44 +0200, Michael Matz wrote: Hi, On Fri, 30 Aug 2013, David Malcolm wrote: [...] And the manual GTY markers are so not maintainable in the long run, gengtype or something else really needs to be taught to create them automatically. Apart from the GTY aspect, how do people feel about the patch series? Generally I like it, the parts I don't like are common to all the C++ification patches [2]. The manual GTY parts give me the creeps, but I think I expressed that already :) Ciao, Michael. [1] http://frakked.de/~matz/kdecore.cc.gz [2] In this series it's the is_a/has_a/dyn_cast uglification, - gcc_gimple_checking_assert (gimple_has_mem_ops (g)); - g-gsmembase.vuse = vuse; + gimple_statement_with_memory_ops *mem_ops_stmt = +as_a gimple_statement_with_memory_ops (g); + mem_ops_stmt-vuse = vuse; I can't really say I find this shorter, easier to read, more expressive or even safer than what was there before. And the repetition for adding the helpers for const and non-const types all the time doesn't make it better. Part of this is the verbose struct names. I mentioned getting rid of the _statement part of the typenames, I think I'll do that. The other part is that the accessor functions become redundant, and that you'd be able to do the cast once, and then use all of the various fields of a gimple_whatever, bypassing the getters/setters. (Btw: something for your helper script: it's customary to put the '=' to the next line; I know there is precedent for your variant, but '=' on next line is prevalent) Thanks; I'll fix that.
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Mon, 2013-09-02 at 14:35 +0200, Martin Jambor wrote: Hi, On Fri, Aug 30, 2013 at 03:21:22PM -0400, David Malcolm wrote: Apart from the GTY aspect, how do people feel about the patch series? FWIW I have vague thoughts about doing something similar for tree - doing so *might* give an easier route to the type vs expression separation that Andrew spoke about at the Cauldron rearchitecture BoF. (I started looking at doing a similar C++-ification of rtx, but... gah) I like it but before you start looking at the biger things, could you perhpas proceed with the symtab? It has much fewer classes, will probably affect private development of fewer people, the accessor macros/functions of symtab are less developed so it will immediately really make code nicer, Honza has approved it and I'm really looking forward to it. Also, perhaps it will show us at much saller scale potential problems with the general scheme. Sorry about the delay. I wasn't aware that it had been approved; there seemed to be a lot of caveats and objections in that thread. On re-reading, http://gcc.gnu.org/ml/gcc-patches/2013-08/msg01147.html could be seen as approval, but I guess I was making a conservative reading of that post. I hope to refresh the patches and reboostrap/repost them at some point this week. I'm only writing this because the development there seems a bit stalled and it it a shame. Of course, you ay want to simplify the manual markings first. I'd perfectly understand that. I've been poking at gengtype (and running benchmarks; see other post), which would affect the symtab patch, though it's something of a quagmire...
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
Hi,
On Fri, 30 Aug 2013, David Malcolm wrote:
Here's the result of a pair of builds of r202029 without and with the
patches, configured with --enable-checking=release, running make, then
stripping debuginfo [1]
So the overall sizes of such binaries are essentially unchanged.
Yep, cool.
Any suggestions on what to compile to compare performance? By 60
seconds, do you mean 60s for one TU, or do you mean a large build e.g.
the linux kernel?
For one TU, so as to not measure any significant execve, as or IO
overhead. Some people compile e.g preprocessed variants of some GCC
source file, myself I'm measuring such speeds since some years with
unchanged versions of the attached (big-code.c, several large functions
with arithmetics and one-deep loops) which can be customized
by hand to produce different largeness and kdecore.cc [1] (real world C++
library code from 2009).
You can customize big-code.c to match some compile time goal by
commenting out some FUNC invocation for some types, or by changing the
body size of the functions by fiddling in the FUNC macro itself to invoke
more of the L2's or even L3's, but that's really slow.
And the manual GTY markers are so not maintainable in the long run,
gengtype or something else really needs to be taught to create them
automatically.
Apart from the GTY aspect, how do people feel about the patch series?
Generally I like it, the parts I don't like are common to all the
C++ification patches [2]. The manual GTY parts give me the creeps, but I
think I expressed that already :)
Ciao,
Michael.
[1] http://frakked.de/~matz/kdecore.cc.gz
[2] In this series it's the is_a/has_a/dyn_cast uglification,
- gcc_gimple_checking_assert (gimple_has_mem_ops (g));
- g-gsmembase.vuse = vuse;
+ gimple_statement_with_memory_ops *mem_ops_stmt =
+as_a gimple_statement_with_memory_ops (g);
+ mem_ops_stmt-vuse = vuse;
I can't really say I find this shorter, easier to read, more
expressive or even safer than what was there before. And the
repetition for adding the helpers for const and non-const types
all the time doesn't make it better.
(Btw: something for your helper script: it's customary to put the
'=' to the next line; I know there is precedent for your variant,
but '=' on next line is prevalent)extern int get_i(void);
typedef signed char schar;
typedef unsigned char uchar;
typedef unsigned short ushort;
typedef unsigned int uint;
typedef unsigned long ulong;
typedef long long llong;
typedef unsigned long long ullong;
typedef long double ldouble;
#define BODY(T) \
for (i = get_i(); i; i--) { \
accum += do_1_ ## T(accum, x, y, z); \
switch (get_i()) { \
case 1: case 3: case 34: case 123: \
if (z*x y) \
x = do_2_ ## T (x * y + z); \
else \
y = do_3_ ## T (x / y - z); \
break; \
case 6: case 43: case -2: \
z = do_4_ ## T ((int)z); \
break; \
}\
for (j = 1; j get_i (); j++) { \
a[i+j] = accum * b[i+j*get_i()] / c[j]; \
b[j] = b[j] - a[j-1]*x; \
x += b[j-1] + b[j+1]; \
y *= c[j*i] += a[0] + b[0] * c[(int)x]; \
} \
for (j = 0; j 8192; j++) { \
a[j] = a[j] + b[j] * c[j]; \
} \
}
#define L1(T) BODY(T) BODY(T) BODY(T) BODY(T) BODY(T) BODY(T) BODY(T) BODY(T)
#define L2(T) L1(T) L1(T) L1(T) L1(T) L1(T) L1(T) L1(T) L1(T)
#define L3(T) L2(T) L2(T) L2(T) L2(T) L2(T) L2(T) L2(T) L2(T)
#define FUNC(T) \
extern T do_1_ ## T(T, T, T, int); \
extern T do_2_ ## T(T); \
extern T do_3_ ## T(T); \
extern T do_4_ ## T(T); \
T func_ ## T (T x, T y, T z, T *a, T *b, T *c) \
{ \
T accum = 0; \
int i, j; \
L2(T) \
/*L2(T)*/ \
return accum; \
}
FUNC(schar)
FUNC(uchar)
FUNC(short)
FUNC(ushort)
FUNC(int)
FUNC(uint)
FUNC(long)
FUNC(ulong)
FUNC(llong)
FUNC(ullong)
FUNC(float)
FUNC(double)
FUNC(ldouble)
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
Hi, On Fri, Aug 30, 2013 at 03:21:22PM -0400, David Malcolm wrote: Apart from the GTY aspect, how do people feel about the patch series? FWIW I have vague thoughts about doing something similar for tree - doing so *might* give an easier route to the type vs expression separation that Andrew spoke about at the Cauldron rearchitecture BoF. (I started looking at doing a similar C++-ification of rtx, but... gah) I like it but before you start looking at the biger things, could you perhpas proceed with the symtab? It has much fewer classes, will probably affect private development of fewer people, the accessor macros/functions of symtab are less developed so it will immediately really make code nicer, Honza has approved it and I'm really looking forward to it. Also, perhaps it will show us at much saller scale potential problems with the general scheme. I'm only writing this because the development there seems a bit stalled and it it a shame. Of course, you ay want to simplify the manual markings first. I'd perfectly understand that. Thanks, Martin
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
Jakub Jelinek ja...@redhat.com wrote:
On Fri, Aug 30, 2013 at 11:28:34AM -0400, Diego Novillo wrote:
Well, it was a wrong decision then. For some smaller types writing
manual
marker might be a sensible thing, or for some extra complicated
constructs. But here we're talking about the most simple struct
hierarchy
imaginable. Having to write manual markers for that one is absurd
IMO.
I want to discourage extending gengtype more than necessary. Long
term, I would like to see memory pools replacing GC. However, that is
likely a long road so we should find an interim solution.
I have doubts that, still somewhat remember the obstack era and memory
pools
would again bring all the issues back, but let's put that aside.
I vaguely remember thinking about what would be needed to have
gengtype deal with inheritance. It needed some pretty ugly
annotations. This made gengtype even more magic. That's very bad for
maintenance.
Teaching the gengtype parser about
{struct,class} name : public {struct,class} someothername { ... }
as opposed to current
{struct,class} name { ... }
shouldn't be that hard. And, if the complaint is that we'd need to
write
whole C++ parser for it, then the response could be that we already
have
one C++ parser (and, even have plugin support and/or emit dwarf etc.).
Note that explicit markings was also about simply writing
GTY((derivesfrom(xxx))) instead of extending the parser. The result could be
as simple as gengtype emitting
ggc_mx ((xxx) *this)
In the marker routine.
Richard.
So, gengtype could very well use a g++ plugin to emit the stuff it
needs,
or parse DWARF, etc. And, we even could not require everybody actually
emitting those themselves, we could check some text form of that
(gengtype.state?) into the tree, so only people actually changing the
compiler would need to run the plugin.
One thing I liked is a suggestion that went something along the lines
of creating some base templates that could be used to facilitate
writing the manual markers.
Even if you have some stuff that helps you writing those, still it will
be a
big source of bugs (very hard to debug) and a maintainance nightmare.
Jakub
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
Diego Novillo dnovi...@google.com wrote:
On Fri, Aug 30, 2013 at 11:37 AM, Jakub Jelinek ja...@redhat.com
wrote:
Teaching the gengtype parser about
{struct,class} name : public {struct,class} someothername { ... }
as opposed to current
{struct,class} name { ... }
shouldn't be that hard. And, if the complaint is that we'd need to
write
whole C++ parser for it, then the response could be that we already
have
one C++ parser (and, even have plugin support and/or emit dwarf
etc.).
It isn't. It's annoying and a duplication of effort.
So, gengtype could very well use a g++ plugin to emit the stuff it
needs,
or parse DWARF, etc. And, we even could not require everybody
actually
emitting those themselves, we could check some text form of that
(gengtype.state?) into the tree, so only people actually changing the
compiler would need to run the plugin.
Yes. Lawrence and I thought about moving gengtype inside g++. That
seemed like a promising approach.
What do you do during stage1? Have a collector that never collects?
Richard.
Even if you have some stuff that helps you writing those, still it
will be a
big source of bugs (very hard to debug) and a maintainance nightmare.
Debugging gengtype is much harder. It is magic code that is not
visible.
Diego.
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Sat, 2013-08-31 at 11:57 +0200, Richard Biener wrote:
Diego Novillo dnovi...@google.com wrote:
Yes. Lawrence and I thought about moving gengtype inside g++. That
seemed like a promising approach.
What do you do during stage1? Have a collector that never collects?
We could imagine that the successor of gengtype would be some GCC plugin
(which would generate C++ code for marking and GC and PCH purposes,
perhaps using ad-hoc attributes and pragmas)
Then for bootstrapping purposes, we could put the generated C++ code in
the source repository (like we already do for configure, or
fixincludes/fixincl.x etc...). Hence stage1 would be buildable with the
generated C++ code in the repository.
A more difficult issue is that the set of GTY-ed types is target
specific and depends upon the .../configure argument at build time.
Perhaps we could consider processing all of it (i.e. every GTY-ed class
declaration), and have our gengtype successor plugin emit appropriate
#if in the generated C++ code.
Of course having gengtype replaced by a plugin requires such a plugin to
be developed and GCC maintainers to have access to some gcc...
Cheers
--
Basile STARYNKEVITCH http://starynkevitch.net/Basile/
email: basileatstarynkevitchdotnet mobile: +33 6 8501 2359
8, rue de la Faiencerie, 92340 Bourg La Reine, France
*** opinions {are only mine, sont seulement les miennes} ***
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Sat, Aug 31, 2013 at 5:57 AM, Richard Biener richard.guent...@gmail.com wrote: What do you do during stage1? Have a collector that never collects? Yes. That was the pebble in the shoe. The cc1plus built for the purposes of gengtype does not need to look at a lot of code, so turning off collection may not be a big problem. We also considered using a retargetable parser like Clang, or even plugins. All these approaches meant keeping GC. Neither of us is very fond of GC, so we did not explore these alternatives very deeply. Diego.
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
Hi,
On Thu, 29 Aug 2013, David Malcolm wrote:
Successfully bootstrapped and tested on x86_64-unknown-linux-gnu: all
testcases show the same results as an unpatched build (relative to
r202029).
I'd like to see some statistics for cc1{,plus} codesize and for compile
time of something reasonably large (needing say 60 seconds to
compile normally), before/after patch series.
And the manual GTY markers are so not maintainable in the long run,
gengtype or something else really needs to be taught to create them
automatically.
Ciao,
Michael.
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Fri, Aug 30, 2013 at 8:44 AM, Michael Matz m...@suse.de wrote: And the manual GTY markers are so not maintainable in the long run, gengtype or something else really needs to be taught to create them automatically. I thought the principle that was acquired was that gengtype shouldn't be improved to support more than what it does now…. -- Gaby
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Fri, Aug 30, 2013 at 08:58:43AM -0500, Gabriel Dos Reis wrote: On Fri, Aug 30, 2013 at 8:44 AM, Michael Matz m...@suse.de wrote: And the manual GTY markers are so not maintainable in the long run, gengtype or something else really needs to be taught to create them automatically. I thought the principle that was acquired was that gengtype shouldn't be improved to support more than what it does now…. If it means that we'll need to write and maintain tons of hand written code that could otherwise be generated and maintained by a tool for us, that principle doesn't look very good. Jakub
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Fri, Aug 30, 2013 at 9:02 AM, Jakub Jelinek ja...@redhat.com wrote: On Fri, Aug 30, 2013 at 08:58:43AM -0500, Gabriel Dos Reis wrote: On Fri, Aug 30, 2013 at 8:44 AM, Michael Matz m...@suse.de wrote: And the manual GTY markers are so not maintainable in the long run, gengtype or something else really needs to be taught to create them automatically. I thought the principle that was acquired was that gengtype shouldn't be improved to support more than what it does now…. If it means that we'll need to write and maintain tons of hand written code that could otherwise be generated and maintained by a tool for us, that principle doesn't look very good. Jakub Back in March 2013, I asked about gengtype support for inheritance. http://gcc.gnu.org/ml/gcc/2013-03/msg00273.html This http://gcc.gnu.org/ml/gcc/2013-03/msg00295.html was the definitive answer that appeared to be the consensus. -- Gaby
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Fri, 2013-08-30 at 09:12 -0500, Gabriel Dos Reis wrote:
On Fri, Aug 30, 2013 at 9:02 AM, Jakub Jelinek ja...@redhat.com wrote:
On Fri, Aug 30, 2013 at 08:58:43AM -0500, Gabriel Dos Reis wrote:
On Fri, Aug 30, 2013 at 8:44 AM, Michael Matz m...@suse.de wrote:
And the manual GTY markers are so not maintainable in the long run,
gengtype or something else really needs to be taught to create them
automatically.
I thought the principle that was acquired was that gengtype shouldn't
be improved to support more than what it does now….
If it means that we'll need to write and maintain tons of hand written code
that could otherwise be generated and maintained by a tool for us, that
principle doesn't look very good.
Jakub
Back in March 2013, I asked about gengtype support for inheritance.
http://gcc.gnu.org/ml/gcc/2013-03/msg00273.html
This
http://gcc.gnu.org/ml/gcc/2013-03/msg00295.html
was the definitive answer that appeared to be the consensus.
[adding Diego to the CC]
I get the impression from the responses so far to this and to the symtab
patches [1] that people would prefer that gengtype be somehow expanded
to cope with single-inheritance.
Handwaving over syntax (borrowing from the union-marking syntax),
perhaps something like this:
struct
GTY((chain_next (%h.next)),
desc (gimple_statement_structure (%h)))
gimple_statement_base {
/* as before */
};
struct GTY((subclass_tag (GSS_PHI))) gimple_statement_phi : public
gimple_statement_base {
}
and then have gengtype recognize the inheritance hierarchy below
gimple_statement_base, and do the right thing (again I'm handwaving).
Also, perhaps we could use a new GTY flag: never_in_pch or somesuch,
so that we can mark e.g. gimple and rtx as never appearing in pch, and
thus only the GC hooks need to exist; the PCH either don't need to be
generated, or could be stubs containing just gcc_unreachable? That
would be an independent feature from the subclass support, of course.
That said, for this particular patch series, for the hand-maintained
route, there would just be one big GTY-related function to maintain,
rather than 3, given the lack of need for PCH support.
Dave
[1] http://gcc.gnu.org/ml/gcc-patches/2013-08/msg01152.html
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
Hi, On Fri, 30 Aug 2013, Gabriel Dos Reis wrote: On Fri, Aug 30, 2013 at 9:02 AM, Jakub Jelinek ja...@redhat.com wrote: I thought the principle that was acquired was that gengtype shouldn't be improved to support more than what it does now…. If it means that we'll need to write and maintain tons of hand written code that could otherwise be generated and maintained by a tool for us, that principle doesn't look very good. Exactly. Back in March 2013, I asked about gengtype support for inheritance. http://gcc.gnu.org/ml/gcc/2013-03/msg00273.html This http://gcc.gnu.org/ml/gcc/2013-03/msg00295.html was the definitive answer that appeared to be the consensus. Well, it was a wrong decision then. For some smaller types writing manual marker might be a sensible thing, or for some extra complicated constructs. But here we're talking about the most simple struct hierarchy imaginable. Having to write manual markers for that one is absurd IMO. Ciao, Michael.
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Fri, Aug 30, 2013 at 10:21 AM, Michael Matz m...@suse.de wrote: Hi, On Fri, 30 Aug 2013, Gabriel Dos Reis wrote: On Fri, Aug 30, 2013 at 9:02 AM, Jakub Jelinek ja...@redhat.com wrote: I thought the principle that was acquired was that gengtype shouldn't be improved to support more than what it does now…. If it means that we'll need to write and maintain tons of hand written code that could otherwise be generated and maintained by a tool for us, that principle doesn't look very good. Exactly. Back in March 2013, I asked about gengtype support for inheritance. http://gcc.gnu.org/ml/gcc/2013-03/msg00273.html This http://gcc.gnu.org/ml/gcc/2013-03/msg00295.html was the definitive answer that appeared to be the consensus. Well, it was a wrong decision then. For some smaller types writing manual marker might be a sensible thing, or for some extra complicated constructs. But here we're talking about the most simple struct hierarchy imaginable. Having to write manual markers for that one is absurd IMO. We can reserve the emotional strong words for later. For now, the focus should be for us to ensure we are being consistent and making design decisions that carry consensus, hence my original note. -- Gaby
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Fri, Aug 30, 2013 at 11:21 AM, Michael Matz m...@suse.de wrote: Hi, On Fri, 30 Aug 2013, Gabriel Dos Reis wrote: On Fri, Aug 30, 2013 at 9:02 AM, Jakub Jelinek ja...@redhat.com wrote: I thought the principle that was acquired was that gengtype shouldn't be improved to support more than what it does now…. If it means that we'll need to write and maintain tons of hand written code that could otherwise be generated and maintained by a tool for us, that principle doesn't look very good. Exactly. Back in March 2013, I asked about gengtype support for inheritance. http://gcc.gnu.org/ml/gcc/2013-03/msg00273.html This http://gcc.gnu.org/ml/gcc/2013-03/msg00295.html was the definitive answer that appeared to be the consensus. Well, it was a wrong decision then. For some smaller types writing manual marker might be a sensible thing, or for some extra complicated constructs. But here we're talking about the most simple struct hierarchy imaginable. Having to write manual markers for that one is absurd IMO. I want to discourage extending gengtype more than necessary. Long term, I would like to see memory pools replacing GC. However, that is likely a long road so we should find an interim solution. I vaguely remember thinking about what would be needed to have gengtype deal with inheritance. It needed some pretty ugly annotations. This made gengtype even more magic. That's very bad for maintenance. One thing I liked is a suggestion that went something along the lines of creating some base templates that could be used to facilitate writing the manual markers. Perhaps we could minimally extend gengtype to generate those. But I think we can take advantage of C++ template features to facilitate this. Diego.
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Fri, Aug 30, 2013 at 11:37 AM, Jakub Jelinek ja...@redhat.com wrote:
Teaching the gengtype parser about
{struct,class} name : public {struct,class} someothername { ... }
as opposed to current
{struct,class} name { ... }
shouldn't be that hard. And, if the complaint is that we'd need to write
whole C++ parser for it, then the response could be that we already have
one C++ parser (and, even have plugin support and/or emit dwarf etc.).
It isn't. It's annoying and a duplication of effort.
So, gengtype could very well use a g++ plugin to emit the stuff it needs,
or parse DWARF, etc. And, we even could not require everybody actually
emitting those themselves, we could check some text form of that
(gengtype.state?) into the tree, so only people actually changing the
compiler would need to run the plugin.
Yes. Lawrence and I thought about moving gengtype inside g++. That
seemed like a promising approach.
Even if you have some stuff that helps you writing those, still it will be a
big source of bugs (very hard to debug) and a maintainance nightmare.
Debugging gengtype is much harder. It is magic code that is not visible.
Diego.
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Fri, Aug 30, 2013 at 10:37 AM, Jakub Jelinek ja...@redhat.com wrote:
On Fri, Aug 30, 2013 at 11:28:34AM -0400, Diego Novillo wrote:
Well, it was a wrong decision then. For some smaller types writing manual
marker might be a sensible thing, or for some extra complicated
constructs. But here we're talking about the most simple struct hierarchy
imaginable. Having to write manual markers for that one is absurd IMO.
I want to discourage extending gengtype more than necessary. Long
term, I would like to see memory pools replacing GC. However, that is
likely a long road so we should find an interim solution.
I have doubts that, still somewhat remember the obstack era and memory pools
would again bring all the issues back, but let's put that aside.
We didn't have the automation of RAII with obstacle, back then.
We do have evidence of widely use industrial
C and C++ compilers that are built around the idea of memory pools.
I vaguely remember thinking about what would be needed to have
gengtype deal with inheritance. It needed some pretty ugly
annotations. This made gengtype even more magic. That's very bad for
maintenance.
Teaching the gengtype parser about
{struct,class} name : public {struct,class} someothername { … }
I would suggest NOT to allow the {struct, class} part in the base-class clause.
as opposed to current
{struct,class} name { ... }
shouldn't be that hard. And, if the complaint is that we'd need to write
whole C++ parser for it, then the response could be that we already have
one C++ parser (and, even have plugin support and/or emit dwarf etc.).
We should not need one. At most the base classes would have the form
optional-typename optional-qualifying-scope identifier
optional-template-argument-list
that should cover most of what we want.
So, gengtype could very well use a g++ plugin to emit the stuff it needs,
or parse DWARF, etc. And, we even could not require everybody actually
emitting those themselves, we could check some text form of that
(gengtype.state?) into the tree, so only people actually changing the
compiler would need to run the plugin.
One thing I liked is a suggestion that went something along the lines
of creating some base templates that could be used to facilitate
writing the manual markers.
Even if you have some stuff that helps you writing those, still it will be a
big source of bugs (very hard to debug) and a maintainance nightmare.
Jakub
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Fri, Aug 30, 2013 at 11:28:34AM -0400, Diego Novillo wrote:
Well, it was a wrong decision then. For some smaller types writing manual
marker might be a sensible thing, or for some extra complicated
constructs. But here we're talking about the most simple struct hierarchy
imaginable. Having to write manual markers for that one is absurd IMO.
I want to discourage extending gengtype more than necessary. Long
term, I would like to see memory pools replacing GC. However, that is
likely a long road so we should find an interim solution.
I have doubts that, still somewhat remember the obstack era and memory pools
would again bring all the issues back, but let's put that aside.
I vaguely remember thinking about what would be needed to have
gengtype deal with inheritance. It needed some pretty ugly
annotations. This made gengtype even more magic. That's very bad for
maintenance.
Teaching the gengtype parser about
{struct,class} name : public {struct,class} someothername { ... }
as opposed to current
{struct,class} name { ... }
shouldn't be that hard. And, if the complaint is that we'd need to write
whole C++ parser for it, then the response could be that we already have
one C++ parser (and, even have plugin support and/or emit dwarf etc.).
So, gengtype could very well use a g++ plugin to emit the stuff it needs,
or parse DWARF, etc. And, we even could not require everybody actually
emitting those themselves, we could check some text form of that
(gengtype.state?) into the tree, so only people actually changing the
compiler would need to run the plugin.
One thing I liked is a suggestion that went something along the lines
of creating some base templates that could be used to facilitate
writing the manual markers.
Even if you have some stuff that helps you writing those, still it will be a
big source of bugs (very hard to debug) and a maintainance nightmare.
Jakub
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Fri, 2013-08-30 at 15:44 +0200, Michael Matz wrote:
Hi,
On Thu, 29 Aug 2013, David Malcolm wrote:
Successfully bootstrapped and tested on x86_64-unknown-linux-gnu: all
testcases show the same results as an unpatched build (relative to
r202029).
I'd like to see some statistics for cc1{,plus} codesize and for compile
time of something reasonably large (needing say 60 seconds to
compile normally), before/after patch series.
Here's the result of a pair of builds of r202029 without and with the
patches, configured with --enable-checking=release, running make, then
stripping debuginfo [1]
# ll */build/gcc/cc1
-rwxr-xr-x. 1 root root 13230048 Aug 30 15:00 control/build/gcc/cc1
-rwxr-xr-x. 1 root root 13230144 Aug 30 15:00 experiment/build/gcc/cc1
(98 bytes difference)
# ll */build/gcc/cc1obj
-rwxr-xr-x. 1 root root 13426336 Aug 30 15:00 control/build/gcc/cc1obj
-rwxr-xr-x. 1 root root 13426432 Aug 30 15:00 experiment/build/gcc/cc1obj
(96 bytes diff)
# ll */build/gcc/cc1plus
-rwxr-xr-x. 1 root root 14328480 Aug 29 13:59 control/build/gcc/cc1plus
-rwxr-xr-x. 1 root root 14328608 Aug 29 13:59 experiment/build/gcc/cc1plus
(128 bytes diff)
# ll */build/gcc/f951
-rwxr-xr-x. 1 root root 13960728 Aug 30 15:05 control/build/gcc/f951
-rwxr-xr-x. 1 root root 13960856 Aug 30 15:05 experiment/build/gcc/f951
(128 bytes diff)
# ll */build/gcc/jc1
-rwxr-xr-x. 1 root root 12607704 Aug 30 15:17 control/build/gcc/jc1
-rwxr-xr-x. 1 root root 12607704 Aug 30 15:17 experiment/build/gcc/jc1
(the same size)
So the overall sizes of such binaries are essentially unchanged.
To dig a bit deeper, I extended my asmdiff tool [2] to compare sizes of
functions; I'm attaching the results of comparing cc1plus before/after.
Any suggestions on what to compile to compare performance? By 60
seconds, do you mean 60s for one TU, or do you mean a large build e.g.
the linux kernel?
And the manual GTY markers are so not maintainable in the long run,
gengtype or something else really needs to be taught to create them
automatically.
Apart from the GTY aspect, how do people feel about the patch series?
FWIW I have vague thoughts about doing something similar for tree -
doing so *might* give an easier route to the type vs expression
separation that Andrew spoke about at the Cauldron rearchitecture BoF.
(I started looking at doing a similar C++-ification of rtx, but...
gah)
Dave
[1] yes, I built as root; this was done on a throwaway provisioning of a
RHEL 6.4 x86_64 box.
[2] https://github.com/davidmalcolm/asmdiff
Old: test/control/build/gcc/cc1plus
New: test/experiment/build/gcc/cc1plus
Function removed: Function('_start-0x1a684')
Function removed: Function('gt_pch_p_18gimple_statement_d(void*, void*, void
(*)(void*, void*), void*)')
Function removed: Function('gt_ggc_mx_gimple_statement_d(void*)')
Function removed: Function('gt_pch_nx_gimple_statement_d(void*)')
Function removed: Function('vecconstraint_expr, va_heap,
vl_ptr::safe_push(constraint_expr const)')
Function added: Function('_start-0x1a6ac')
Function added: Function('gt_ggc_mx(gimple_statement_base*)')
Function added: Function('gt_pch_nx(gimple_statement_base*)')
Function added: Function('gt_pch_nx(gimple_statement_base*, void (*)(void*,
void*), void*)')
Function added: Function('gt_pch_p_21gimple_statement_base(void*, void*, void
(*)(void*, void*), void*)')
Function added: Function('gt_ggc_mx_gimple_statement_base(void*)')
Function added: Function('gt_pch_nx_gimple_statement_base(void*)')
Function hash_tableoecount_hasher, xcallocator::find_slot_with_hash(void
const*, unsigned int, insert_option) changed size from 5984 to 5872 bytes
Function same_succ_def::equal(same_succ_def const*, same_succ_def const*)
changed size from 480 to 512 bytes
Function maybe_remove_unreachable_handlers() changed size from 112 to 128
bytes
Function verify_ssa_operands(gimple_statement_d*) changed size from 927 to
960 bytes
(renamed to verify_ssa_operands(gimple_statement_base*))
Function vect_analyze_data_ref_accesses(_loop_vec_info*, _bb_vec_info*)
changed size from 3920 to 3872 bytes
Function make_pass_ipa_pta(gcc::context*) changed size from 36880 to 16464
bytes
Function inline_merge_summary(cgraph_edge*) changed size from 17904 to 17920
bytes
Function release_defs_bitset(bitmap_head_def*) changed size from 2464 to 2304
bytes
Function gt_pch_nx(loop*) changed size from 144 to 32 bytes
Function make_pass_fold_builtins(gcc::context*) changed size from 5711 to
5727 bytes
Function dump_value_range(_IO_FILE*, value_range_d*) changed size from 32880
to 32800 bytes
Function extract_true_false_edges_from_block(basic_block_def*, edge_def**,
edge_def**) changed size from 20208 to 20224 bytes
Function tree_ssa_lim() changed size from 4847 to 4831 bytes
Function gt_ggc_mx_control_flow_graph(void*) changed size from 96 to 192 bytes
Function vectorize_loops() changed size from 3999 to 4191 bytes
Function
Re: [PATCH 0/6] Convert gimple to a C++ class hierarchy
On Thu, 2013-08-29 at 12:20 -0400, David Malcolm wrote:
[...]
Successfully bootstrapped and tested on x86_64-unknown-linux-gnu: all
testcases show the same results as an unpatched build (relative to
r202029).
I messed up the testing for this by accidentally configuring the builds
with --enable-checking=release, thus not running the run-time checks.
Upon rechecking with a plain ./configure (and thus enabling the
run-time checks), the patches break 20 test cases involving OpenMP.
Sorry about this.
I'm working on a fix [1]; I'll also add gcc_unreachable to the
PCH-related hooks for gimple (and thus reduce the size of the
handwritten GTY code by a factor of 3; not sure if it will make it any
more popular...).
Dave
[1] I believe the issue is this overzealous generated is_a_helper test:
template
template
inline bool
is_a_helper gimple_statement_omp_parallel::test (gimple gs)
{
return gs-code == GIMPLE_OMP_PARALLEL;
}
which should clearly also permit gs-code to be GIMPLE_OMP_TASK; this
bug leads to various failures in omp-lowering for obvious reasons.
[PATCH 0/6] Convert gimple to a C++ class hierarchy
The various gimple types are currently implemented using a hand-coded
C inheritance scheme, with a union gimple_statement_d holding the
various possible structs for a statement.
The following series of patches convert it to a C++ class
hierarchy, using the existing structs, eliminating the union. The
gimple typedef changes from being a
(union gimple_statement_d *)
to being a:
(struct gimple_statement_base *)
There are no virtual functions in the new code: the sizes of the various
structs are unchanged.
It makes use of is-a.h, using the as_a T template function to
perform downcasts, which are checked (via gcc_checking_assert) in an
ENABLE_CHECKING build, and are simple casts in an unchecked build,
albeit it in an inlined function rather than a macro.
For example, one can write:
gimple_statement_phi *phi =
as_a gimple_statement_phi (gsi_stmt (gsi));
and then directly access the fields of the phi, as a phi. The existing
accessor functions in gimple.h become somewhat redundant in this
scheme, but are preserved.
I believe this is a superior scheme to the C implementation:
* We can get closer to compile-time type-safety, checking the gimple
code once and downcasting with an as_a, then directly accessing
fields, rather than going through accessor functions that check
each time. In some places we may want to replace a gimple with
a subclass e.g. phis are always of the phi subclass, to get full
compile-time type-safety.
* This scheme is likely to be easier for newbies to understand.
* Currently in gdb, dereferencing a gimple leads to screenfuls of text,
showing all the various union values. With this, you get just the base
class, and can cast it to the appropriate subclass.
* We're working directly with the language constructs, rather than
rolling our own, and thus other tools can better understand the
code. For example, you can see Doxygen's rendering of the gimple
class hierarchy (in the modified code) here:
http://fedorapeople.org/~dmalcolm/gcc/2013-08-28/doxygen/html/structgimple__statement__base.html
The names of the structs are rather verbose. I would prefer to also
rename them all to eliminate the _statement component:
gimple_statement_base - gimple_base
gimple_statement_phi - gimple_phi
gimple_statement_omp - gimple_omp
etc, but I didn't do this to mimimize the patch size. But if the core
maintainers are up for that, I can redo the patch series with that
change also.
The patch is in 6 parts; all of them are needed together.
* Patch 1 of 6: This patch adds inheritance to the various gimple
types, eliminating the initial baseclass fields, and eliminating the
union gimple_statement_d. All the types remain structs. They
become marked with GTY((user)).
* Patch 2 of 6: This patch ports various accessor functions within
gimple.h to the new scheme.
* Patch 3 of 6: This patch is autogenerated by refactor_gimple.py
from https://github.com/davidmalcolm/gcc-refactoring-scripts
There is a test suite test_refactor_gimple.py which may give a
clearer idea of the changes that the script makes (and add
confidence that it's doing the right thing).
The patch converts code of the form:
{
GIMPLE_CHECK (gs, SOME_CODE);
gimple_subclass_get/set_some_field (gs, value);
}
to code of this form:
{
some_subclass *stmt = as_a some_subclass (gs);
stmt-some_field = value;
}
It also autogenerates specializations of
is_a_helper T::test
equivalent to a GIMPLE_CHECK() for use by is_a and as_a.
* Patch 4 of 6: This patch implement further specializations of
is_a_helper T::test, for gimple_has_ops and gimple_has_mem_ops.
* Patch 5 of 6: This patch does the rest of porting from union access
to subclass access (all the fiddly places that the script in patch 3
couldn't handle).
* Patch 6 of 6: This patch adds manual implementation of the GTY
hooks, written by heavily-editing the autogenerated gtype-desc.c
code from an earlier incarnation. It implements the equivalent
of chain_next for gimple statements.
Successfully bootstrapped and tested on x86_64-unknown-linux-gnu: all
testcases show the same results as an unpatched build (relative to
r202029).
OK for trunk?
gcc/Makefile.in |2 +-
gcc/coretypes.h |5 +-
gcc/ggc.h |6 +-
gcc/gimple-iterator.c | 72 +--
gcc/gimple-pretty-print.c |6 +-
gcc/gimple-pretty-print.h |4 +-
gcc/gimple-streamer-in.c | 19 +-
gcc/gimple-streamer-out.c |2 +-
gcc/gimple.c | 819 +++-
gcc/gimple.h | 1545 +
gcc/gimplify.c|4 +-
gcc/system.h |2 +-
gcc/tree-cfg.c|6 +-
gcc/tree-inline.c |2 +-
gcc/tree-phinodes.c | 39 +-
gcc/tree-ssa-ccp.c
