Re: Dump before flag

[Xinliang David Li <davidxl@google.com>]

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 <davidxl@google.com> wrote:
> On Tue, Jun 14, 2011 at 6:58 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Fri, Jun 10, 2011 at 8:44 PM, Xinliang David Li <davidxl@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 <davidxl@google.com> wrote:
>>>> On Fri, Jun 10, 2011 at 1:52 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Thu, Jun 9, 2011 at 5:47 PM, Xinliang David Li <davidxl@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.guenther@gmail.com> wrote:
>>>>>>> On Thu, Jun 9, 2011 at 12:31 AM, Xinliang David Li <davidxl@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 <davidxl@google.com> wrote:
>>>>>>>>> On Wed, Jun 8, 2011 at 2:06 AM, Richard Guenther
>>>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>>>> On Wed, Jun 8, 2011 at 1:08 AM, Xinliang David Li <davidxl@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.guenther@gmail.com> wrote:
>>>>>>>>>>>> On Mon, Jun 6, 2011 at 6:20 PM, Xinliang David Li <davidxl@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.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>