From patchwork Thu May 26 09:04:28 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 97509 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id B380EB6F8C for ; Thu, 26 May 2011 19:04:52 +1000 (EST) Received: (qmail 27942 invoked by alias); 26 May 2011 09:04:50 -0000 Received: (qmail 27929 invoked by uid 22791); 26 May 2011 09:04:48 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, RFC_ABUSE_POST X-Spam-Check-By: sourceware.org Received: from mail-wy0-f175.google.com (HELO mail-wy0-f175.google.com) (74.125.82.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 26 May 2011 09:04:31 +0000 Received: by wye20 with SMTP id 20so386097wye.20 for ; Thu, 26 May 2011 02:04:29 -0700 (PDT) MIME-Version: 1.0 Received: by 10.227.152.132 with SMTP id g4mr573527wbw.24.1306400669072; Thu, 26 May 2011 02:04:29 -0700 (PDT) Received: by 10.227.37.152 with HTTP; Thu, 26 May 2011 02:04:28 -0700 (PDT) In-Reply-To: References: <20110518183708.D5E5A207D9@syzygy.mtv.corp.google.com> Date: Thu, 26 May 2011 11:04:28 +0200 Message-ID: Subject: Re: New options to disable/enable any pass for any functions (issue4550056) From: Richard Guenther To: "Joseph S. Myers" Cc: Xinliang David Li , reply@codereview.appspotmail.com, GCC Patches X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers wrote: > On Wed, 25 May 2011, Xinliang David Li wrote: > >> Ping. The link to the message: >> >> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html > > I don't consider this an option handling patch.  Patches adding whole new > features involving new options should be reviewed by maintainers for the > part of the compiler relevant to those features (since there isn't a pass > manager maintainer, I guess that means middle-end). Hmm, I suppose then you reviewed the option handling parts and they are ok? Those globbing options always cause headache to me. +-fenable-ipa-@var{pass} @gol +-fenable-rtl-@var{pass} @gol +-fenable-rtl-@var{pass}=@var{range-list} @gol +-fenable-tree-@var{pass} @gol so, no -fenable-tree-@var{pass}=@var{range-list}? Does the pass name match 1:1 with the dump file name? In which case +Disable ipa pass @var{pass}. @var{pass} is the pass name. If the same pass is statically invoked in the compiler multiple times, the pass name should be appended with a sequential number starting from 1. isn't true as passes that are invoked only a single time lack the number suffix (yes, I'd really like that to be changed ...) Please break likes also in .texi files and stick to 80 columns. Please document that these options are debugging options and regular options for enabling/disabling passes should be used. I would suggest to group documentation differently and document -fenable-* and -fdisable-*, thus, + -fdisable-@var{kind}-@var{pass} + -fenable-@var{kind}-@var{pass} Even in .texi files please two spaces after a full-stop. +extern void enable_disable_pass (const char *, bool); I'd rather have both enable_pass and disable_pass ;) +struct function; +extern void pass_dump_function_header (FILE *, tree, struct function *); that's odd and probably should be split out, the function should maybe reside in tree-pretty-print.c. wrappers. +void +register_pass_name (struct opt_pass *pass, const char *name) doesn't seem to need exporting, so don't and make it static. + if (!pass_name_tab) + pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL); see above, the initial size should be larger - we have 223 passes at the moment, so use 256. + else + return; /* Ignore plugin passes. */ ? You mean they might clash? +struct opt_pass * +get_pass_by_name (const char *name) doesn't need exporting either. + if (is_enable) + error ("unrecognized option -fenable"); + else + error ("unrecognized option -fdisable"); I think that should be fatal_error - Joseph? + if (is_enable) + error ("unknown pass %s specified in -fenable", phase_name); + else + error ("unknown pass %s specified in -fdisble", phase_name); likewise. + if (!enabled_pass_uid_range_tab) + enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL); instead of using a hashtable here please use a VEC indexed by the static_pass_number which shoud speed up +static bool +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass, + tree func, htab_t tab) +{ + struct uid_range **slot, *range, key; + int cgraph_uid; + + if (!tab) + return false; + + key.pass = pass; + slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT); + if (!slot || !*slot) + return false; and simplify the code quite a bit. + cgraph_uid = func ? cgraph_get_node (func)->uid : 0; note that cgraph uids are recycled, so it might not be the best idea to use them as discriminator (though I don't have a good idea how to represent ranges without them). + explicitly_enabled = is_pass_explicitly_enabled (pass, current_function_decl); + explicitly_disabled = is_pass_explicitly_disabled (pass, current_function_decl); + current_pass = pass; /* Check whether gate check should be avoided. User controls the value of the gate through the parameter "gate_status". */ gate_status = (pass->gate == NULL) ? true : pass->gate(); + gate_status = !explicitly_disabled && (gate_status || explicitly_enabled); so explicitly disabling wins over explicit enabling ;) I think this implementation detail and the fact that you always query both hints at that the interface should be like gate_status = override_gate_status (pass, current_function_decl, gate_status); instead. Thus, please split out the function header dumping changes and rework the rest of the patch as suggested. Thanks, Richard. > -- > Joseph S. Myers > joseph@codesourcery.com > Index: tree-ssa-loop-ivopts.c =================================================================== --- tree-ssa-loop-ivopts.c (revision 173837) +++ tree-ssa-loop-ivopts.c (working copy) @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d well - doesn't belong here ;) +static hashval_t +passr_hash (const void *p) +{ + const struct pass_registry *const s = (const struct pass_registry *const) p; + return htab_hash_string (s->unique_name); +} + +/* Hash equal function */ + +static int +passr_eq (const void *p1, const void *p2) +{ + const struct pass_registry *const s1 = (const struct pass_registry *const) p1; + const struct pass_registry *const s2 = (const struct pass_registry *const) p2; + + return !strcmp (s1->unique_name, s2->unique_name); +} you can use htab_hash_string and strcmp directly, no need for these