From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31677 invoked by alias); 8 Jun 2011 08:41:39 -0000 Received: (qmail 31666 invoked by uid 22791); 8 Jun 2011 08:41:38 -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,TW_FN,TW_TM X-Spam-Check-By: sourceware.org Received: from mail-ww0-f51.google.com (HELO mail-ww0-f51.google.com) (74.125.82.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 08 Jun 2011 08:41:24 +0000 Received: by wwf26 with SMTP id 26so227601wwf.8 for ; Wed, 08 Jun 2011 01:41:22 -0700 (PDT) MIME-Version: 1.0 Received: by 10.227.169.139 with SMTP id z11mr780060wby.60.1307522482671; Wed, 08 Jun 2011 01:41:22 -0700 (PDT) Received: by 10.227.37.152 with HTTP; Wed, 8 Jun 2011 01:41:22 -0700 (PDT) In-Reply-To: References: Date: Wed, 08 Jun 2011 08:54:00 -0000 Message-ID: Subject: Re: -fdump-passes -fenable-xxx=func_name_list From: Richard Guenther To: Xinliang David Li Cc: GCC Patches Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes 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 X-SW-Source: 2011-06/txt/msg00620.txt.bz2 On Tue, Jun 7, 2011 at 8:54 PM, Xinliang David Li wrot= e: > Please review the attached two patches. > > In the first patch, gate functions are cleaned up. All the per > function legality checks are moved into the executor and the > optimization heuristic checks (optimize for size) remain in the > gators. These allow the the following overriding order: > > =A0 =A0common flags (O2, -ftree-vrp, -fgcse etc) =A0 <--- =A0compiler > heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass > options =A0<--- legality check > > Testing under going. Ok for trunk? It's somewhat odd that you keep the optimize for size/speed checks in the gate - yes, they also work with a NULL cfun, returning a "default" (previously they were just optimize_size checks). So probably it makes sense to keep those in the gates. I notice that with the patch we will now unconditionally execute TODOs for those passes that were disabled per function which will eventually cause some compile-time regression, mostly restricted to checking enabled builds (which is ok I guess). I suppose we could allow the execute function to return a special value that says "I really didn't do anything". OTOH most of the pass machinery needs some cleanup anyway which can be done as followup. The gate cleanup patch looks ok to me, please give others a chance to comment on this change. Thanks, Richard. > Thanks, > > David > > On Tue, Jun 7, 2011 at 9:24 AM, Xinliang David Li wr= ote: >> Ok -- that sounds good. >> >> David >> >> On Tue, Jun 7, 2011 at 3:10 AM, Richard Guenther >> wrote: >>> On Mon, Jun 6, 2011 at 6:00 PM, Xinliang David Li = wrote: >>>> On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther >>>> wrote: >>>>> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li wrote: >>>>>> This is the version of the patch that walks through pass lists. >>>>>> >>>>>> Ok with this one? >>>>> >>>>> +/* Dump all optimization passes. =A0*/ >>>>> + >>>>> +void >>>>> +dump_passes (void) >>>>> +{ >>>>> + =A0struct cgraph_node *n, *node =3D NULL; >>>>> + =A0tree save_fndecl =3D current_function_decl; >>>>> + >>>>> + =A0fprintf (stderr, "MAX_UID =3D %d\n", cgraph_max_uid); >>>>> >>>>> this isn't accurate info - cloning can cause more cgraph nodes to >>>>> appear (it also looks completely unrelated to dump_passes ...). >>>>> Please drop it. >>>> >>>> Ok. >>>> >>>> >>>>> >>>>> + =A0create_pass_tab(); >>>>> + =A0gcc_assert (pass_tab); >>>>> >>>>> you have quite many asserts of this kind - we don't want them when >>>>> the previous stmt as in this case indicates everything is ok. >>>> >>>> ok. >>>> >>>>> >>>>> + =A0push_cfun (DECL_STRUCT_FUNCTION (node->decl)); >>>>> >>>>> this has side-effects, I'm not sure we want this here. =A0Why do you >>>>> need it? =A0Probably because of >>>>> >>>>> + =A0is_really_on =3D override_gate_status (pass, current_function_de= cl, is_on); >>>>> >>>>> ? =A0But that is dependent on the function given which should have no >>>>> effect (unless it is overridden globally in which case override_gate_= status >>>>> and friends should deal with a NULL cfun). >>>> >>>> As we discussed, currently some pass gate functions depend on per node >>>> information -- those checks need to be pushed into execute functions. >>>> I would like to clean those up later -- at which time, the push/pop >>>> can be removed. >>> >>> I'd like to do it the other way around, first clean up the gate functio= ns then >>> drop in this patch without the cfun push/pop. =A0The revised patch look= s ok >>> to me with the cfun push/pop removed. >>> >>> Thanks, >>> Richard. >>> >>>>> >>>>> I don't understand why you need another table mapping pass to name >>>>> when pass->name is available and the info is trivially re-constructib= le. >>>> >>>> This is needed as the pass->name is not the canonicalized name (i.e., >>>> not with number suffix etc), so the extra mapping from id to >>>> normalized name is needed. >>>> >>>> Thanks, >>>> >>>> David >>>> >>>>> >>>>> Thanks, >>>>> Richard. >>>>> >>>>>> David >>>>>> >>>>>> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li wrote: >>>>>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther >>>>>>> wrote: >>>>>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li wrote: >>>>>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther >>>>>>>>> wrote: >>>>>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li wrote: >>>>>>>>>>> The following patch implements the a new option that dumps gcc = PASS >>>>>>>>>>> configuration. The sample output is attached. =A0There is one >>>>>>>>>>> limitation: some placeholder passes that are named with '*xxx' = are >>>>>>>>>>> note registered thus they are not listed. They are not importan= t as >>>>>>>>>>> they can not be turned on/off anyway. >>>>>>>>>>> >>>>>>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow = a list >>>>>>>>>>> of function assembler names to be specified. >>>>>>>>>>> >>>>>>>>>>> Ok for trunk? >>>>>>>>>> >>>>>>>>>> Please split the patch. >>>>>>>>>> >>>>>>>>>> I'm not too happy how you dump the pass configuration. =A0Why no= t simply, >>>>>>>>>> at a _single_ place, walk the pass tree? =A0Instead of doing pie= ces of it >>>>>>>>>> at pass execution time when it's not already dumped - that reall= y looks >>>>>>>>>> gross. >>>>>>>>> >>>>>>>>> Yes, that was the original plan -- but it has problems >>>>>>>>> 1) the dumper needs to know the root pass lists -- which can chan= ge >>>>>>>>> frequently -- it can be a long term maintanance burden; >>>>>>>>> 2) the centralized dumper needs to be done after option processing >>>>>>>>> 3) not sure if gate functions have any side effects or have depen= dencies on cfun >>>>>>>>> >>>>>>>>> The proposed solutions IMHO is not that intrusive -- just three h= ooks >>>>>>>>> to do the dumping and tracking indentation. >>>>>>>> >>>>>>>> Well, if you have a CU that is empty or optimized to nothing at so= me point >>>>>>>> you will not get a complete pass list. =A0I suppose optimize attri= butes might >>>>>>>> also confuse output. =A0Your solution might not be that intrusive >>>>>>>> but it is still ugly. =A0I don't see 1) as an issue, for 2) you ca= n just call the >>>>>>>> dumping from toplev_main before calling do_compile (), 3) gate fun= ctions >>>>>>>> shouldn't have side-effects, but as they could gate on optimize_fo= r_speed () >>>>>>>> your option summary output will be bogus anyway. >>>>>>>> >>>>>>>> So - what is the output intended for if it isn't reliable? >>>>>>> >>>>>>> This needs to be cleaned up at some point -- the gate function shou= ld >>>>>>> behave the same for all functions and per-function decisions need to >>>>>>> be pushed down to the executor body. =A0I will try to rework the pa= tch >>>>>>> as you suggested to see if there are problems. >>>>>>> >>>>>>> David >>>>>>> >>>>>>> >>>>>>>> >>>>>>>> Richard. >>>>>>>> >>>>>>>>>> >>>>>>>>>> The documentation should also link this option to the -fenable/d= isable >>>>>>>>>> options as obviously the pass names in that dump are those to be >>>>>>>>>> used for those flags (and not readily available anywhere else). >>>>>>>>> >>>>>>>>> Ok. >>>>>>>>> >>>>>>>>>> >>>>>>>>>> I also think that it would be way more useful to note in the ind= ividual >>>>>>>>>> dump files the functions (at the place they would usually appear= ) that >>>>>>>>>> have the pass explicitly enabled/disabled. >>>>>>>>> >>>>>>>>> Ok -- for ipa passes or tree/rtl passes where all functions are >>>>>>>>> explicitly disabled. >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> >>>>>>>>> David >>>>>>>>> >>>>>>>>>> >>>>>>>>>> Richard. >>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> >>>>>>>>>>> David >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >