Re: Passes are now C++ classes (was Re: [PATCH 03/11] Handwritten part of conversion of passes to C++ classes)
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)
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))
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))
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))
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)
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)
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)
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)
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)
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
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
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
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} ***