From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 27954 invoked by alias); 14 Jun 2011 19:41:17 -0000 Received: (qmail 27946 invoked by uid 22791); 14 Jun 2011 19:41:16 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from smtp-out.google.com (HELO smtp-out.google.com) (74.125.121.67) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 14 Jun 2011 19:40:57 +0000 Received: from kpbe11.cbf.corp.google.com (kpbe11.cbf.corp.google.com [172.25.105.75]) by smtp-out.google.com with ESMTP id p5EJet0x004020 for ; Tue, 14 Jun 2011 12:40:55 -0700 Received: from gxk22 (gxk22.prod.google.com [10.202.11.22]) by kpbe11.cbf.corp.google.com with ESMTP id p5EJejMd023812 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for ; Tue, 14 Jun 2011 12:40:54 -0700 Received: by gxk22 with SMTP id 22so4929384gxk.2 for ; Tue, 14 Jun 2011 12:40:53 -0700 (PDT) MIME-Version: 1.0 Received: by 10.150.59.15 with SMTP id h15mr8145939yba.73.1308080453514; Tue, 14 Jun 2011 12:40:53 -0700 (PDT) Received: by 10.151.26.21 with HTTP; Tue, 14 Jun 2011 12:40:53 -0700 (PDT) In-Reply-To: References: <20110601231202.224188ad.basile@starynkevitch.net> Date: Tue, 14 Jun 2011 20:05:00 -0000 Message-ID: Subject: Re: Dump before flag From: Xinliang David Li To: Richard Guenther Cc: GCC Patches , Diego Novillo Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-System-Of-Record: true 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/msg01103.txt.bz2 Committed after Bootstrapping and regression testing on x86-64/linux. The follow up patch will come soon. Thanks, David On Tue, Jun 14, 2011 at 8:57 AM, Xinliang David Li wro= te: > On Tue, Jun 14, 2011 at 6:58 AM, Richard Guenther > wrote: >> On Fri, Jun 10, 2011 at 8:44 PM, Xinliang David Li = wrote: >>> This is the revised patch as suggested. >>> >>> How does it look? >> >> =A0} >> >> +static void >> +execute_function_dump (void *data ATTRIBUTE_UNUSED) >> >> function needs a comment. >> >> Ok with that change. >> >> Please always specify how you tested the patch - the past fallouts >> suggest you didn't do the required testing carefully. > > I think I did -- the fallout was probably due to different > '--enable-checking' setting. I have now turned it to 'yes' > > Thanks, > > David > >> >> A changelog is missing as well. >> >> Thanks, >> Richard. >> >>> Thanks, >>> >>> David >>> >>> On Fri, Jun 10, 2011 at 9:22 AM, Xinliang David Li = wrote: >>>> On Fri, Jun 10, 2011 at 1:52 AM, Richard Guenther >>>> wrote: >>>>> On Thu, Jun 9, 2011 at 5:47 PM, Xinliang David Li wrote: >>>>>> See attached. >>>>> >>>>> Hmm. =A0I don't like how you still wire dumping in the TODO routines. >>>>> Doesn't it work to just dump the body from pass_fini_dump_file ()? >>>>> Or if that doesn't sound clean from (a subset of) places where it >>>>> is called? (we might want to exclude the ipa read/write/summary >>>>> stages) >>>> >>>> That may require another round of function traversal -- but probably >>>> not a big deal -- it sounds cleaner. >>>> >>>> David >>>> >>>>> >>>>> Richard. >>>>> >>>>>> Thanks, >>>>>> >>>>>> David >>>>>> >>>>>> On Thu, Jun 9, 2011 at 2:02 AM, Richard Guenther >>>>>> wrote: >>>>>>> On Thu, Jun 9, 2011 at 12:31 AM, Xinliang David Li wrote: >>>>>>>> this is the patch that just removes the TODO_dump flag and forces = it >>>>>>>> to dump. The original code cfun->last_verified =3D flags & >>>>>>>> TODO_verify_all looks weird -- depending on TODO_dump is set or no= t, >>>>>>>> the behavior of the update is different (when no other todo flags = is >>>>>>>> set). >>>>>>>> >>>>>>>> Ok for trunk? >>>>>>> >>>>>>> -ENOPATCH. >>>>>>> >>>>>>> Richard. >>>>>>> >>>>>>>> David >>>>>>>> >>>>>>>> On Wed, Jun 8, 2011 at 9:52 AM, Xinliang David Li wrote: >>>>>>>>> On Wed, Jun 8, 2011 at 2:06 AM, Richard Guenther >>>>>>>>> wrote: >>>>>>>>>> On Wed, Jun 8, 2011 at 1:08 AM, Xinliang David Li wrote: >>>>>>>>>>> The following is the patch that does the job. Most of the chang= es are >>>>>>>>>>> just =A0removing TODO_dump_func. The major change is in passes.= c and >>>>>>>>>>> tree-pass.h. >>>>>>>>>>> >>>>>>>>>>> -fdump-xxx-yyy-start =A0 =A0 =A0 <-- dump before TODO_start >>>>>>>>>>> -fdump-xxx-yyy-before =A0 =A0<-- dump before main pass after TO= DO_pass >>>>>>>>>>> -fdump-xxx-yyy-after =A0 =A0 =A0 <-- dump after main pass befor= e TODO_finish >>>>>>>>>>> -fdump-xxx-yyy-finish =A0 =A0 =A0<-- dump after TODO_finish >>>>>>>>>> >>>>>>>>>> Can we bikeshed a bit more about these names? >>>>>>>>> >>>>>>>>> These names may be less confusing: >>>>>>>>> >>>>>>>>> before_preparation >>>>>>>>> before >>>>>>>>> after >>>>>>>>> after_cleanup >>>>>>>>> >>>>>>>>> David >>>>>>>>> >>>>>>>>>>=A0"start" and "before" >>>>>>>>>> have no semantical difference to me ... as the dump before TODO_= start >>>>>>>>>> of a pass and the dump after TODO_finish of the previous pass are >>>>>>>>>> identical (hopefully ;)), maybe merge those into a -between flag? >>>>>>>>>> If you'd specify it for a single pass then you'd get both -start= and -finish >>>>>>>>>> (using your naming scheme). =A0Splitting that dump(s) to differe= nt files >>>>>>>>>> then might make sense (not sure about the name to use). >>>>>>>>>> >>>>>>>>>> Note that I find it extremely useful to have dumping done in >>>>>>>>>> chronological order - splitting some of it to different files de= stroys >>>>>>>>>> this, especially a dump after TODO_start or before TODO_finish >>>>>>>>>> should appear in the same file (or we could also start splitting >>>>>>>>>> individual TODO_ output into sub-dump-files). =A0I guess what wo= uld >>>>>>>>>> be nice instread would be a fancy dump-file viewer that could >>>>>>>>>> show diffs, hide things like SCEV output, etc. >>>>>>>>>> >>>>>>>>>> I suppose a patch that removes the dump TODO and unconditionally >>>>>>>>>> dumps at the current point would be a good preparation for this >>>>>>>>>> enhancing patch. >>>>>>>>>> >>>>>>>>>> Richard. >>>>>>>>>> >>>>>>>>>>> The default is 'finish'. >>>>>>>>>>> >>>>>>>>>>> Does it look ok? >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> >>>>>>>>>>> David >>>>>>>>>>> >>>>>>>>>>> On Tue, Jun 7, 2011 at 2:36 AM, Richard Guenther >>>>>>>>>>> wrote: >>>>>>>>>>>> On Mon, Jun 6, 2011 at 6:20 PM, Xinliang David Li wrote: >>>>>>>>>>>>>> >>>>>>>>>>>>>> Your patch doesn't really improve this but adds to the confu= sion. >>>>>>>>>>>>>> >>>>>>>>>>>>>> + =A0/* Override dump TODOs. =A0*/ >>>>>>>>>>>>>> + =A0if (dump_file && (pass->todo_flags_finish & TODO_dump_f= unc) >>>>>>>>>>>>>> + =A0 =A0 =A0&& (dump_flags & TDF_BEFORE)) >>>>>>>>>>>>>> + =A0 =A0{ >>>>>>>>>>>>>> + =A0 =A0 =A0pass->todo_flags_finish &=3D ~TODO_dump_func; >>>>>>>>>>>>>> + =A0 =A0 =A0pass->todo_flags_start |=3D TODO_dump_func; >>>>>>>>>>>>>> + =A0 =A0} >>>>>>>>>>>>>> >>>>>>>>>>>>>> and certainly writing to pass is not ok. =A0And the TDF_BEFO= RE flag >>>>>>>>>>>>>> looks misplaced as it controls TODOs, not dumping behavior. >>>>>>>>>>>>>> Yes, it's a mess right now but the above looks like a hack o= ntop >>>>>>>>>>>>>> of that mess (maybe because of it, but well ...). >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> How about removing dumping TODO completely -- this can be don= e easily >>>>>>>>>>>>> -- I don't understand why pass wants extra control on the dum= ping if >>>>>>>>>>>>> user already asked for dumping -- it is annoying to see empty= IR dump >>>>>>>>>>>>> for a pass when I want to see it. >>>>>>>>>>>>> >>>>>>>>>>>>>> At least I would have expected to also get the dump after the >>>>>>>>>>>>>> pass, not only the one before it with this dump flag. >>>>>>>>>>>>>> >>>>>>>>>>>>>> Now, why can't you look at the previous pass output for the >>>>>>>>>>>>>> before-dump (as I do usually)? >>>>>>>>>>>>> >>>>>>>>>>>>> For one thing, you need to either remember what is the previo= us pass, >>>>>>>>>>>>> or dump all passes which for large files can take very long t= ime. Even >>>>>>>>>>>>> with all the dumps, you will need to eyeballing to find the p= revious >>>>>>>>>>>>> pass which may or may not have the IR dumped. >>>>>>>>>>>>> >>>>>>>>>>>>> How about removing dump TODO? >>>>>>>>>>>> >>>>>>>>>>>> Yeah, I think this would go in the right direction. =A0Current= ly some passes >>>>>>>>>>>> do not dump function bodies because they presumably do no IL >>>>>>>>>>>> modification. =A0But this is certainly the minority (and some = passes do not >>>>>>>>>>>> dump bodies even though they are modifying the IL ...). >>>>>>>>>>>> >>>>>>>>>>>> So I'd say we should by default dump function bodies. >>>>>>>>>>>> >>>>>>>>>>>> Note that there are three useful dumping positions (maybe four= ), >>>>>>>>>>>> before todo-start, after todo-start, before todo-finish and af= ter todo-finish. >>>>>>>>>>>> By default we'd want after todo-finish. =A0When we no longer d= ump via >>>>>>>>>>>> a TODO then we could indeed use dump-flags to control this >>>>>>>>>>>> (maybe -original for the body before todo-start). >>>>>>>>>>>> >>>>>>>>>>>> What to others think? >>>>>>>>>>>> >>>>>>>>>>>> Richard. >>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> >>>>>>>>>>>>> David >>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> Richard. >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >