Re: Passes are now C++ classes (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)

2013-08-07 Thread Eric Botcazou
 The mips*-*-* targets are not building.  It looks like the mips reorg
 pass (pass_mips_machine_reorg2) in config/mips/mips.c was not converted
 and/or was not converted correctly.

Likewise for the SPARC targets, because of the same issue.  David, could you 
take a quick look?  Thanks in advance.

-- 
Eric Botcazou


Re: Passes are now C++ classes (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)

2013-08-06 Thread Steve Ellcey
On Mon, 2013-08-05 at 17:03 -0400, David Malcolm wrote:

 Given all of the above testing I'm reasonably confident that this works.
 However this is such a large change [1] that there's a non-zero chance
 of at least one glitch - let me know if you see any breakages.

The mips*-*-* targets are not building.  It looks like the mips reorg
pass (pass_mips_machine_reorg2) in config/mips/mips.c was not converted
and/or was not converted correctly.

Steve Ellcey
sell...@mips.com




[Patch] Unbreak build on mips (was Re: Passes are now C++ classes (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes))

2013-08-06 Thread David Malcolm
On Tue, 2013-08-06 at 08:16 -0700, Steve Ellcey wrote:
 On Mon, 2013-08-05 at 17:03 -0400, David Malcolm wrote:
 
  Given all of the above testing I'm reasonably confident that this works.
  However this is such a large change [1] that there's a non-zero chance
  of at least one glitch - let me know if you see any breakages.
 
 The mips*-*-* targets are not building.  It looks like the mips reorg
 pass (pass_mips_machine_reorg2) in config/mips/mips.c was not converted
 and/or was not converted correctly.

Sorry about this.

I was able to reproduce this build error with configure
--target=mips-linux-elf:
../../src/gcc/config/mips/mips.c:16379:28: error: expected
primary-expression before ‘.’ token

I'm attaching a patch which fixes that issue for me.  Only lightly
tested (buildhost=x86_64, target as above) - I'm able to build stage1,
and cc1 appears to generate assembler on a simple test case.  I stepped
through the changed code in the debugger and it appears to do the right
thing.  However I'm not familiar with the internals of the pass in
question.

commit 11d46884e8bd9802b0f528a16b3970b4076fe8a9
Author: David Malcolm dmalc...@redhat.com
Date:   Tue Aug 6 13:48:59 2013 -0400

gcc/
	* config/mips/mips.c (insert_pass_mips_machine_reorg2): Move
	into...
	(mips_option_override): ...here, porting to new C++ API for
	passes.

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index 05ba003..4da80f4 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -56,6 +56,7 @@ along with GCC; see the file COPYING3.  If not see
 #include target-globals.h
 #include opts.h
 #include tree-pass.h
+#include context.h
 
 /* True if X is an UNSPEC wrapper around a SYMBOL_REF or LABEL_REF.  */
 #define UNSPEC_ADDRESS_P(X)	\
@@ -16374,13 +16375,6 @@ make_pass_mips_machine_reorg2 (gcc::context *ctxt)
   return new pass_mips_machine_reorg2 (ctxt);
 }
 
-struct register_pass_info insert_pass_mips_machine_reorg2 =
-{
-  pass_mips_machine_reorg2.pass,	/* pass */
-  dbr,/* reference_pass_name */
-  1,	/* ref_pass_instance_number */
-  PASS_POS_INSERT_AFTER			/* po_op */
-};
 
 /* Implement TARGET_ASM_OUTPUT_MI_THUNK.  Generate rtl rather than asm text
in order to avoid duplicating too much logic from elsewhere.  */
@@ -17174,6 +17168,14 @@ mips_option_override (void)
   /* We register a second machine specific reorg pass after delay slot
  filling.  Registering the pass must be done at start up.  It's
  convenient to do it here.  */
+  opt_pass *new_pass = make_pass_mips_machine_reorg2 (g);
+  struct register_pass_info insert_pass_mips_machine_reorg2 =
+{
+  new_pass,		/* pass */
+  dbr,			/* reference_pass_name */
+  1,			/* ref_pass_instance_number */
+  PASS_POS_INSERT_AFTER	/* po_op */
+};
   register_pass (insert_pass_mips_machine_reorg2);
 
   if (TARGET_HARD_FLOAT_ABI  TARGET_MIPS5900)


Re: [Patch] Unbreak build on mips (was Re: Passes are now C++ classes (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes))

