From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25676 invoked by alias); 23 Jul 2013 15:43:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 25667 invoked by uid 89); 23 Jul 2013 15:43:18 -0000 X-Spam-SWARE-Status: No, score=-4.6 required=5.0 tests=AWL,BAYES_50,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,RDNS_NONE,SPF_HELO_PASS,SPF_PASS autolearn=no version=3.3.1 Received: from Unknown (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Tue, 23 Jul 2013 15:43:12 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r6NFh59J019858 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Tue, 23 Jul 2013 11:43:05 -0400 Received: from [10.18.25.132] ([10.18.25.132]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r6NFh4wD029521; Tue, 23 Jul 2013 11:43:05 -0400 Message-ID: <1374594184.3898.68.camel@surprise> Subject: Re: [PATCH 3/4] Introduce NEXT_PASS_NUM macro From: David Malcolm To: Martin Jambor Cc: gcc-patches@gcc.gnu.org Date: Tue, 23 Jul 2013 15:46:00 -0000 In-Reply-To: <20130723144602.GB3658@virgil.suse> References: <1374110303-9758-1-git-send-email-dmalcolm@redhat.com> <1374110303-9758-4-git-send-email-dmalcolm@redhat.com> <20130722182548.GB9531@virgil.suse> <1374520953.3898.29.camel@surprise> <20130723144602.GB3658@virgil.suse> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-SW-Source: 2013-07/txt/msg01029.txt.bz2 On Tue, 2013-07-23 at 16:46 +0200, Martin Jambor wrote: > Hi, > > On Mon, Jul 22, 2013 at 03:22:33PM -0400, David Malcolm wrote: > > On Mon, 2013-07-22 at 20:25 +0200, Martin Jambor wrote: > > > On Wed, Jul 17, 2013 at 09:18:22PM -0400, David Malcolm wrote: > > > > gcc/ > > > > > > > > Explicitly number the instances of passes within passes.def. > > > > > > > > This is needed by a subsequent patch so that we can create > > > > fields within the pipeline class for each pass instance (to help > > > > locate pass instances when debugging). > > > > > > > > > > I don't really understand what you want to achieve. Are you sure the > > > benefits are worth the work necessary to implement the processing of > > > passes.def.in? Especially given that we already initialize > > > static_pass_number at run time and copy stuff around in > > > make_pass_instance when it is already set. I assume this would > > > somehow allow us to directly dump data of say forwprop3 as apposed to > > > forwprop2 to but that would require constant awareness of the sequence > > > number of the currently running pass, which I think is also unpleasant > > > and error-prone. > > > > > > I mean, you may have perfectly legitimate reasons for doing this, I'm > > > just wondering whether we are perhaps over-engineering this a bit. > > > > The main goal here is part of eliminating global variables from gcc [1], > > to be able to create: > > > > class pipeline > > { > > /* omitting various other fields for clarity */ > > > > opt_pass *pass_warn_unused_result; > > opt_pass *pass_diagnose_omp_blocks; > > opt_pass *pass_diagnose_tm_blocks; > > opt_pass *pass_mudflap_1; > > opt_pass *pass_lower_omp; > > opt_pass *pass_lower_cf; > > opt_pass *pass_lower_tm; > > opt_pass *pass_refactor_eh; > > opt_pass *pass_lower_eh; > > opt_pass *pass_build_cfg; > > opt_pass *pass_warn_function_return; > > opt_pass *pass_expand_omp; > > opt_pass *pass_build_cgraph_edges; > > opt_pass *pass_ipa_free_lang_data; > > opt_pass *pass_ipa_function_and_variable_visibility; > > opt_pass *pass_early_local_passes; > > opt_pass *pass_fixup_cfg; > > opt_pass *pass_init_datastructures; > > /* etc */ > > opt_pass *pass_clean_state; > > }; > > > > without having to list all of the pass kinds again, thus reusing the > > pass description from passes.def. Without the numbering, I couldn't see > > a good way to avoid duplicate field names in the class. So the > > numbering gives us uniqueness of field names in that class (and also > > makes debugging slightly easier, but that's a minor side-benefit). > > > > I really think the easier debugging benefit is really very small, if > any. Is there another one? Otherwise, I wouldn't bother with > explicit static fields for each pass but just have a linked list of > them. If we ever make the pass manager to really be a manager of some > sort, this will happen anyway. > > And as far as gdb is concerned, I'd rather avoid typing: > > p current_context->pipeline_for_Ox_except_Og->my_great_pass_with_long_name_number_2->my_array[1]->stuff > p current_context->pipeline_for_Ox_except_Og->my_great_pass_with_long_name_number_2->my_array[2]->stuff > p current_context->pipeline_for_Ox_except_Og->my_great_pass_with_long_name_number_2->my_array[3]->stuff Thanks - yes - I completely agree, having spent a lot of time in gdb with this lately :) Note that there is only one "pipeline" per context, and I've kept the existing pass struct names (meaning "pass_vrp" rather than "vrp"). BTW, you mentioned dumping in an earlier post, sorry for not clarifying that aspect. I haven't changed how dumping works, and I've taken some care to ensure that the numbering of the pass instances isn't disturbed (an earlier version of my patches broke that, leading to some of the test suite failing). So whatever static_pass_number the 2nd instance of vrp ended up with before, it will continue to end up with after my patches. > and instead do > > set $d = (my_great_pass_class *) current_context->current_pass > p $d->my_array[1]->stuff > p $d->my_array[2]->stuff > p $d->my_array[3]->stuff > > Or am I missing something? Otherwise I'd just say don't bother with awk. To give you a flavor of what I'm aiming at, here's a transcript from gdb on a build with some further patches, with some comments added inline: The global "current context" variable is simply "g", for ease of typing - and my hope is that eventually this will be the *only* global variable [1]: (gdb) p g $1 = (gcc::context *) 0x15652a0 In the talk I gave at Cauldron [2], this was a (universe*), but I've changed my mind again, and prefer (gcc::context*) i.e. it's now a "context" within a "gcc" namespace. The namespace is confusing gengtype, so I'm not sure about that aspect. The pipeline of passes is simply the "passes_" field of the context; here's an example of tab-completion: (gdb) p g->passes_-> Display all 291 possibilities? (y or n) Typing a pass name, and tab-completing to see the 8 instances of it (copy_prop has the most instances): (gdb) p g->passes_->pass_copy_prop_ pass_copy_prop_1 pass_copy_prop_2 pass_copy_prop_3 pass_copy_prop_4 pass_copy_prop_5 pass_copy_prop_6 pass_copy_prop_7 pass_copy_prop_8 and viewing one: (gdb) p g->passes_->pass_copy_prop_1 $2 = (opt_pass *) 0x1588940 (gdb) p *g->passes_->pass_copy_prop_1 $3 = { = {type = GIMPLE_PASS, name = 0xef7fe7 "copyprop", optinfo_flags = 0, has_gate = true, has_execute = true, tv_id = TV_TREE_COPY_PROP, properties_required = 40, properties_provided = 0, properties_destroyed = 0, todo_flags_start = 524288, todo_flags_finish = 2084}, _vptr.opt_pass = 0xef8410, sub = 0x0, next = 0x15889a0, static_pass_number = 30, ctxt_ = 0x15652a0} and here's a view of the top of the struct showing some of the other globals that I've moved to there: (gdb) p *g->passes_ $4 = {all_passes = 0x15896f0, all_small_ipa_passes = 0x1588280, all_lowering_passes = 0x157e000, all_regular_ipa_passes = 0x1589060, all_lto_gen_passes = 0x1589530, all_late_ipa_passes = 0x1589690, passes_by_id = 0x15a9df0, passes_by_id_size = 251, pass_lists = {0x1587590, 0x1587588, 0x1587598, 0x15875a0, 0x1587580}, ctxt_ = 0x15652a0, pass_warn_unused_result = 0x157e000, pass_diagnose_omp_blocks = 0x157e060, (...etc) Note that we do need access to certain specific passes from various places in the compiler. For example, config/i386/i386.c invokes pass_mode_switching within its target-specific "vzeroupper" pass. Hence it's useful to be able to store the pass and access it - so it's not just about debugging the pass creation. In my local copy I'm doing this within the relevant place in i386.c: g->get_passes ().execute_pass_mode_switching (); The only passes I've found so far needing this are these, with these methods in my local copy: class pipeline { public: /* ...snip...*/ /* Access to two specific passes, so that the majority can be private. */ void execute_early_local_passes (); // ^^ used in 3 places in cgraphunit.c unsigned int execute_pass_mode_switching (); // ^^ used by i386.c /* ...snip...*/ private: /* Macros using passes.def to supply fields for the various pass instances; edited for clarity; the other macros have empty expansions. */ #define NEXT_PASS(PASS) opt_pass *PASS #define NEXT_PASS_NUM(PASS, NUM) opt_pass *PASS ## _ ## NUM #include "passes.def" }; // class pipeline I'm sorry that this may not make sense without seeing the relevant patches. I'm currently blocked by gengtype issues with putting things into a "gcc" namespace; if I don't make progress on that, I guess I can get rid of the namespace and bootstrap and post what I've got. Hope this is helpful. Dave [1] so "g" can stand for either "the global", "gcc", or "gnu" as people prefer :) [2] https://raw.github.com/davidmalcolm/2013-cauldron-talk/master/2013-cauldron-talk.txt