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 <davi...@google.com> wrote: > On Tue, Jun 14, 2011 at 6:58 AM, Richard Guenther > <richard.guent...@gmail.com> wrote: >> 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. > > 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 <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. >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >> >