2013-08-06 Thread Richard Sandiford
David Malcolm dmalc...@redhat.com writes:
 commit 11d46884e8bd9802b0f528a16b3970b4076fe8a9
 Author: David Malcolm dmalc...@redhat.com
 Date:   Tue Aug 6 13:48:59 2013 -0400

 gcc/
   * config/mips/mips.c (insert_pass_mips_machine_reorg2): Move
   into...
   (mips_option_override): ...here, porting to new C++ API for
   passes.

OK.  Thanks for the quick fix!

Richard


Re: [Patch] Unbreak build on mips (was Re: Passes are now C++ classes (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes))

2013-08-06 Thread David Malcolm
On Tue, 2013-08-06 at 19:11 +0100, Richard Sandiford wrote:
 David Malcolm dmalc...@redhat.com writes:
  commit 11d46884e8bd9802b0f528a16b3970b4076fe8a9
  Author: David Malcolm dmalc...@redhat.com
  Date:   Tue Aug 6 13:48:59 2013 -0400
 
  gcc/
  * config/mips/mips.c (insert_pass_mips_machine_reorg2): Move
  into...
  (mips_option_override): ...here, porting to new C++ API for
  passes.
 
 OK.  Thanks for the quick fix!

Thanks; committed to trunk as r201542.




Re: Passes are now C++ classes (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)

2013-08-06 Thread Basile Starynkevitch
On Mon, 2013-08-05 at 17:03 -0400, David Malcolm wrote:
 On Mon, 2013-07-29 at 15:41 -0600, Jeff Law wrote:
  On 07/26/2013 09:04 AM, David Malcolm wrote:
   This patch is the hand-written part of the conversion of passes from
   C structs to C++ classes.  It does not work without the subsequent
   autogenerated part, which is huge.
  [ ... ]
  With the changes from pipeline - pass_manager this is fine.  As is the 
  follow-up autogenerated patch.
 
 I've committed this and the other patches that needed to go with it to
 trunk, with the name fix, having successfully bootstrapped and tested
 them on x86_64-unknown-linux-gnu - so opt_pass is now a C++ class
 hierarchy, allowing for pass instances to hold pass-specific data
 (albeit not GTY refs yet), 

Did you follow my suggestion of putting __FILE__ and __LINE__ in
instances of opt_pass? I really hope that will go into 4.9!

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} ***




Passes are now C++ classes (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)

2013-08-05 Thread David Malcolm
On Mon, 2013-07-29 at 15:41 -0600, Jeff Law wrote:
 On 07/26/2013 09:04 AM, David Malcolm wrote:
  This patch is the hand-written part of the conversion of passes from
  C structs to C++ classes.  It does not work without the subsequent
  autogenerated part, which is huge.
 [ ... ]
 With the changes from pipeline - pass_manager this is fine.  As is the 
 follow-up autogenerated patch.

I've committed this and the other patches that needed to go with it to
trunk, with the name fix, having successfully bootstrapped and tested
them on x86_64-unknown-linux-gnu - so opt_pass is now a C++ class
hierarchy, allowing for pass instances to hold pass-specific data
(albeit not GTY refs yet), and the future possibility of multiple pass
managers and pass pipelines (e.g. for embedding GCC for
JIT-compilation).

Specifically, I fixed the names and rebased against master and tested
repeatedly (for the later GC patches, which are still being reviewed).
I refreshed the 03/11 patch (which became r201505) to resolve a
trivial conflict in cgraphunit.c, and then bootstrapped and tested the
series of 5 patches against r201494 on x86_64-unknown-linux-gnu (RHEL
6.4 in fact).

It successfully built all 3 stages, and gave the same testing results as
an unpatched build of r201494.

Given that, I've committed the following to trunk:
  * r201505: this patch (aka 03/11; handwritten part of conversion,
approved above by Jeff)
  * r201506: the zero-initialization of pass_manager (aka 3.1/11;
approved by rth in
http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00147.html )
  * r201508: the big autogenerated patch (aka 04/11; approved above by
Jeff)
  * r201509: adding -fno-rtti when building plugins in the testsuite
