On Fri, Jun 10, 2011 at 8:44 PM, Xinliang David Li <davi...@google.com> 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. A changelog is missing as well. Thanks, Richard. > Thanks, > > David > > On Fri, Jun 10, 2011 at 9:22 AM, Xinliang David Li <davi...@google.com> wrote: >> On Fri, Jun 10, 2011 at 1:52 AM, Richard Guenther >> <richard.guent...@gmail.com> wrote: >>> On Thu, Jun 9, 2011 at 5:47 PM, Xinliang David Li <davi...@google.com> >>> 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 >>>> <richard.guent...@gmail.com> wrote: >>>>> On Thu, Jun 9, 2011 at 12:31 AM, Xinliang David Li <davi...@google.com> >>>>> 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 <davi...@google.com> >>>>>> wrote: >>>>>>> On Wed, Jun 8, 2011 at 2:06 AM, Richard Guenther >>>>>>> <richard.guent...@gmail.com> wrote: >>>>>>>> On Wed, Jun 8, 2011 at 1:08 AM, Xinliang David Li <davi...@google.com> >>>>>>>> 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 >>>>>>>>> <richard.guent...@gmail.com> wrote: >>>>>>>>>> On Mon, Jun 6, 2011 at 6:20 PM, Xinliang David Li >>>>>>>>>> <davi...@google.com> 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. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >