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 23:22:00 -0000 [thread overview]
Message-ID: <BANLkTikcP9Q4cUcOdTXHszZHA2+bPgGYQW3ExJfQCh7SfhhrgA@mail.gmail.com> (raw)
In-Reply-To: <BANLkTi=MnrSxdYN0P+iqr+bUkwu=ZmxA9MZYBvbkqEqsK31mrQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 8910 bytes --]
Here is one the of follow up patches: support of -before_preparation,
-before, -after, -after_cleanup dump flags.
The default dumping behavior does not change at all, but if any one of
the above flags is specified, the function IR will be dumped into a
file with the corresponding suffix. The enhancement is to simplify IR
diffing.
Bootstrapped and regression tested on x86-64/linux.
Ok for trunk?
Thanks,
David
On Tue, Jun 14, 2011 at 12:40 PM, Xinliang David Li <davidxl@google.com> wrote:
> 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.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
[-- Attachment #2: dump-where.p --]
[-- Type: text/x-pascal, Size: 8212 bytes --]
2011-06-14 David Li <davidxl@google.com>
* passes.c
(do_per_function_toporder): Support for dump splitting.
(execute_function_dump): Support for dump splitting.
(pass_init_dump_file): Support for dump splitting.
(execute_one_ipa_transform_pass): Support for dump splitting.
(execute_one_pass): Support for dump splitting.
Index: tree-dump.c
===================================================================
--- tree-dump.c (revision 175032)
+++ tree-dump.c (working copy)
@@ -822,6 +822,10 @@ static const struct dump_option_value_in
{"eh", TDF_EH},
{"alias", TDF_ALIAS},
{"nouid", TDF_NOUID},
+ {"before_preparation", TDF_START},
+ {"before", TDF_BEFORE},
+ {"after", TDF_AFTER},
+ {"after_cleanup", TDF_FINISH},
{"enumerate_locals", TDF_ENUMERATE_LOCALS},
{"all", ~(TDF_RAW | TDF_SLIM | TDF_LINENO | TDF_TREE | TDF_RTL | TDF_IPA
| TDF_STMTADDR | TDF_GRAPH | TDF_DIAGNOSTIC | TDF_VERBOSE
Index: tree-pass.h
===================================================================
--- tree-pass.h (revision 175032)
+++ tree-pass.h (working copy)
@@ -83,6 +83,11 @@ enum tree_dump_index
#define TDF_ALIAS (1 << 21) /* display alias information */
#define TDF_ENUMERATE_LOCALS (1 << 22) /* Enumerate locals by uid. */
#define TDF_CSELIB (1 << 23) /* Dump cselib details. */
+#define TDF_START (1 << 24) /* Dump before TODO_start. */
+#define TDF_BEFORE (1 << 25) /* Dump before pass. */
+#define TDF_AFTER (1 << 26) /* Dump after pass. */
+#define TDF_FINISH (1 << 27) /* Dump after TODO_finish. */
+
/* In tree-dump.c */
Index: passes.c
===================================================================
--- passes.c (revision 175051)
+++ passes.c (working copy)
@@ -128,6 +128,7 @@ int dump_flags;
bool in_gimple_form;
bool first_pass_instance;
+static FILE *dump_file_start, *dump_file_before, *dump_file_after, *dump_file_finish;
/* This is called from various places for FUNCTION_DECL, VAR_DECL,
and TYPE_DECL nodes.
@@ -1605,21 +1606,23 @@ do_per_function_toporder (void (*callbac
/* Helper function to perform function body dump. */
static void
-execute_function_dump (void *data ATTRIBUTE_UNUSED)
+execute_function_dump (void *data)
{
- if (dump_file && current_function_decl)
+ FILE *df = (FILE *)data;
+
+ if (df && current_function_decl)
{
if (cfun->curr_properties & PROP_trees)
- dump_function_to_file (current_function_decl, dump_file, dump_flags);
+ dump_function_to_file (current_function_decl, df, dump_flags);
else
{
if (dump_flags & TDF_SLIM)
- print_rtl_slim_with_bb (dump_file, get_insns (), dump_flags);
+ print_rtl_slim_with_bb (df, get_insns (), dump_flags);
else if ((cfun->curr_properties & PROP_cfg)
&& (dump_flags & TDF_BLOCKS))
- print_rtl_with_bb (dump_file, get_insns ());
+ print_rtl_with_bb (df, get_insns ());
else
- print_rtl (dump_file, get_insns ());
+ print_rtl (df, get_insns ());
if ((cfun->curr_properties & PROP_cfg)
&& graph_dump_format != no_graph
@@ -1629,7 +1632,7 @@ execute_function_dump (void *data ATTRIB
/* Flush the file. If verification fails, we won't be able to
close the file before aborting. */
- fflush (dump_file);
+ fflush (df);
}
}
@@ -1784,6 +1787,50 @@ verify_curr_properties (void *data)
}
#endif
+
+/* Helper function to set up file descriptors for IR dumps. WHERE_FLAG
+ is the flag indicating where the IR come from; SUFFIX is the file suffix
+ of the dump file, and MODE is the file open mode. */
+
+static FILE *
+init_ir_dump_file (int where_flag, const char *suffix, const char *mode)
+{
+ if (dump_flags & where_flag)
+ {
+ char *dump_name = concat (dump_file_name, suffix, NULL);
+ FILE *stream = fopen (dump_name, mode);
+ if (!stream)
+ error ("could not open dump file %qs: %m", dump_name);
+ free (dump_name);
+ if (current_function_decl)
+ dump_function_header (stream, current_function_decl, dump_flags);
+ return stream;
+ }
+ else
+ return dump_file;
+}
+
+/* Helper function to set up file descriptors for IR dumps. */
+
+static void
+pass_init_ir_dump_files (struct opt_pass *pass)
+{
+ struct dump_file_info *dfi;
+ const char *mode;
+
+ if (!dump_file)
+ return;
+
+ dfi = get_dump_file_info (pass->static_pass_number);
+ mode = dfi->state < 0 ? "w" : "a";
+
+ dump_file_start = init_ir_dump_file (TDF_START, ".before_preparation", mode);
+ dump_file_before = init_ir_dump_file (TDF_BEFORE, ".before", mode);
+ dump_file_after = init_ir_dump_file (TDF_AFTER, ".after", mode);
+ dump_file_finish = init_ir_dump_file (TDF_FINISH, ".after_cleanup", mode);
+}
+
+
/* Initialize pass dump file. */
/* This is non-static so that the plugins can use it. */
@@ -1798,12 +1845,35 @@ pass_init_dump_file (struct opt_pass *pa
dump_file = dump_begin (pass->static_pass_number, &dump_flags);
if (dump_file && current_function_decl)
dump_function_header (dump_file, current_function_decl, dump_flags);
+
+ if (dump_file)
+ pass_init_ir_dump_files (pass);
+
return initializing_dump;
}
else
return false;
}
+/* Close dump files. */
+
+static void
+close_ir_dump_file (void)
+{
+ if (dump_file_start != dump_file)
+ fclose (dump_file_start);
+ dump_file_start = NULL;
+ if (dump_file_before != dump_file)
+ fclose (dump_file_before);
+ dump_file_before = NULL;
+ if (dump_file_after != dump_file)
+ fclose (dump_file_after);
+ dump_file_after = NULL;
+ if (dump_file_finish != dump_file)
+ fclose (dump_file_finish);
+ dump_file_finish = NULL;
+}
+
/* Flush PASS dump file. */
/* This is non-static so that plugins can use it. */
@@ -1811,6 +1881,7 @@ void
pass_fini_dump_file (struct opt_pass *pass)
{
/* Flush and close dump file. */
+ close_ir_dump_file ();
if (dump_file_name)
{
free (CONST_CAST (char *, dump_file_name));
@@ -1886,9 +1957,15 @@ execute_one_ipa_transform_pass (struct c
pass_init_dump_file (pass);
+ if (dump_file && (dump_flags & TDF_START))
+ do_per_function (execute_function_dump, dump_file_start);
+
/* Run pre-pass verification. */
execute_todo (ipa_pass->function_transform_todo_flags_start);
+ if (dump_file && (dump_flags & TDF_BEFORE))
+ do_per_function (execute_function_dump, dump_file_before);
+
/* If a timevar is present, start it. */
if (pass->tv_id != TV_NONE)
timevar_push (pass->tv_id);
@@ -1900,11 +1977,15 @@ execute_one_ipa_transform_pass (struct c
if (pass->tv_id != TV_NONE)
timevar_pop (pass->tv_id);
+ if (dump_file && (dump_flags & TDF_AFTER))
+ do_per_function (execute_function_dump, dump_file_after);
+
/* Run post-pass cleanup and verification. */
execute_todo (todo_after);
verify_interpass_invariants ();
- do_per_function (execute_function_dump, NULL);
+ if (dump_file)
+ do_per_function (execute_function_dump, dump_file_finish);
pass_fini_dump_file (pass);
current_pass = NULL;
@@ -2005,9 +2086,15 @@ execute_one_pass (struct opt_pass *pass)
initializing_dump = pass_init_dump_file (pass);
+ if (dump_file && (dump_flags & TDF_START))
+ do_per_function (execute_function_dump, dump_file_start);
+
/* Run pre-pass verification. */
execute_todo (pass->todo_flags_start);
+ if (dump_file && (dump_flags & TDF_BEFORE))
+ do_per_function (execute_function_dump, dump_file_before);
+
#ifdef ENABLE_CHECKING
do_per_function (verify_curr_properties,
(void *)(size_t)pass->properties_required);
@@ -2042,10 +2129,16 @@ execute_one_pass (struct opt_pass *pass)
clean_graph_dump_file (dump_file_name);
}
+ if (dump_file && (dump_flags & TDF_AFTER))
+ do_per_function (execute_function_dump, dump_file_after);
+
/* Run post-pass cleanup and verification. */
execute_todo (todo_after | pass->todo_flags_finish);
verify_interpass_invariants ();
- do_per_function (execute_function_dump, NULL);
+
+ if (dump_file)
+ do_per_function (execute_function_dump, dump_file_finish);
+
if (pass->type == IPA_PASS)
{
struct cgraph_node *node;
prev parent reply other threads:[~2011-06-14 23:15 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
2011-06-14 20:05 ` Xinliang David Li
2011-06-14 23:22 ` Xinliang David Li [this message]
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=BANLkTikcP9Q4cUcOdTXHszZHA2+bPgGYQW3ExJfQCh7SfhhrgA@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).