From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6052 invoked by alias); 14 Jun 2011 15:57:46 -0000 Received: (qmail 6033 invoked by uid 22791); 14 Jun 2011 15:57:44 -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 15:57:29 +0000 Received: from hpaq2.eem.corp.google.com (hpaq2.eem.corp.google.com [172.25.149.2]) by smtp-out.google.com with ESMTP id p5EFvOXX027376 for ; Tue, 14 Jun 2011 08:57:26 -0700 Received: from gxk21 (gxk21.prod.google.com [10.202.11.21]) by hpaq2.eem.corp.google.com with ESMTP id p5EFubVW002533 (version=TLSv1/SSLv3 cipher=RC4-SHA bits=128 verify=NOT) for ; Tue, 14 Jun 2011 08:57:21 -0700 Received: by gxk21 with SMTP id 21so4981607gxk.33 for ; Tue, 14 Jun 2011 08:57:21 -0700 (PDT) MIME-Version: 1.0 Received: by 10.150.170.2 with SMTP id s2mr8606449ybe.16.1308067040597; Tue, 14 Jun 2011 08:57:20 -0700 (PDT) Received: by 10.151.26.21 with HTTP; Tue, 14 Jun 2011 08:57:20 -0700 (PDT) In-Reply-To: References: <20110601231202.224188ad.basile@starynkevitch.net> Date: Tue, 14 Jun 2011 16:02: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/msg01072.txt.bz2 On Tue, Jun 14, 2011 at 6:58 AM, Richard Guenther wrote: > On Fri, Jun 10, 2011 at 8:44 PM, Xinliang David Li w= rote: >> 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 not, >>>>>>> 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 change= s 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 TOD= O_pass >>>>>>>>>> -fdump-xxx-yyy-after =A0 =A0 =A0 <-- dump after main pass before= 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_s= tart >>>>>>>>> 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 differen= t 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 des= troys >>>>>>>>> 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 wou= ld >>>>>>>>> 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 confus= ion. >>>>>>>>>>>>> >>>>>>>>>>>>> + =A0/* Override dump TODOs. =A0*/ >>>>>>>>>>>>> + =A0if (dump_file && (pass->todo_flags_finish & TODO_dump_fu= nc) >>>>>>>>>>>>> + =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_BEFOR= E flag >>>>>>>>>>>>> looks misplaced as it controls TODOs, not dumping behavior. >>>>>>>>>>>>> Yes, it's a mess right now but the above looks like a hack on= top >>>>>>>>>>>>> of that mess (maybe because of it, but well ...). >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> How about removing dumping TODO completely -- this can be done= easily >>>>>>>>>>>> -- I don't understand why pass wants extra control on the dump= ing 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 previou= s pass, >>>>>>>>>>>> or dump all passes which for large files can take very long ti= me. Even >>>>>>>>>>>> with all the dumps, you will need to eyeballing to find the pr= evious >>>>>>>>>>>> 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. =A0Currentl= y some passes >>>>>>>>>>> do not dump function bodies because they presumably do no IL >>>>>>>>>>> modification. =A0But this is certainly the minority (and some p= asses 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 aft= er todo-finish. >>>>>>>>>>> By default we'd want after todo-finish. =A0When we no longer du= mp 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. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >