Here is one the of follow up patches: support of -before_preparation, -before, -after, -after_cleanup dump flags. The default dumping behavior does not change at all, but if any one of the above flags is specified, the function IR will be dumped into a file with the corresponding suffix. The enhancement is to simplify IR diffing. Bootstrapped and regression tested on x86-64/linux. Ok for trunk? Thanks, David On Tue, Jun 14, 2011 at 12:40 PM, Xinliang David Li wrote: > 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 wrote: >> 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? >>> >>>  } >>> >>> +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.  I 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 = 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 changes are >>>>>>>>>>>> just  removing TODO_dump_func. The major change is in passes.c and >>>>>>>>>>>> tree-pass.h. >>>>>>>>>>>> >>>>>>>>>>>> -fdump-xxx-yyy-start       <-- dump before TODO_start >>>>>>>>>>>> -fdump-xxx-yyy-before    <-- dump before main pass after TODO_pass >>>>>>>>>>>> -fdump-xxx-yyy-after       <-- dump after main pass before TODO_finish >>>>>>>>>>>> -fdump-xxx-yyy-finish      <-- 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 >>>>>>>>>> >>>>>>>>>>> "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).  Splitting that dump(s) to different 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 destroys >>>>>>>>>>> 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).  I guess what would >>>>>>>>>>> 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 confusion. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> +  /* Override dump TODOs.  */ >>>>>>>>>>>>>>> +  if (dump_file && (pass->todo_flags_finish & TODO_dump_func) >>>>>>>>>>>>>>> +      && (dump_flags & TDF_BEFORE)) >>>>>>>>>>>>>>> +    { >>>>>>>>>>>>>>> +      pass->todo_flags_finish &= ~TODO_dump_func; >>>>>>>>>>>>>>> +      pass->todo_flags_start |= TODO_dump_func; >>>>>>>>>>>>>>> +    } >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> and certainly writing to pass is not ok.  And the TDF_BEFORE flag >>>>>>>>>>>>>>> looks misplaced as it controls TODOs, not dumping behavior. >>>>>>>>>>>>>>> Yes, it's a mess right now but the above looks like a hack ontop >>>>>>>>>>>>>>> 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 dumping 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 previous pass, >>>>>>>>>>>>>> or dump all passes which for large files can take very long time. Even >>>>>>>>>>>>>> with all the dumps, you will need to eyeballing to find the previous >>>>>>>>>>>>>> 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.  Currently some passes >>>>>>>>>>>>> do not dump function bodies because they presumably do no IL >>>>>>>>>>>>> modification.  But 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 after todo-finish. >>>>>>>>>>>>> By default we'd want after todo-finish.  When we no longer dump 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. >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >