From: Xinliang David Li <davidxl@google.com>
To: Richard Guenther <richard.guenther@gmail.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
Diego Novillo <dnovillo@google.com>
Subject: Re: Dump before flag
Date: Tue, 14 Jun 2011 16:02:00 -0000 [thread overview]
Message-ID: <BANLkTikXdkQx1=DhRF-TWsLUZf74f5bCpzxRLcf+5tVDgTzxFg@mail.gmail.com> (raw)
In-Reply-To: <BANLkTikEsgnuRH9k1M9zLcUNbXL6n=tACw@mail.gmail.com>
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.
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
next prev parent reply other threads:[~2011-06-14 15:57 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-06-01 20:26 Xinliang David Li
2011-06-01 20:37 ` Richard Guenther
2011-06-01 20:38 ` Xinliang David Li
2011-06-01 21:12 ` Basile Starynkevitch
2011-06-01 21:24 ` Xinliang David Li
2011-06-06 11:12 ` Richard Guenther
2011-06-06 16:21 ` Xinliang David Li
2011-06-07 9:36 ` Richard Guenther
2011-06-07 16:23 ` Xinliang David Li
2011-06-07 16:43 ` Diego Novillo
2011-06-07 16:51 ` Xinliang David Li
2011-06-07 17:01 ` Diego Novillo
2011-06-07 17:07 ` Xinliang David Li
2011-06-07 17:26 ` Diego Novillo
2011-06-07 23:24 ` Xinliang David Li
2011-06-07 23:34 ` Xinliang David Li
2011-06-08 6:44 ` Xinliang David Li
2011-06-08 9:29 ` Richard Guenther
2011-06-08 16:52 ` Xinliang David Li
2011-06-08 17:14 ` Xinliang David Li
2011-06-08 23:15 ` Xinliang David Li
2011-06-09 9:28 ` Richard Guenther
2011-06-09 15:58 ` Xinliang David Li
2011-06-10 9:04 ` Richard Guenther
2011-06-10 16:54 ` Xinliang David Li
2011-06-10 18:49 ` Xinliang David Li
2011-06-14 14:19 ` Richard Guenther
2011-06-14 16:02 ` Xinliang David Li [this message]
2011-06-14 20:05 ` Xinliang David Li
2011-06-14 23:22 ` Xinliang David Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='BANLkTikXdkQx1=DhRF-TWsLUZf74f5bCpzxRLcf+5tVDgTzxFg@mail.gmail.com' \
--to=davidxl@google.com \
--cc=dnovillo@google.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).