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. >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >