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