(aka 05/11; approved by Jeff in
http://gcc.gnu.org/ml/gcc-patches/2013-07/msg01433.html )
  * r201511: aka 06/11; using pass-clone and fixing pass switch
numbering (approved by rth in
http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00054.html )

Note that r201505 and r201506 don't compile by themselves; they need
r201508 to update all of the passes to the new internal API, and r201509
and r201511 are then needed otherwise you'll see regressions in the
testsuite.

Given all of the above testing I'm reasonably confident that this works.
However this is such a large change [1] that there's a non-zero chance
of at least one glitch - let me know if you see any breakages.  One
glitch that I ran into when applying the patches to svn was that I saw
some end-of-file whitespace changes in some of the gcc/testsuite files:
I'm not sure how those crept in, but I used svn revert by hand on them
before committing to ensure that only the files I was expecting to
change were changed.

Some of followup patches in the series are now approved, some aren't, so
I'm next going to bootstrap/test/commit the approved ones as
appropriate.

Dave

[1] 127 files changed, 10227 insertions(+), 4465 deletions(-)



Re: [PATCH 3.1/11] Explicitly initialize the macro-generated pass fields (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)

2013-08-03 Thread Richard Henderson
On 08/02/2013 02:38 PM, David Malcolm wrote:
 OK for trunk? (as part of patches 3-6)

Ok.


Re: [PATCH 3.1/11] Explicitly initialize the macro-generated pass fields (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)

2013-08-02 Thread David Malcolm
On Thu, 2013-08-01 at 13:13 -0400, David Malcolm wrote:
 On Mon, 2013-07-29 at 15:41 -0600, Jeff Law wrote:
  On 07/26/2013 09:04 AM, David Malcolm wrote:
   This patch is the hand-written part of the conversion of passes from
   C structs to C++ classes.  It does not work without the subsequent
   autogenerated part, which is huge.
  [ ... ]
  With the changes from pipeline - pass_manager this is fine.  As is the 
  follow-up autogenerated patch.
 
 Thanks.
 
 The current status is that Jeff has approved patches 1-5 for the trunk,
 and patches 6-11 haven't yet been reviewed by a global reviewer.  I have
 committed patches 1 and 2, having bootstrapped+tested each in turn, but
 I can't commit any more of these without further review - details
 follow.
 
 I had only bootstrapped and tested the combination of all 11 patches
 together, so I've been attempting to test the approved patches.  For
 reference:
   * Patch 3 is the handwritten part of the conversion of passes to C++
 classes
   * Patch 4 is the autogenerated part
   * Patch 5 adds -fno-rtti to the testsuite when building plugins
   * Patch 6 is the not-yet-approved patch currently named Rewrite how
 instances of passes are cloned (but that's not all it does, see below).
 
 I had thought that the combination of patch 3 + 4 + 5 would work as a
 unit, and hoped to commit each of these after testing the combination of
 (3, 4, 5), but upon attempting a bootstrap I found two bugs:
 
   (a) patch 3 adds an gcc_assert to the pass_manager constructor to
 ensure that pass instance fields are NULL when created, to ensure that
 the macro-driven logic is correct.   However, the instance fields
 haven't been explicitly initialized at that point, leading to sporadic
 assertion failures.  This wasn't an issue for the full patch series
 since patch 11 adds an operator new for pass_manager, allocating it from
 the GC heap, using the *cleared* allocator:
void*
pass_manager::operator new (size_t sz)
{
  return ggc_internal_cleared_alloc_stat (sz MEM_STAT_INFO);
}
 hence ensuring that the fields are zeroed.  So patch 3 works with patch
 11, but not without.  In the meantime, I'm attaching a new patch 3.1,
 which explicitly zeroes these fields early on in the pass_manager ctor.
 
   (b) with just these patches, although static_pass_number for every
 pass instance is correct, the dumpfile *switch* numbering is wrong,
 leading to numerous testsuite failures (e.g. unrecognized command line
 option '-fdump-tree-dce2') .  Patch 6 is needed: it does the necessary
 fixups at the right time to ensure that the per-pass-instance dump files
 get the correct switch names (I'll add some more notes on this to that
 email).
 
 With the attached patch, the combination of patches (03, 03.1, 04, 05
 *and* 06) successfully bootstraps on x86_64-unknown-linux-gnu, and all
 testcases show the same results as an unpatched build (relative to
 r201397).
 
 So I'm looking for review of the attached patch, and of at least patch 6
 before I can proceed with this.  AIUI, Jeff is away at the moment, so
 another global reviewer would need to do it.
Here's a better version of this patch: instead of explicitly
initializing the fields (which would become redundant when patch 11 goes
in), instead this patch adds an operator new, using xcalloc.  This
operator new gets reimplemented in a later version of patch 11 to use
ggc_internal_cleared_alloc_stat when the class becomes GC-managed, hence
at each stage of committing, the initialization is sane.

I've successfully bootstrapped the *end result* of the revised patch
series 3-11 on x86_64-unknown-linux-gnu: all testcases show the same
results as an unpatched build (relative to r201397).

OK for trunk? (as part of patches 3-6)

From 3198835b85b9a35906bf93ba8f510762e8e017b9 Mon Sep 17 00:00:00 2001
From: David Malcolm dmalc...@redhat.com
Date: Wed, 31 Jul 2013 14:20:07 -0400
Subject: [PATCH 04/15] Zero-initialize pass_manager

Ensure that pass_manager instances are fully zero-initialized, by
providing an operator new, implemented using xcalloc.

gcc/

	* passes.c (pass_manager::operator new): New.
---
 gcc/pass_manager.h | 2 ++
 gcc/passes.c   | 7 +++
 2 files changed, 9 insertions(+)

diff --git a/gcc/pass_manager.h b/gcc/pass_manager.h
index ea078a5..00f0b1c 100644
--- a/gcc/pass_manager.h
+++ b/gcc/pass_manager.h
@@ -47,6 +47,8 @@ class context;
 class pass_manager
 {
 public:
+  void *operator new (size_t sz);
+
   pass_manager(context *ctxt);
 
   void register_pass (struct register_pass_info *pass_info);
diff --git a/gcc/passes.c b/gcc/passes.c
index fcbd630..8efce30 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -1339,6 +1339,13 @@ pass_manager::register_pass (struct register_pass_info *pass_info)
 - all_passes
 */
 
+void *
+pass_manager::operator new (size_t sz)
+{
+  /* Ensure that all fields of the pass manager are zero-initialized.  */
+  return xcalloc (1, sz);
+}
+
 pass_manager::pass_manager (context 

[PATCH 3.1/11] Explicitly initialize the macro-generated pass fields (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)

2013-08-01 Thread David Malcolm
On Mon, 2013-07-29 at 15:41 -0600, Jeff Law wrote:
 On 07/26/2013 09:04 AM, David Malcolm wrote:
  This patch is the hand-written part of the conversion of passes from
  C structs to C++ classes.  It does not work without the subsequent
  autogenerated part, which is huge.
 [ ... ]
 With the changes from pipeline - pass_manager this is fine.  As is the 
 follow-up autogenerated patch.

Thanks.

The current status is that Jeff has approved patches 1-5 for the trunk,
and patches 6-11 haven't yet been reviewed by a global reviewer.  I have
committed patches 1 and 2, having bootstrapped+tested each in turn, but
I can't commit any more of these without further review - details
follow.

I had only bootstrapped and tested the combination of all 11 patches
together, so I've been attempting to test the approved patches.  For
reference:
  * Patch 3 is the handwritten part of the conversion of passes to C++
classes
  * Patch 4 is the autogenerated part
  * Patch 5 adds -fno-rtti to the testsuite when building plugins
  * Patch 6 is the not-yet-approved patch currently named Rewrite how
instances of passes are cloned (but that's not all it does, see below).

I had thought that the combination of patch 3 + 4 + 5 would work as a
unit, and hoped to commit each of these after testing the combination of
(3, 4, 5), but upon attempting a bootstrap I found two bugs:

  (a) patch 3 adds an gcc_assert to the pass_manager constructor to
ensure that pass instance fields are NULL when created, to ensure that
the macro-driven logic is correct.   However, the instance fields
haven't been explicitly initialized at that point, leading to sporadic
assertion failures.  This wasn't an issue for the full patch series
since patch 11 adds an operator new for pass_manager, allocating it from
the GC heap, using the *cleared* allocator:
   void*
   pass_manager::operator new (size_t sz)
   {
 return ggc_internal_cleared_alloc_stat (sz MEM_STAT_INFO);
   }
hence ensuring that the fields are zeroed.  So patch 3 works with patch
11, but not without.  In the meantime, I'm attaching a new patch 3.1,
which explicitly zeroes these fields early on in the pass_manager ctor.

  (b) with just these patches, although static_pass_number for every
pass instance is correct, the dumpfile *switch* numbering is wrong,
leading to numerous testsuite failures (e.g. unrecognized command line
option '-fdump-tree-dce2') .  Patch 6 is needed: it does the necessary
fixups at the right time to ensure that the per-pass-instance dump files
get the correct switch names (I'll add some more notes on this to that
email).

With the attached patch, the combination of patches (03, 03.1, 04, 05
*and* 06) successfully bootstraps on x86_64-unknown-linux-gnu, and all
testcases show the same results as an unpatched build (relative to
r201397).

So I'm looking for review of the attached patch, and of at least patch 6
before I can proceed with this.  AIUI, Jeff is away at the moment, so
another global reviewer would need to do it.

Thanks
Dave
From 49d7df3c645459a0f90179dfb1a24cef459b186e Mon Sep 17 00:00:00 2001
From: David Malcolm dmalc...@redhat.com
Date: Wed, 31 Jul 2013 14:20:07 -0400
Subject: [PATCH 03.1/15] Explicitly initialize the macro-generated pass fields
 to NULL

gcc/

	* passes.c (pass_manager::pass_manager): Explicitly initialize
	the macro-generated pass fields to NULL.
---
 gcc/passes.c | 22 ++
 1 file changed, 22 insertions(+)

diff --git a/gcc/passes.c b/gcc/passes.c
index fcbd630..c4f4f39 100644
--- a/gcc/passes.c
+++ b/gcc/passes.c
@@ -1352,6 +1352,28 @@ pass_manager::pass_manager (context *ctxt)
   GCC_PASS_LISTS
 #undef DEF_PASS_LIST
 
+  /* For safety, NULL-initialize the various fields for individual pass
+ instances (mainly so that the:
+	gcc_assert (NULL == PASS ## _ ## NUM);
+ sees an initial NULL value).
+
+ We cannot use member initializers for this since pass-instances.def
+ contains semicolons.  */
+
+#define INSERT_PASSES_AFTER(PASS)
+#define PUSH_INSERT_PASSES_WITHIN(PASS)
+#define POP_INSERT_PASSES()
+#define NEXT_PASS(PASS, NUM) PASS ## _ ## NUM = NULL;
+#define TERMINATE_PASS_LIST()
+
+#include pass-instances.def
+
+#undef INSERT_PASSES_AFTER
+#undef PUSH_INSERT_PASSES_WITHIN
+#undef POP_INSERT_PASSES
+#undef NEXT_PASS
+#undef TERMINATE_PASS_LIST
+
   /* Build the tree of passes.  */
 
 #define INSERT_PASSES_AFTER(PASS) \
-- 
1.7.11.7



Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes

2013-07-29 Thread David Malcolm
On Sun, 2013-07-28 at 10:37 +0200, Basile Starynkevitch wrote:
 On Fri, 2013-07-26 at 11:04 -0400, David Malcolm wrote:
  This patch is the hand-written part of the conversion of passes from
  C structs to C++ classes.  It does not work without the subsequent
  autogenerated part, which is huge.
  
  Given that the autogenerated part of the conversion is very large
  (500k), for the sake of human comprehension I have kept the change as
  two separate patches to keep the hand-written changes separate from the
  automatically-generated ones.  I would commit these as two separate
  changes to SVN in order to keep this readability for posterity in the
  logs as well as at review-time.
  
  This pair of patches eliminates the mutable global variables
  representing the passes, allowing for multiple compilation contexts in
  one process, potentially with different combinations of passes, and with
  pass instance owning additional data.
  
  It converts the hierarchy of opt_pass types into an actual C++ class
  hierarchy, where each of:
  
* gimple_opt_pass
* rtl_opt_pass
* ipa_opt_pass_d
* simple_ipa_opt_pass
  
  all become subclasses of opt_pass.
 [...]
 
 This looks good to me. I suggest adding into the `opt_pass` class two
 constant fields for the approximate source location of the pass, e.g. a
 field const char* _file; and another const unsigned _lineno; initialized
 with __FILE__ and __LINE__ respectively.
 
 This won't cost much (we don't have zillions of instances of
 opt_pass) and would help a lot finding where (in which source file)
 an actual pass is.
 
 This is particularly useful for newbies writing plugins (which are
 trying to add new passes). It takes a lot of time to them to find which
 actual source file inside the compiler is implementing a given
 (existing) pass.

Thanks - I like this idea.  As a relative newbie myself, I've often
found myself hunting down the implementation of a specific pass, and it
seems like your idea is something we'd want to expose in plugins such as
MELT and the Python plugin - I wrote a script to draw a subway map of
GCC's passes:
https://gcc-python-plugin.readthedocs.org/en/latest/tables-of-passes.html
and it would be great to be able to add hyperlinks to the relevant
passes.

So this would be an extra (const char*) and int per pass, or per pass
instance, giving about 2 kilobytes of extra memory usage, which sounds
acceptable to me.  We could either put it in the pass_data instance, so
that this could look like this (and be purely const):

  const pass_data pass_data_vrp =
  {
GIMPLE_PASS, /* type */
vrp, /* name */
OPTGROUP_NONE, /* optinfo_flags */
true, /* has_gate */
true, /* has_execute */
TV_TREE_VRP, /* tv_id */
PROP_ssa, /* properties_required */
0, /* properties_provided */
0, /* properties_destroyed */
0, /* todo_flags_start */
( TODO_cleanup_cfg | TODO_update_ssa
  | TODO_verify_ssa
  | TODO_verify_flow ), /* todo_flags_finish */
__FILE__, __LINE__,
*   ^^^ New stuff here
  };

or in the opt_pass itself, looking like:

  class pass_vrp : public gimple_opt_pass
  {
  public:
pass_vrp(gcc::context *ctxt)
  : gimple_opt_pass(pass_data_vrp, ctxt, __FILE__, __LINE__)
*^^
*^ New stuff here
{}

A related idea occurred to me: adding links to the *documentation* of
the specific passes: right now, if you want to go from a pass to the
documentation of said pass, there doesn't seem to be a consistent URL
pattern.  For example, tree-vrp.c defines pass_vrp, with name vrp,
but the documentation is in 
  @item Value range propagation
within passes.texi
In the HTML build of the documentation, this is documented on this page:
http://gcc.gnu.org/onlinedocs/gccint/Tree-SSA-passes.html#Tree-SSA-passes
though the generated HTML appears to have no per-pass anchors; all I see
is:
  liValue range propagation
followed by a p tag (it's also not-well-formed as XHTML, but that's a
whole other issue).
So it would be great if 
(a) the pass docs had per-pass anchors and
(b) the runtime pass metadata contained enough information so that we
could generate URLs to the docs, so that e.g. you could ask for docs at
the command-line and get sane URLs.

It may be possible to achieve (b) by making the anchors in (a) be the
pass names.




Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes

2013-07-29 Thread Jeff Law

On 07/26/2013 09:04 AM, David Malcolm wrote:

This patch is the hand-written part of the conversion of passes from
C structs to C++ classes.  It does not work without the subsequent
autogenerated part, which is huge.

[ ... ]
With the changes from pipeline - pass_manager this is fine.  As is the 
follow-up autogenerated patch.


Jeff


Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes

2013-07-28 Thread Basile Starynkevitch
On Fri, 2013-07-26 at 11:04 -0400, David Malcolm wrote:
 This patch is the hand-written part of the conversion of passes from
 C structs to C++ classes.  It does not work without the subsequent
 autogenerated part, which is huge.
 
 Given that the autogenerated part of the conversion is very large
 (500k), for the sake of human comprehension I have kept the change as
 two separate patches to keep the hand-written changes separate from the
 automatically-generated ones.  I would commit these as two separate
 changes to SVN in order to keep this readability for posterity in the
 logs as well as at review-time.
 
 This pair of patches eliminates the mutable global variables
 representing the passes, allowing for multiple compilation contexts in
 one process, potentially with different combinations of passes, and with
 pass instance owning additional data.
 
 It converts the hierarchy of opt_pass types into an actual C++ class
 hierarchy, where each of:
 
   * gimple_opt_pass
   * rtl_opt_pass
   * ipa_opt_pass_d
   * simple_ipa_opt_pass
 
 all become subclasses of opt_pass.
[...]

This looks good to me. I suggest adding into the `opt_pass` class two
constant fields for the approximate source location of the pass, e.g. a
field const char* _file; and another const unsigned _lineno; initialized
with __FILE__ and __LINE__ respectively.

This won't cost much (we don't have zillions of instances of
opt_pass) and would help a lot finding where (in which source file)
an actual pass is.

This is particularly useful for newbies writing plugins (which are
trying to add new passes). It takes a lot of time to them to find which
actual source file inside the compiler is implementing a given
(existing) pass.

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} ***