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