* Re: -fdump-passes -fenable-xxx=func_name_list
[not found] <BANLkTikXRUTmZZokg4OtJA5fBrWUG+7yZux3=CLDBox1Q+Qhtw@mail.gmail.com>
@ 2011-06-01 8:51 ` Richard Guenther
2011-06-01 16:17 ` Xinliang David Li
0 siblings, 1 reply; 30+ messages in thread
From: Richard Guenther @ 2011-06-01 8:51 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
> The following patch implements the a new option that dumps gcc PASS
> configuration. The sample output is attached. There is one
> limitation: some placeholder passes that are named with '*xxx' are
> note registered thus they are not listed. They are not important as
> they can not be turned on/off anyway.
>
> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
> of function assembler names to be specified.
>
> Ok for trunk?
Please split the patch.
I'm not too happy how you dump the pass configuration. Why not simply,
at a _single_ place, walk the pass tree? Instead of doing pieces of it
at pass execution time when it's not already dumped - that really looks
gross.
The documentation should also link this option to the -fenable/disable
options as obviously the pass names in that dump are those to be
used for those flags (and not readily available anywhere else).
I also think that it would be way more useful to note in the individual
dump files the functions (at the place they would usually appear) that
have the pass explicitly enabled/disabled.
Richard.
> Thanks,
>
> David
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-01 8:51 ` -fdump-passes -fenable-xxx=func_name_list Richard Guenther
@ 2011-06-01 16:17 ` Xinliang David Li
2011-06-01 17:24 ` Xinliang David Li
` (2 more replies)
0 siblings, 3 replies; 30+ messages in thread
From: Xinliang David Li @ 2011-06-01 16:17 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>> The following patch implements the a new option that dumps gcc PASS
>> configuration. The sample output is attached. There is one
>> limitation: some placeholder passes that are named with '*xxx' are
>> note registered thus they are not listed. They are not important as
>> they can not be turned on/off anyway.
>>
>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>> of function assembler names to be specified.
>>
>> Ok for trunk?
>
> Please split the patch.
>
> I'm not too happy how you dump the pass configuration. Why not simply,
> at a _single_ place, walk the pass tree? Instead of doing pieces of it
> at pass execution time when it's not already dumped - that really looks
> gross.
Yes, that was the original plan -- but it has problems
1) the dumper needs to know the root pass lists -- which can change
frequently -- it can be a long term maintanance burden;
2) the centralized dumper needs to be done after option processing
3) not sure if gate functions have any side effects or have dependencies on cfun
The proposed solutions IMHO is not that intrusive -- just three hooks
to do the dumping and tracking indentation.
>
> The documentation should also link this option to the -fenable/disable
> options as obviously the pass names in that dump are those to be
> used for those flags (and not readily available anywhere else).
Ok.
>
> I also think that it would be way more useful to note in the individual
> dump files the functions (at the place they would usually appear) that
> have the pass explicitly enabled/disabled.
Ok -- for ipa passes or tree/rtl passes where all functions are
explicitly disabled.
Thanks,
David
>
> Richard.
>
>> Thanks,
>>
>> David
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-01 16:17 ` Xinliang David Li
@ 2011-06-01 17:24 ` Xinliang David Li
2011-06-05 17:25 ` Xinliang David Li
2011-06-06 11:22 ` Richard Guenther
2011-06-01 19:29 ` Xinliang David Li
2011-06-01 19:29 ` Richard Guenther
2 siblings, 2 replies; 30+ messages in thread
From: Xinliang David Li @ 2011-06-01 17:24 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 2203 bytes --]
The attached is the split #1 patch that enhances -fenable/disable.
Ok after testing?
Thanks,
David
On Wed, Jun 1, 2011 at 9:16 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> The following patch implements the a new option that dumps gcc PASS
>>> configuration. The sample output is attached. There is one
>>> limitation: some placeholder passes that are named with '*xxx' are
>>> note registered thus they are not listed. They are not important as
>>> they can not be turned on/off anyway.
>>>
>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>> of function assembler names to be specified.
>>>
>>> Ok for trunk?
>>
>> Please split the patch.
>>
>> I'm not too happy how you dump the pass configuration. Why not simply,
>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>> at pass execution time when it's not already dumped - that really looks
>> gross.
>
> Yes, that was the original plan -- but it has problems
> 1) the dumper needs to know the root pass lists -- which can change
> frequently -- it can be a long term maintanance burden;
> 2) the centralized dumper needs to be done after option processing
> 3) not sure if gate functions have any side effects or have dependencies on cfun
>
> The proposed solutions IMHO is not that intrusive -- just three hooks
> to do the dumping and tracking indentation.
>
>>
>> The documentation should also link this option to the -fenable/disable
>> options as obviously the pass names in that dump are those to be
>> used for those flags (and not readily available anywhere else).
>
> Ok.
>
>>
>> I also think that it would be way more useful to note in the individual
>> dump files the functions (at the place they would usually appear) that
>> have the pass explicitly enabled/disabled.
>
> Ok -- for ipa passes or tree/rtl passes where all functions are
> explicitly disabled.
>
> Thanks,
>
> David
>
>>
>> Richard.
>>
>>> Thanks,
>>>
>>> David
>>>
>>
>
[-- Attachment #2: enable-disable-funcname.p --]
[-- Type: text/x-pascal, Size: 14301 bytes --]
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi (revision 174424)
+++ doc/invoke.texi (working copy)
@@ -5056,11 +5056,12 @@ appended with a sequential number starti
Disable rtl pass @var{pass}. @var{pass} is the pass name. If the same pass is
statically invoked in the compiler multiple times, the pass name should be
appended with a sequential number starting from 1. @var{range-list} is a comma
-seperated list of function ranges. Each range is a number pair seperated by a colon.
-The range is inclusive in both ends. If the range is trivial, the number pair can be
-simplified a a single number. If the function's cgraph node's @var{uid} is falling
-within one of the specified ranges, the @var{pass} is disabled for that function.
-The @var{uid} is shown in the function header of a dump file.
+seperated list of function ranges or assembler names. Each range is a number
+pair seperated by a colon. The range is inclusive in both ends. If the range
+is trivial, the number pair can be simplified as a single number. If the
+function's cgraph node's @var{uid} is falling within one of the specified ranges,
+the @var{pass} is disabled for that function. The @var{uid} is shown in the
+function header of a dump file.
@item -fdisable-tree-@var{pass}
@item -fdisable-tree-@var{pass}=@var{range-list}
@@ -5090,7 +5091,8 @@ of option arguments.
-fenable-tree-cunroll=1
# disable gcse2 for functions at the following ranges [1,1],
# [300,400], and [400,1000]
- -fdisable-rtl-gcse2=1:100,300,400:1000
+# disable gcse2 for functions foo and foo2
+ -fdisable-rtl-gcse2=foo,foo2
# disable early inlining
-fdisable-tree-einline
# disable ipa inlining
Index: testsuite/gcc.dg/inline_2.c
===================================================================
--- testsuite/gcc.dg/inline_2.c (revision 0)
+++ testsuite/gcc.dg/inline_2.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=0:3 -fdisable-ipa-inline" } */
+int g;
+__attribute__((always_inline)) void bar (void)
+{
+ g++;
+}
+
+int foo (void)
+{
+ bar ();
+ return g;
+}
+
+int foo2 (void)
+{
+ bar();
+ return g + 1;
+}
+
+/* { dg-final { scan-tree-dump-times "bar" 5 "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/inline_6.c
===================================================================
--- testsuite/gcc.dg/inline_6.c (revision 0)
+++ testsuite/gcc.dg/inline_6.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=foo2 -fdisable-ipa-inline" } */
+int g;
+__attribute__((always_inline)) void bar (void)
+{
+ g++;
+}
+
+int foo (void)
+{
+ bar ();
+ return g;
+}
+
+int foo2 (void)
+{
+ bar();
+ return g + 1;
+}
+
+/* { dg-final { scan-tree-dump-times "bar" 4 "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/unroll_2.c
===================================================================
--- testsuite/gcc.dg/unroll_2.c (revision 0)
+++ testsuite/gcc.dg/unroll_2.c (revision 0)
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-rtl-loop2_unroll -fno-peel-loops -fdisable-tree-cunroll=1 -fdisable-tree-cunrolli=1 -fenable-rtl-loop2_unroll" } */
+
+unsigned a[100], b[100];
+inline void bar()
+{
+ a[10] = b[10];
+}
+
+int foo(void)
+{
+ int i;
+ bar();
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+int foo2(void)
+{
+ int i;
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+/* { dg-final { scan-rtl-dump-times "Decided to peel loop completely" 1 "loop2_unroll" } } */
+/* { dg-final { cleanup-rtl-dump "loop2_unroll" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/inline_3.c
===================================================================
--- testsuite/gcc.dg/inline_3.c (revision 0)
+++ testsuite/gcc.dg/inline_3.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=0:1,2,99:100 -fdisable-ipa-inline" } */
+int g;
+__attribute__((always_inline)) void bar (void)
+{
+ g++;
+}
+
+int foo (void)
+{
+ bar ();
+ return g;
+}
+
+int foo2 (void)
+{
+ bar();
+ return g + 1;
+}
+
+/* { dg-final { scan-tree-dump-times "bar" 5 "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/unroll_3.c
===================================================================
--- testsuite/gcc.dg/unroll_3.c (revision 0)
+++ testsuite/gcc.dg/unroll_3.c (revision 0)
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-rtl-loop2_unroll -fno-peel-loops -fdisable-tree-cunroll -fdisable-tree-cunrolli -fenable-rtl-loop2_unroll=1" } */
+
+unsigned a[100], b[100];
+inline void bar()
+{
+ a[10] = b[10];
+}
+
+int foo(void)
+{
+ int i;
+ bar();
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+int foo2(void)
+{
+ int i;
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+/* { dg-final { scan-rtl-dump-times "Decided to peel loop completely" 1 "loop2_unroll" } } */
+/* { dg-final { cleanup-rtl-dump "loop2_unroll" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/inline_4.c
===================================================================
--- testsuite/gcc.dg/inline_4.c (revision 0)
+++ testsuite/gcc.dg/inline_4.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=1:1,3,4:100 -fdisable-ipa-inline" } */
+int g;
+__attribute__((always_inline)) void bar (void)
+{
+ g++;
+}
+
+int foo (void)
+{
+ bar ();
+ return g;
+}
+
+int foo2 (void)
+{
+ bar();
+ return g + 1;
+}
+
+/* { dg-final { scan-tree-dump-times "bar" 4 "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/unroll_4.c
===================================================================
--- testsuite/gcc.dg/unroll_4.c (revision 0)
+++ testsuite/gcc.dg/unroll_4.c (revision 0)
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-rtl-loop2_unroll -fno-peel-loops -fdisable-tree-cunroll -fdisable-tree-cunrolli -fenable-rtl-loop2_unroll=foo2" } */
+
+unsigned a[100], b[100];
+inline void bar()
+{
+ a[10] = b[10];
+}
+
+int foo(void)
+{
+ int i;
+ bar();
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+int foo2(void)
+{
+ int i;
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+/* { dg-final { scan-rtl-dump-times "Decided to peel loop completely" 1 "loop2_unroll" } } */
+/* { dg-final { cleanup-rtl-dump "loop2_unroll" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/inline_1.c
===================================================================
--- testsuite/gcc.dg/inline_1.c (revision 0)
+++ testsuite/gcc.dg/inline_1.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline -fdisable-ipa-inline" } */
+int g;
+__attribute__((always_inline)) void bar (void)
+{
+ g++;
+}
+
+int foo (void)
+{
+ bar ();
+ return g;
+}
+
+int foo2 (void)
+{
+ bar();
+ return g + 1;
+}
+
+/* { dg-final { scan-tree-dump-times "bar" 5 "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/inline_5.c
===================================================================
--- testsuite/gcc.dg/inline_5.c (revision 0)
+++ testsuite/gcc.dg/inline_5.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=foo,foo2 -fdisable-ipa-inline" } */
+int g;
+__attribute__((always_inline)) void bar (void)
+{
+ g++;
+}
+
+int foo (void)
+{
+ bar ();
+ return g;
+}
+
+int foo2 (void)
+{
+ bar();
+ return g + 1;
+}
+
+/* { dg-final { scan-tree-dump-times "bar" 5 "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/unroll_1.c
===================================================================
--- testsuite/gcc.dg/unroll_1.c (revision 0)
+++ testsuite/gcc.dg/unroll_1.c (revision 0)
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-rtl-loop2_unroll -fno-peel-loops -fdisable-tree-cunroll -fdisable-tree-cunrolli -fenable-rtl-loop2_unroll" } */
+
+unsigned a[100], b[100];
+inline void bar()
+{
+ a[10] = b[10];
+}
+
+int foo(void)
+{
+ int i;
+ bar();
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+int foo2(void)
+{
+ int i;
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+/* { dg-final { scan-rtl-dump-times "Decided to peel loop completely" 2 "loop2_unroll" } } */
+/* { dg-final { cleanup-rtl-dump "loop2_unroll" } } */
+/* { dg-excess-errors "extra notes" } */
Index: passes.c
===================================================================
--- passes.c (revision 174424)
+++ passes.c (working copy)
@@ -530,6 +530,7 @@ struct uid_range
{
unsigned int start;
unsigned int last;
+ const char *assem_name;
struct uid_range *next;
};
@@ -541,6 +542,35 @@ DEF_VEC_ALLOC_P(uid_range_p, heap);
static VEC(uid_range_p, heap) *enabled_pass_uid_range_tab = NULL;
static VEC(uid_range_p, heap) *disabled_pass_uid_range_tab = NULL;
+/* A helper function to determine if an identifier is valid to
+ be an assembler name (better to use target specific hook). */
+
+static bool
+is_valid_assembler_name (const char *str)
+{
+ const char *p = str;
+ char c;
+
+ c = *p;
+ if (!((c >= 'a' && c <= 'z')
+ || (c >= 'A' && c <= 'Z')
+ || *p == '_'))
+ return false;
+
+ p++;
+ while ((c = *p))
+ {
+ if (!((c >= 'a' && c <= 'z')
+ || (c >= 'A' && c <= 'Z')
+ || (c >= '0' && c <= '9')
+ || *p == '_'))
+ return false;
+ p++;
+ }
+
+ return true;
+}
+
/* Parse option string for -fdisable- and -fenable-
The syntax of the options:
@@ -627,6 +657,7 @@ enable_disable_pass (const char *arg, bo
uid_range_p new_range;
char *invalid = NULL;
long start;
+ char *func_name = NULL;
next_range = strchr (one_range, ',');
if (next_range)
@@ -644,17 +675,31 @@ enable_disable_pass (const char *arg, bo
start = strtol (one_range, &invalid, 10);
if (*invalid || start < 0)
{
- error ("Invalid range %s in option %s",
- one_range,
- is_enable ? "-fenable" : "-fdisable");
- free (argstr);
- return;
+ if (end_val || !is_valid_assembler_name (one_range))
+ {
+ error ("Invalid range %s in option %s",
+ one_range,
+ is_enable ? "-fenable" : "-fdisable");
+ free (argstr);
+ return;
+ }
+ else
+ func_name = one_range;
}
if (!end_val)
{
new_range = XCNEW (struct uid_range);
- new_range->start = (unsigned) start;
- new_range->last = (unsigned) start;
+ if (!func_name)
+ {
+ new_range->start = (unsigned) start;
+ new_range->last = (unsigned) start;
+ }
+ else
+ {
+ new_range->start = (unsigned) -1;
+ new_range->last = (unsigned) -1;
+ new_range->assem_name = xstrdup (func_name);
+ }
}
else
{
@@ -676,15 +721,28 @@ enable_disable_pass (const char *arg, bo
new_range->next = slot;
VEC_replace (uid_range_p, *tab, pass->static_pass_number,
new_range);
-
if (is_enable)
- inform (UNKNOWN_LOCATION,
- "enable pass %s for functions in the range of [%u, %u]",
- phase_name, new_range->start, new_range->last);
+ {
+ if (new_range->assem_name)
+ inform (UNKNOWN_LOCATION,
+ "enable pass %s for function %s",
+ phase_name, new_range->assem_name);
+ else
+ inform (UNKNOWN_LOCATION,
+ "enable pass %s for functions in the range of [%u, %u]",
+ phase_name, new_range->start, new_range->last);
+ }
else
- inform (UNKNOWN_LOCATION,
- "disable pass %s for functions in the range of [%u, %u]",
- phase_name, new_range->start, new_range->last);
+ {
+ if (new_range->assem_name)
+ inform (UNKNOWN_LOCATION,
+ "disable pass %s for function %s",
+ phase_name, new_range->assem_name);
+ else
+ inform (UNKNOWN_LOCATION,
+ "disable pass %s for functions in the range of [%u, %u]",
+ phase_name, new_range->start, new_range->last);
+ }
one_range = next_range;
} while (next_range);
@@ -718,6 +776,7 @@ is_pass_explicitly_enabled_or_disabled (
{
uid_range_p slot, range;
int cgraph_uid;
+ const char *aname = NULL;
if (!tab
|| (unsigned) pass->static_pass_number >= VEC_length (uid_range_p, tab)
@@ -729,6 +788,8 @@ is_pass_explicitly_enabled_or_disabled (
return false;
cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
+ if (func && DECL_ASSEMBLER_NAME_SET_P (func))
+ aname = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (func));
range = slot;
while (range)
@@ -736,6 +797,9 @@ is_pass_explicitly_enabled_or_disabled (
if ((unsigned) cgraph_uid >= range->start
&& (unsigned) cgraph_uid <= range->last)
return true;
+ if (range->assem_name && aname
+ && !strcmp (range->assem_name, aname))
+ return true;
range = range->next;
}
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-01 16:17 ` Xinliang David Li
2011-06-01 17:24 ` Xinliang David Li
2011-06-01 19:29 ` Xinliang David Li
@ 2011-06-01 19:29 ` Richard Guenther
2011-06-01 19:46 ` Xinliang David Li
2 siblings, 1 reply; 30+ messages in thread
From: Richard Guenther @ 2011-06-01 19:29 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> The following patch implements the a new option that dumps gcc PASS
>>> configuration. The sample output is attached. There is one
>>> limitation: some placeholder passes that are named with '*xxx' are
>>> note registered thus they are not listed. They are not important as
>>> they can not be turned on/off anyway.
>>>
>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>> of function assembler names to be specified.
>>>
>>> Ok for trunk?
>>
>> Please split the patch.
>>
>> I'm not too happy how you dump the pass configuration. Why not simply,
>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>> at pass execution time when it's not already dumped - that really looks
>> gross.
>
> Yes, that was the original plan -- but it has problems
> 1) the dumper needs to know the root pass lists -- which can change
> frequently -- it can be a long term maintanance burden;
> 2) the centralized dumper needs to be done after option processing
> 3) not sure if gate functions have any side effects or have dependencies on cfun
>
> The proposed solutions IMHO is not that intrusive -- just three hooks
> to do the dumping and tracking indentation.
Well, if you have a CU that is empty or optimized to nothing at some point
you will not get a complete pass list. I suppose optimize attributes might
also confuse output. Your solution might not be that intrusive
but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
dumping from toplev_main before calling do_compile (), 3) gate functions
shouldn't have side-effects, but as they could gate on optimize_for_speed ()
your option summary output will be bogus anyway.
So - what is the output intended for if it isn't reliable?
Richard.
>>
>> The documentation should also link this option to the -fenable/disable
>> options as obviously the pass names in that dump are those to be
>> used for those flags (and not readily available anywhere else).
>
> Ok.
>
>>
>> I also think that it would be way more useful to note in the individual
>> dump files the functions (at the place they would usually appear) that
>> have the pass explicitly enabled/disabled.
>
> Ok -- for ipa passes or tree/rtl passes where all functions are
> explicitly disabled.
>
> Thanks,
>
> David
>
>>
>> Richard.
>>
>>> Thanks,
>>>
>>> David
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-01 16:17 ` Xinliang David Li
2011-06-01 17:24 ` Xinliang David Li
@ 2011-06-01 19:29 ` Xinliang David Li
2011-06-01 19:29 ` Richard Guenther
2 siblings, 0 replies; 30+ messages in thread
From: Xinliang David Li @ 2011-06-01 19:29 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 2184 bytes --]
The attached is patch-2 (-fdump-passes) and a sample output:
Ok for trunk?
David
On Wed, Jun 1, 2011 at 9:16 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> The following patch implements the a new option that dumps gcc PASS
>>> configuration. The sample output is attached. There is one
>>> limitation: some placeholder passes that are named with '*xxx' are
>>> note registered thus they are not listed. They are not important as
>>> they can not be turned on/off anyway.
>>>
>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>> of function assembler names to be specified.
>>>
>>> Ok for trunk?
>>
>> Please split the patch.
>>
>> I'm not too happy how you dump the pass configuration. Why not simply,
>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>> at pass execution time when it's not already dumped - that really looks
>> gross.
>
> Yes, that was the original plan -- but it has problems
> 1) the dumper needs to know the root pass lists -- which can change
> frequently -- it can be a long term maintanance burden;
> 2) the centralized dumper needs to be done after option processing
> 3) not sure if gate functions have any side effects or have dependencies on cfun
>
> The proposed solutions IMHO is not that intrusive -- just three hooks
> to do the dumping and tracking indentation.
>
>>
>> The documentation should also link this option to the -fenable/disable
>> options as obviously the pass names in that dump are those to be
>> used for those flags (and not readily available anywhere else).
>
> Ok.
>
>>
>> I also think that it would be way more useful to note in the individual
>> dump files the functions (at the place they would usually appear) that
>> have the pass explicitly enabled/disabled.
>
> Ok -- for ipa passes or tree/rtl passes where all functions are
> explicitly disabled.
>
> Thanks,
>
> David
>
>>
>> Richard.
>>
>>> Thanks,
>>>
>>> David
>>>
>>
>
[-- Attachment #2: dump-pass2.p --]
[-- Type: text/x-pascal, Size: 7868 bytes --]
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi (revision 174535)
+++ doc/invoke.texi (working copy)
@@ -291,6 +291,7 @@ Objective-C and Objective-C++ Dialects}.
-fdump-translation-unit@r{[}-@var{n}@r{]} @gol
-fdump-class-hierarchy@r{[}-@var{n}@r{]} @gol
-fdump-ipa-all -fdump-ipa-cgraph -fdump-ipa-inline @gol
+-fdump-passes @gol
-fdump-statistics @gol
-fdump-tree-all @gol
-fdump-tree-original@r{[}-@var{n}@r{]} @gol
@@ -5060,7 +5061,8 @@ seperated list of function ranges. Each
The range is inclusive in both ends. If the range is trivial, the number pair can be
simplified a a single number. If the function's cgraph node's @var{uid} is falling
within one of the specified ranges, the @var{pass} is disabled for that function.
-The @var{uid} is shown in the function header of a dump file.
+The @var{uid} is shown in the function header of a dump file, and pass names can be
+dumped by using option @option{-fdump-passes}.
@item -fdisable-tree-@var{pass}
@item -fdisable-tree-@var{pass}=@var{range-list}
@@ -5483,6 +5485,11 @@ Dump after function inlining.
@end table
+@item -fdump-passes
+@opindex fdump-passes
+Dump the list of optimization passes that are turned on and off by
+the current command line options.
+
@item -fdump-statistics-@var{option}
@opindex fdump-statistics
Enable and control dumping of pass statistics in a separate file. The
Index: common.opt
===================================================================
--- common.opt (revision 174535)
+++ common.opt (working copy)
@@ -1012,6 +1012,10 @@ fdump-noaddr
Common Report Var(flag_dump_noaddr)
Suppress output of addresses in debugging dumps
+fdump-passes
+Common Var(flag_dump_passes) Init(0)
+Dump optimization passes
+
fdump-unnumbered
Common Report Var(flag_dump_unnumbered)
Suppress output of instruction numbers, line number notes and addresses in debugging dumps
Index: passes.c
===================================================================
--- passes.c (revision 174536)
+++ passes.c (working copy)
@@ -478,7 +478,7 @@ passr_eq (const void *p1, const void *p2
return !strcmp (s1->unique_name, s2->unique_name);
}
-static htab_t pass_name_tab = NULL;
+static htab_t name_to_pass_map = NULL;
/* Register PASS with NAME. */
@@ -488,11 +488,11 @@ register_pass_name (struct opt_pass *pas
struct pass_registry **slot;
struct pass_registry pr;
- if (!pass_name_tab)
- pass_name_tab = htab_create (256, passr_hash, passr_eq, NULL);
+ if (!name_to_pass_map)
+ name_to_pass_map = htab_create (256, passr_hash, passr_eq, NULL);
pr.unique_name = name;
- slot = (struct pass_registry **) htab_find_slot (pass_name_tab, &pr, INSERT);
+ slot = (struct pass_registry **) htab_find_slot (name_to_pass_map, &pr, INSERT);
if (!*slot)
{
struct pass_registry *new_pr;
@@ -506,6 +506,117 @@ register_pass_name (struct opt_pass *pas
return; /* Ignore plugin passes. */
}
+typedef struct {
+ /* Pass name with kind prefix and instance number suffix. */
+ const char *pass_name;
+ /* Flag indicating if the pass info has been dumped. */
+ bool dumped;
+} pass_info;
+
+DEF_VEC_O(pass_info);
+DEF_VEC_ALLOC_O(pass_info, heap);
+static VEC(pass_info, heap) *pass_tab = NULL;
+
+/* Callback function for traversing NAME_TO_PASS_MAP. */
+
+static int
+pass_traverse (void **slot, void *data)
+{
+ int* tab_size = (int *)data;
+ struct pass_registry **p = (struct pass_registry **)slot;
+ struct opt_pass *pass = (*p)->pass;
+ pass_info *pd;
+
+ gcc_assert (pass->static_pass_number > 0);
+ if (tab_size)
+ {
+ if (pass->static_pass_number > *tab_size)
+ *tab_size = pass->static_pass_number;
+
+ return 1;
+ }
+
+ gcc_assert (pass_tab);
+ pd = VEC_index (pass_info, pass_tab, pass->static_pass_number);
+ pd->pass_name = (*p)->unique_name;
+ pd->dumped = false;
+
+ return 1;
+}
+
+/* The function traverses NAME_TO_PASS_MAP and creates a pass info
+ table for dumping purpose. */
+
+static void
+create_pass_tab (void)
+{
+ int tab_size = 0;
+
+ if (!flag_dump_passes || pass_tab)
+ return;
+
+ htab_traverse (name_to_pass_map, pass_traverse, &tab_size);
+ VEC_safe_grow_cleared (pass_info, heap,
+ pass_tab, tab_size + 1);
+ htab_traverse (name_to_pass_map, pass_traverse, NULL);
+}
+
+
+static int pass_indent = 0;
+
+/* Tracks pass dumping indentation. */
+
+static inline void
+enter_pass_list (void)
+{
+ pass_indent++;
+}
+
+/* Tracks pass dumping indentation. */
+
+static inline void
+exit_pass_list (void)
+{
+ pass_indent--;
+}
+
+/* Dump the instantiated name for PASS. IS_ON indicates if PASS
+ is turned on or not. */
+
+static void
+dump_one_pass (struct opt_pass *pass, bool is_on, bool is_really_on)
+{
+ pass_info *pi;
+ int indent = 3 * pass_indent;
+ static int uid_range_dumped = false;
+
+ if (!uid_range_dumped)
+ {
+ fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
+ uid_range_dumped = true;
+ }
+
+ create_pass_tab();
+ gcc_assert (pass_tab);
+
+ if (pass->static_pass_number <= 0)
+ return;
+
+ pi = VEC_index (pass_info, pass_tab,
+ pass->static_pass_number);
+ if (pi->dumped)
+ return;
+
+ fprintf (stderr, "%*s%-35s%*s:%s%s\n", indent, " ",
+ pi->pass_name,
+ (10 - indent < 0 ? 0 : 10 - indent), " ",
+ is_on ? " ON" : " OFF",
+ ((!is_on) == (!is_really_on) ? ""
+ : (is_really_on ? " (FORCED_ON)" : " (FORCED_OFF)")));
+ pi->dumped = true;
+}
+
+
/* Returns the pass with NAME. */
static struct opt_pass *
@@ -513,9 +624,9 @@ get_pass_by_name (const char *name)
{
struct pass_registry **slot, pr;
- gcc_assert (pass_name_tab);
+ gcc_assert (name_to_pass_map);
pr.unique_name = name;
- slot = (struct pass_registry **) htab_find_slot (pass_name_tab,
+ slot = (struct pass_registry **) htab_find_slot (name_to_pass_map,
&pr, NO_INSERT);
if (!slot || !*slot)
@@ -1807,7 +1918,7 @@ execute_one_pass (struct opt_pass *pass)
bool initializing_dump;
unsigned int todo_after = 0;
- bool gate_status;
+ bool gate_status0, gate_status;
/* IPA passes are executed on whole program, so cfun should be NULL.
Other passes need function context set. */
@@ -1820,8 +1931,11 @@ execute_one_pass (struct opt_pass *pass)
/* Check whether gate check should be avoided.
User controls the value of the gate through the parameter "gate_status". */
- gate_status = (pass->gate == NULL) ? true : pass->gate();
- gate_status = override_gate_status (pass, current_function_decl, gate_status);
+ gate_status0 = (pass->gate == NULL) ? true : pass->gate();
+ gate_status = override_gate_status (pass, current_function_decl, gate_status0);
+
+ if (flag_dump_passes)
+ dump_one_pass (pass, gate_status0, gate_status);
/* Override gate with plugin. */
invoke_plugin_callbacks (PLUGIN_OVERRIDE_GATE, &gate_status);
@@ -1910,6 +2024,7 @@ execute_one_pass (struct opt_pass *pass)
void
execute_pass_list (struct opt_pass *pass)
{
+ enter_pass_list ();
do
{
gcc_assert (pass->type == GIMPLE_PASS
@@ -1919,6 +2034,7 @@ execute_pass_list (struct opt_pass *pass
pass = pass->next;
}
while (pass);
+ exit_pass_list ();
}
/* Same as execute_pass_list but assume that subpasses of IPA passes
@@ -2221,6 +2337,7 @@ ipa_read_optimization_summaries (void)
void
execute_ipa_pass_list (struct opt_pass *pass)
{
+ enter_pass_list ();
do
{
gcc_assert (!current_function_decl);
@@ -2246,6 +2363,7 @@ execute_ipa_pass_list (struct opt_pass *
pass = pass->next;
}
while (pass);
+ exit_pass_list ();
}
/* Execute stmt fixup hooks of all passes in PASS for NODE and STMTS. */
[-- Attachment #3: out --]
[-- Type: application/octet-stream, Size: 10765 bytes --]
cc1: note: disable pass ipa-inline for functions in the range of [0, 4294967295]
cc1: note: enable pass tree-unswitch for functions in the range of [0, 4294967295]
MAX_UID = 4
tree-mudflap1 : OFF
tree-omplower : ON
tree-lower : ON
tree-ehopt : OFF
tree-eh : ON
tree-cfg : ON
ipa-visibility : ON
ipa-early_local_cleanups : ON
tree-ompexp : OFF
tree-ssa : ON
tree-veclower : ON
tree-inline_param1 : ON
tree-einline : ON
tree-early_optimizations : ON
tree-copyrename1 : ON
tree-ccp1 : ON
tree-forwprop1 : ON
tree-ealias : ON
tree-esra : ON
tree-fre1 : ON
tree-copyprop1 : ON
tree-mergephi1 : ON
tree-cddce1 : ON
tree-eipa_sra : ON
tree-tailr1 : ON
tree-switchconv : ON
tree-ehcleanup1 : OFF
tree-profile_estimate : ON
tree-local-pure-const1 : ON
tree-fnsplit : ON
tree-release_ssa : ON
tree-inline_param2 : ON
ipa-profile : OFF
ipa-increase_alignment : OFF
ipa-matrix-reorg : OFF
ipa-emutls : OFF
ipa-whole-program : ON
ipa-profile_estimate : ON
ipa-cp : ON
ipa-cdtor : OFF
ipa-inline : ON (FORCED_OFF)
ipa-pure-const : ON
ipa-static-var : ON
ipa-pta : OFF
tree-ehdisp : OFF
tree-copyrename2 : ON
tree-cunrolli : ON
tree-ccp2 : ON
tree-forwprop2 : ON
tree-cdce : ON
tree-alias : ON
tree-retslot : ON
tree-phiprop : ON
tree-fre2 : ON
tree-copyprop2 : ON
tree-mergephi2 : ON
tree-vrp1 : ON
tree-dce1 : ON
tree-cselim : ON
tree-ifcombine : ON
tree-phiopt1 : ON
tree-tailr2 : ON
tree-ch : ON
tree-stdarg : OFF
tree-cplxlower : ON
tree-sra : ON
tree-copyrename3 : ON
tree-dom1 : ON
tree-phicprop1 : ON
tree-dse1 : ON
tree-reassoc1 : ON
tree-dce2 : ON
tree-forwprop3 : ON
tree-phiopt2 : ON
tree-objsz : ON
tree-ccp3 : ON
tree-copyprop3 : ON
tree-sincos : ON
tree-bswap : ON
tree-crited : ON
tree-pre : ON
tree-sink : ON
tree-loop : ON
tree-loopinit : ON
tree-lim1 : ON
tree-copyprop4 : ON
tree-dceloop1 : ON
tree-unswitch : OFF (FORCED_ON)
tree-sccp : ON
tree-ckdd : OFF
tree-ldist : OFF
tree-copyprop5 : ON
tree-graphite0 : OFF
tree-ivcanon : ON
tree-ifcvt : OFF
tree-vect : OFF
tree-pcom : OFF
tree-cunroll : ON
tree-slp : OFF
tree-parloops : OFF
tree-aprefetch : OFF
tree-ivopts : ON
tree-loopdone : ON
tree-recip : OFF
tree-reassoc2 : ON
tree-vrp2 : ON
tree-dom2 : ON
tree-phicprop2 : ON
tree-cddce2 : ON
tree-tracer : OFF
tree-uninit : OFF
tree-dse2 : ON
tree-forwprop4 : ON
tree-phiopt3 : ON
tree-fab : ON
tree-widening_mul : ON
tree-tailc : ON
tree-copyrename4 : ON
tree-uncprop : ON
tree-local-pure-const2 : ON
tree-cplxlower0 : OFF
tree-ehcleanup2 : OFF
tree-resx : OFF
tree-nrv : ON
tree-mudflap2 : OFF
tree-optimized : ON
rtl-expand : ON
rtl-sibling : ON
rtl-rtl_eh : OFF
rtl-initvals : ON
rtl-unshare : ON
rtl-vregs : ON
rtl-into_cfglayout : ON
rtl-jump : ON
rtl-subreg1 : ON
rtl-dfinit : ON
rtl-cse1 : ON
rtl-fwprop1 : ON
rtl-cprop1 : ON
rtl-rtl pre : ON
rtl-hoist : OFF
rtl-cprop2 : ON
rtl-store_motion : OFF
rtl-cse_local : OFF
rtl-ce1 : ON
rtl-reginfo : ON
rtl-loop2 : ON
rtl-loop2_init : ON
rtl-loop2_invariant : ON
rtl-loop2_unswitch : OFF
rtl-loop2_unroll : OFF
rtl-loop2_doloop : OFF
rtl-loop2_done : ON
rtl-web : OFF
rtl-cprop3 : ON
rtl-cse2 : ON
rtl-dse1 : ON
rtl-fwprop2 : ON
rtl-auto_inc_dec : OFF
rtl-init-regs : ON
rtl-ud_dce : ON
rtl-combine : ON
rtl-ce2 : ON
rtl-bbpart : OFF
rtl-regmove : ON
rtl-outof_cfglayout : ON
rtl-split1 : ON
rtl-subreg2 : ON
rtl-no-opt dfinit : OFF
rtl-mode_sw : ON
rtl-asmcons : ON
rtl-sms : OFF
rtl-sched1 : OFF
rtl-ira : ON
rtl-postreload : ON
rtl-gcse2 : OFF
rtl-split2 : ON
rtl-zee : ON
rtl-cmpelim : OFF
rtl-btl1 : OFF
rtl-pro_and_epilogue : ON
rtl-dse2 : ON
rtl-csa : ON
rtl-peephole2 : ON
rtl-ce3 : ON
rtl-rnreg : OFF
rtl-cprop_hardreg : ON
rtl-rtl_dce : ON
rtl-bbro : ON
rtl-btl2 : OFF
rtl-split4 : ON
rtl-sched2 : ON
rtl-split3 : OFF
rtl-stack : ON
rtl-alignments : ON
rtl-compgotos : ON
rtl-vartrack : OFF
rtl-mach : ON
rtl-barriers : ON
rtl-dbr : OFF
rtl-split5 : OFF
rtl-eh_ranges : OFF
rtl-shorten : ON
rtl-nothrow : ON
rtl-final : ON
rtl-dfinish : ON
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-01 19:29 ` Richard Guenther
@ 2011-06-01 19:46 ` Xinliang David Li
2011-06-02 7:13 ` Xinliang David Li
0 siblings, 1 reply; 30+ messages in thread
From: Xinliang David Li @ 2011-06-01 19:46 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> The following patch implements the a new option that dumps gcc PASS
>>>> configuration. The sample output is attached. There is one
>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>> note registered thus they are not listed. They are not important as
>>>> they can not be turned on/off anyway.
>>>>
>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>> of function assembler names to be specified.
>>>>
>>>> Ok for trunk?
>>>
>>> Please split the patch.
>>>
>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>> at pass execution time when it's not already dumped - that really looks
>>> gross.
>>
>> Yes, that was the original plan -- but it has problems
>> 1) the dumper needs to know the root pass lists -- which can change
>> frequently -- it can be a long term maintanance burden;
>> 2) the centralized dumper needs to be done after option processing
>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>
>> The proposed solutions IMHO is not that intrusive -- just three hooks
>> to do the dumping and tracking indentation.
>
> Well, if you have a CU that is empty or optimized to nothing at some point
> you will not get a complete pass list. I suppose optimize attributes might
> also confuse output. Your solution might not be that intrusive
> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
> dumping from toplev_main before calling do_compile (), 3) gate functions
> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
> your option summary output will be bogus anyway.
>
> So - what is the output intended for if it isn't reliable?
This needs to be cleaned up at some point -- the gate function should
behave the same for all functions and per-function decisions need to
be pushed down to the executor body. I will try to rework the patch
as you suggested to see if there are problems.
David
>
> Richard.
>
>>>
>>> The documentation should also link this option to the -fenable/disable
>>> options as obviously the pass names in that dump are those to be
>>> used for those flags (and not readily available anywhere else).
>>
>> Ok.
>>
>>>
>>> I also think that it would be way more useful to note in the individual
>>> dump files the functions (at the place they would usually appear) that
>>> have the pass explicitly enabled/disabled.
>>
>> Ok -- for ipa passes or tree/rtl passes where all functions are
>> explicitly disabled.
>>
>> Thanks,
>>
>> David
>>
>>>
>>> Richard.
>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-01 19:46 ` Xinliang David Li
@ 2011-06-02 7:13 ` Xinliang David Li
2011-06-05 17:25 ` Xinliang David Li
2011-06-06 11:38 ` Richard Guenther
0 siblings, 2 replies; 30+ messages in thread
From: Xinliang David Li @ 2011-06-02 7:13 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 3386 bytes --]
This is the version of the patch that walks through pass lists.
Ok with this one?
David
On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>> configuration. The sample output is attached. There is one
>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>> note registered thus they are not listed. They are not important as
>>>>> they can not be turned on/off anyway.
>>>>>
>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>> of function assembler names to be specified.
>>>>>
>>>>> Ok for trunk?
>>>>
>>>> Please split the patch.
>>>>
>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>> at pass execution time when it's not already dumped - that really looks
>>>> gross.
>>>
>>> Yes, that was the original plan -- but it has problems
>>> 1) the dumper needs to know the root pass lists -- which can change
>>> frequently -- it can be a long term maintanance burden;
>>> 2) the centralized dumper needs to be done after option processing
>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>
>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>> to do the dumping and tracking indentation.
>>
>> Well, if you have a CU that is empty or optimized to nothing at some point
>> you will not get a complete pass list. I suppose optimize attributes might
>> also confuse output. Your solution might not be that intrusive
>> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
>> dumping from toplev_main before calling do_compile (), 3) gate functions
>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>> your option summary output will be bogus anyway.
>>
>> So - what is the output intended for if it isn't reliable?
>
> This needs to be cleaned up at some point -- the gate function should
> behave the same for all functions and per-function decisions need to
> be pushed down to the executor body. I will try to rework the patch
> as you suggested to see if there are problems.
>
> David
>
>
>>
>> Richard.
>>
>>>>
>>>> The documentation should also link this option to the -fenable/disable
>>>> options as obviously the pass names in that dump are those to be
>>>> used for those flags (and not readily available anywhere else).
>>>
>>> Ok.
>>>
>>>>
>>>> I also think that it would be way more useful to note in the individual
>>>> dump files the functions (at the place they would usually appear) that
>>>> have the pass explicitly enabled/disabled.
>>>
>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>> explicitly disabled.
>>>
>>> Thanks,
>>>
>>> David
>>>
>>>>
>>>> Richard.
>>>>
>>>>> Thanks,
>>>>>
>>>>> David
>>>>>
>>>>
>>>
>>
>
[-- Attachment #2: dump-pass3.p --]
[-- Type: application/octet-stream, Size: 7243 bytes --]
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi (revision 174550)
+++ doc/invoke.texi (working copy)
@@ -291,6 +291,7 @@ Objective-C and Objective-C++ Dialects}.
-fdump-translation-unit@r{[}-@var{n}@r{]} @gol
-fdump-class-hierarchy@r{[}-@var{n}@r{]} @gol
-fdump-ipa-all -fdump-ipa-cgraph -fdump-ipa-inline @gol
+-fdump-passes @gol
-fdump-statistics @gol
-fdump-tree-all @gol
-fdump-tree-original@r{[}-@var{n}@r{]} @gol
@@ -5060,7 +5061,8 @@ seperated list of function ranges. Each
The range is inclusive in both ends. If the range is trivial, the number pair can be
simplified a a single number. If the function's cgraph node's @var{uid} is falling
within one of the specified ranges, the @var{pass} is disabled for that function.
-The @var{uid} is shown in the function header of a dump file.
+The @var{uid} is shown in the function header of a dump file, and pass names can be
+dumped by using option @option{-fdump-passes}.
@item -fdisable-tree-@var{pass}
@item -fdisable-tree-@var{pass}=@var{range-list}
@@ -5483,6 +5485,11 @@ Dump after function inlining.
@end table
+@item -fdump-passes
+@opindex fdump-passes
+Dump the list of optimization passes that are turned on and off by
+the current command line options.
+
@item -fdump-statistics-@var{option}
@opindex fdump-statistics
Enable and control dumping of pass statistics in a separate file. The
Index: tree-pass.h
===================================================================
--- tree-pass.h (revision 174550)
+++ tree-pass.h (working copy)
@@ -639,5 +639,6 @@ extern void do_per_function_toporder (vo
extern void disable_pass (const char *);
extern void enable_pass (const char *);
+extern void dump_passes (void);
#endif /* GCC_TREE_PASS_H */
Index: cgraphunit.c
===================================================================
--- cgraphunit.c (revision 174550)
+++ cgraphunit.c (working copy)
@@ -1112,6 +1112,9 @@ cgraph_finalize_compilation_unit (void)
fflush (stderr);
}
+ if (flag_dump_passes)
+ dump_passes ();
+
/* Gimplify and lower all functions, compute reachability and
remove unreachable nodes. */
cgraph_analyze_functions ();
Index: common.opt
===================================================================
--- common.opt (revision 174550)
+++ common.opt (working copy)
@@ -1012,6 +1012,10 @@ fdump-noaddr
Common Report Var(flag_dump_noaddr)
Suppress output of addresses in debugging dumps
+fdump-passes
+Common Var(flag_dump_passes) Init(0)
+Dump optimization passes
+
fdump-unnumbered
Common Report Var(flag_dump_unnumbered)
Suppress output of instruction numbers, line number notes and addresses in debugging dumps
Index: passes.c
===================================================================
--- passes.c (revision 174550)
+++ passes.c (working copy)
@@ -478,7 +478,7 @@ passr_eq (const void *p1, const void *p2
return !strcmp (s1->unique_name, s2->unique_name);
}
-static htab_t pass_name_tab = NULL;
+static htab_t name_to_pass_map = NULL;
/* Register PASS with NAME. */
@@ -488,11 +488,11 @@ register_pass_name (struct opt_pass *pas
struct pass_registry **slot;
struct pass_registry pr;
- if (!pass_name_tab)
- pass_name_tab = htab_create (256, passr_hash, passr_eq, NULL);
+ if (!name_to_pass_map)
+ name_to_pass_map = htab_create (256, passr_hash, passr_eq, NULL);
pr.unique_name = name;
- slot = (struct pass_registry **) htab_find_slot (pass_name_tab, &pr, INSERT);
+ slot = (struct pass_registry **) htab_find_slot (name_to_pass_map, &pr, INSERT);
if (!*slot)
{
struct pass_registry *new_pr;
@@ -506,6 +506,127 @@ register_pass_name (struct opt_pass *pas
return; /* Ignore plugin passes. */
}
+/* Map from pass id to canonicalized pass name. */
+
+typedef const char *char_ptr;
+DEF_VEC_P(char_ptr);
+DEF_VEC_ALLOC_P(char_ptr, heap);
+static VEC(char_ptr, heap) *pass_tab = NULL;
+
+/* Callback function for traversing NAME_TO_PASS_MAP. */
+
+static int
+pass_traverse (void **slot, void *data ATTRIBUTE_UNUSED)
+{
+ struct pass_registry **p = (struct pass_registry **)slot;
+ struct opt_pass *pass = (*p)->pass;
+
+ gcc_assert (pass->static_pass_number > 0);
+ gcc_assert (pass_tab);
+
+ VEC_replace (char_ptr, pass_tab, pass->static_pass_number,
+ (*p)->unique_name);
+
+ return 1;
+}
+
+/* The function traverses NAME_TO_PASS_MAP and creates a pass info
+ table for dumping purpose. */
+
+static void
+create_pass_tab (void)
+{
+ if (!flag_dump_passes)
+ return;
+
+ VEC_safe_grow_cleared (char_ptr, heap,
+ pass_tab, passes_by_id_size + 1);
+ htab_traverse (name_to_pass_map, pass_traverse, NULL);
+}
+
+static bool override_gate_status (struct opt_pass *, tree, bool);
+
+/* Dump the instantiated name for PASS. IS_ON indicates if PASS
+ is turned on or not. */
+
+static void
+dump_one_pass (struct opt_pass *pass, int pass_indent)
+{
+ int indent = 3 * pass_indent;
+ const char *pn;
+ bool is_on, is_really_on;
+
+ is_on = (pass->gate == NULL) ? true : pass->gate();
+ is_really_on = override_gate_status (pass, current_function_decl, is_on);
+
+ if (pass->static_pass_number <= 0)
+ pn = pass->name;
+ else
+ pn = VEC_index (char_ptr, pass_tab, pass->static_pass_number);
+
+ fprintf (stderr, "%*s%-40s%*s:%s%s\n", indent, " ", pn,
+ (15 - indent < 0 ? 0 : 15 - indent), " ",
+ is_on ? " ON" : " OFF",
+ ((!is_on) == (!is_really_on) ? ""
+ : (is_really_on ? " (FORCED_ON)" : " (FORCED_OFF)")));
+}
+
+/* Dump pass list PASS with indentation INDENT. */
+
+static void
+dump_pass_list (struct opt_pass *pass, int indent)
+{
+ do
+ {
+ dump_one_pass (pass, indent);
+ if (pass->sub)
+ dump_pass_list (pass->sub, indent + 1);
+ pass = pass->next;
+ }
+ while (pass);
+}
+
+/* Dump all optimization passes. */
+
+void
+dump_passes (void)
+{
+ struct cgraph_node *n, *node = NULL;
+ tree save_fndecl = current_function_decl;
+
+ fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
+
+ create_pass_tab();
+ gcc_assert (pass_tab);
+
+ n = cgraph_nodes;
+ while (n)
+ {
+ if (DECL_STRUCT_FUNCTION (n->decl))
+ {
+ node = n;
+ break;
+ }
+ n = n->next;
+ }
+
+ if (!node)
+ return;
+
+ push_cfun (DECL_STRUCT_FUNCTION (node->decl));
+ current_function_decl = node->decl;
+
+ dump_pass_list (all_lowering_passes, 1);
+ dump_pass_list (all_small_ipa_passes, 1);
+ dump_pass_list (all_regular_ipa_passes, 1);
+ dump_pass_list (all_lto_gen_passes, 1);
+ dump_pass_list (all_passes, 1);
+
+ pop_cfun ();
+ current_function_decl = save_fndecl;
+}
+
+
/* Returns the pass with NAME. */
static struct opt_pass *
@@ -513,9 +634,9 @@ get_pass_by_name (const char *name)
{
struct pass_registry **slot, pr;
- gcc_assert (pass_name_tab);
+ gcc_assert (name_to_pass_map);
pr.unique_name = name;
- slot = (struct pass_registry **) htab_find_slot (pass_name_tab,
+ slot = (struct pass_registry **) htab_find_slot (name_to_pass_map,
&pr, NO_INSERT);
if (!slot || !*slot)
[-- Attachment #3: out --]
[-- Type: application/octet-stream, Size: 14902 bytes --]
cc1: note: enable pass tree-unswitch for functions in the range of [0, 4294967295]
cc1: note: disable pass ipa-inline for functions in the range of [0, 4294967295]
MAX_UID = 4
*warn_unused_result : ON
*diagnose_omp_blocks : OFF
tree-mudflap1 : OFF
tree-omplower : ON
tree-lower : ON
tree-ehopt : OFF
tree-eh : ON
tree-cfg : ON
*warn_function_return : ON
*build_cgraph_edges : ON
*free_lang_data : ON
ipa-visibility : ON
ipa-early_local_cleanups : ON
*free_cfg_annotations : ON
*init_datastructures : ON
tree-ompexp : OFF
*referenced_vars : ON
tree-ssa : ON
tree-veclower : ON
*early_warn_uninitialized : OFF
*rebuild_cgraph_edges : ON
tree-inline_param1 : ON
tree-einline : ON
tree-early_optimizations : ON
*remove_cgraph_callee_edges : ON
tree-copyrename1 : ON
tree-ccp1 : ON
tree-forwprop1 : ON
tree-ealias : ON
tree-esra : ON
tree-fre1 : ON
tree-copyprop1 : ON
tree-mergephi1 : ON
tree-cddce1 : ON
tree-eipa_sra : ON
tree-tailr1 : ON
tree-switchconv : ON
tree-ehcleanup1 : OFF
tree-profile_estimate : ON
tree-local-pure-const1 : ON
tree-fnsplit : ON
tree-release_ssa : ON
*rebuild_cgraph_edges : ON
tree-inline_param2 : ON
ipa-profile : OFF
tree-feedback_fnsplit : OFF
ipa-increase_alignment : OFF
ipa-matrix-reorg : OFF
ipa-emutls : OFF
ipa-whole-program : ON
ipa-profile_estimate : ON
ipa-cp : ON
ipa-cdtor : OFF
ipa-inline : ON (FORCED_OFF)
ipa-pure-const : ON
ipa-static-var : ON
ipa-pta : OFF
ipa-lto_gimple_out : OFF
ipa-lto_decls_out : OFF
tree-ehdisp : OFF
*all_optimizations : ON
*remove_cgraph_callee_edges : ON
*strip_predict_hints : ON
tree-copyrename2 : ON
tree-cunrolli : ON
tree-ccp2 : ON
tree-forwprop2 : ON
tree-cdce : ON
tree-alias : ON
tree-retslot : ON
tree-phiprop : ON
tree-fre2 : ON
tree-copyprop2 : ON
tree-mergephi2 : ON
tree-vrp1 : ON
tree-dce1 : ON
tree-cselim : ON
tree-ifcombine : ON
tree-phiopt1 : ON
tree-tailr2 : ON
tree-ch : ON
tree-stdarg : OFF
tree-cplxlower : ON
tree-sra : ON
tree-copyrename3 : ON
tree-dom1 : ON
tree-phicprop1 : ON
tree-dse1 : ON
tree-reassoc1 : ON
tree-dce2 : ON
tree-forwprop3 : ON
tree-phiopt2 : ON
tree-objsz : ON
tree-ccp3 : ON
tree-copyprop3 : ON
tree-sincos : ON
tree-bswap : ON
tree-crited : ON
tree-pre : ON
tree-sink : ON
tree-loop : ON
tree-loopinit : ON
tree-lim1 : ON
tree-copyprop4 : ON
tree-dceloop1 : ON
tree-unswitch : OFF (FORCED_ON)
tree-sccp : ON
*record_bounds : ON
tree-ckdd : OFF
tree-ldist : OFF
tree-copyprop5 : ON
tree-graphite0 : OFF
tree-graphite : OFF
tree-lim2 : ON
tree-copyprop6 : ON
tree-dceloop2 : ON
tree-ivcanon : ON
tree-ifcvt : OFF
tree-vect : OFF
tree-veclower2 : OFF
tree-dceloop3 : ON
tree-pcom : OFF
tree-cunroll : ON
tree-slp : OFF
tree-parloops : OFF
tree-aprefetch : OFF
tree-ivopts : ON
tree-loopdone : ON
tree-recip : OFF
tree-reassoc2 : ON
tree-vrp2 : ON
tree-dom2 : ON
tree-phicprop2 : ON
tree-cddce2 : ON
tree-tracer : OFF
tree-uninit : OFF
tree-dse2 : ON
tree-forwprop4 : ON
tree-phiopt3 : ON
tree-fab : ON
tree-widening_mul : ON
tree-tailc : ON
tree-copyrename4 : ON
tree-uncprop : ON
tree-local-pure-const2 : ON
tree-cplxlower0 : ON
tree-ehcleanup2 : OFF
tree-resx : OFF
tree-nrv : ON
tree-mudflap2 : OFF
tree-optimized : ON
*warn_function_noreturn : OFF
rtl-expand : ON
*rest_of_compilation : ON
*init_function : ON
rtl-sibling : ON
rtl-rtl_eh : OFF
rtl-initvals : ON
rtl-unshare : ON
rtl-vregs : ON
rtl-into_cfglayout : ON
rtl-jump : ON
rtl-subreg1 : ON
rtl-dfinit : ON
rtl-cse1 : ON
rtl-fwprop1 : ON
rtl-cprop1 : ON
rtl-rtl pre : ON
rtl-hoist : OFF
rtl-cprop2 : ON
rtl-store_motion : OFF
rtl-cse_local : OFF
rtl-ce1 : ON
rtl-reginfo : ON
rtl-loop2 : ON
rtl-loop2_init : ON
rtl-loop2_invariant : ON
rtl-loop2_unswitch : OFF
rtl-loop2_unroll : OFF
rtl-loop2_doloop : OFF
rtl-loop2_done : ON
rtl-web : OFF
rtl-cprop3 : ON
rtl-cse2 : ON
rtl-dse1 : ON
rtl-fwprop2 : ON
rtl-auto_inc_dec : OFF
rtl-init-regs : ON
rtl-ud_dce : ON
rtl-combine : ON
rtl-ce2 : ON
rtl-bbpart : OFF
rtl-regmove : ON
rtl-outof_cfglayout : ON
rtl-split1 : ON
rtl-subreg2 : ON
rtl-no-opt dfinit : OFF
*stack_ptr_mod : ON
rtl-mode_sw : ON
rtl-asmcons : ON
rtl-sms : OFF
rtl-sched1 : OFF
rtl-ira : ON
*all-postreload : OFF
rtl-postreload : OFF
rtl-gcse2 : OFF
rtl-split2 : ON
rtl-zee : ON
rtl-cmpelim : OFF
rtl-btl1 : OFF
rtl-pro_and_epilogue : ON
rtl-dse2 : ON
rtl-csa : ON
rtl-peephole2 : ON
rtl-ce3 : ON
rtl-rnreg : OFF
rtl-cprop_hardreg : ON
rtl-rtl_dce : ON
rtl-bbro : ON
rtl-btl2 : OFF
*leaf_regs : ON
rtl-split4 : ON
rtl-sched2 : ON
*stack_regs : ON
rtl-split3 : OFF
rtl-stack : ON
rtl-alignments : ON
rtl-compgotos : ON
rtl-vartrack : OFF
*free_cfg : ON
rtl-mach : ON
rtl-barriers : ON
rtl-dbr : OFF
rtl-split5 : OFF
rtl-eh_ranges : OFF
rtl-shorten : ON
rtl-nothrow : ON
rtl-final : ON
rtl-dfinish : ON
*clean_state : ON
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-01 17:24 ` Xinliang David Li
@ 2011-06-05 17:25 ` Xinliang David Li
2011-06-06 11:22 ` Richard Guenther
1 sibling, 0 replies; 30+ messages in thread
From: Xinliang David Li @ 2011-06-05 17:25 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
Is this patch ok?
Thanks,
David
On Wed, Jun 1, 2011 at 10:24 AM, Xinliang David Li <davidxl@google.com> wrote:
> The attached is the split #1 patch that enhances -fenable/disable.
>
> Ok after testing?
>
> Thanks,
> David
>
> On Wed, Jun 1, 2011 at 9:16 AM, Xinliang David Li <davidxl@google.com> wrote:
>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> The following patch implements the a new option that dumps gcc PASS
>>>> configuration. The sample output is attached. There is one
>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>> note registered thus they are not listed. They are not important as
>>>> they can not be turned on/off anyway.
>>>>
>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>> of function assembler names to be specified.
>>>>
>>>> Ok for trunk?
>>>
>>> Please split the patch.
>>>
>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>> at pass execution time when it's not already dumped - that really looks
>>> gross.
>>
>> Yes, that was the original plan -- but it has problems
>> 1) the dumper needs to know the root pass lists -- which can change
>> frequently -- it can be a long term maintanance burden;
>> 2) the centralized dumper needs to be done after option processing
>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>
>> The proposed solutions IMHO is not that intrusive -- just three hooks
>> to do the dumping and tracking indentation.
>>
>>>
>>> The documentation should also link this option to the -fenable/disable
>>> options as obviously the pass names in that dump are those to be
>>> used for those flags (and not readily available anywhere else).
>>
>> Ok.
>>
>>>
>>> I also think that it would be way more useful to note in the individual
>>> dump files the functions (at the place they would usually appear) that
>>> have the pass explicitly enabled/disabled.
>>
>> Ok -- for ipa passes or tree/rtl passes where all functions are
>> explicitly disabled.
>>
>> Thanks,
>>
>> David
>>
>>>
>>> Richard.
>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-02 7:13 ` Xinliang David Li
@ 2011-06-05 17:25 ` Xinliang David Li
2011-06-06 11:38 ` Richard Guenther
1 sibling, 0 replies; 30+ messages in thread
From: Xinliang David Li @ 2011-06-05 17:25 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
Is this one ok?
Thanks,
David
On Thu, Jun 2, 2011 at 12:12 AM, Xinliang David Li <davidxl@google.com> wrote:
> This is the version of the patch that walks through pass lists.
>
> Ok with this one?
>
> David
>
> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>> configuration. The sample output is attached. There is one
>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>> note registered thus they are not listed. They are not important as
>>>>>> they can not be turned on/off anyway.
>>>>>>
>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>> of function assembler names to be specified.
>>>>>>
>>>>>> Ok for trunk?
>>>>>
>>>>> Please split the patch.
>>>>>
>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>> at pass execution time when it's not already dumped - that really looks
>>>>> gross.
>>>>
>>>> Yes, that was the original plan -- but it has problems
>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>> frequently -- it can be a long term maintanance burden;
>>>> 2) the centralized dumper needs to be done after option processing
>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>
>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>> to do the dumping and tracking indentation.
>>>
>>> Well, if you have a CU that is empty or optimized to nothing at some point
>>> you will not get a complete pass list. I suppose optimize attributes might
>>> also confuse output. Your solution might not be that intrusive
>>> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
>>> dumping from toplev_main before calling do_compile (), 3) gate functions
>>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>>> your option summary output will be bogus anyway.
>>>
>>> So - what is the output intended for if it isn't reliable?
>>
>> This needs to be cleaned up at some point -- the gate function should
>> behave the same for all functions and per-function decisions need to
>> be pushed down to the executor body. I will try to rework the patch
>> as you suggested to see if there are problems.
>>
>> David
>>
>>
>>>
>>> Richard.
>>>
>>>>>
>>>>> The documentation should also link this option to the -fenable/disable
>>>>> options as obviously the pass names in that dump are those to be
>>>>> used for those flags (and not readily available anywhere else).
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>> I also think that it would be way more useful to note in the individual
>>>>> dump files the functions (at the place they would usually appear) that
>>>>> have the pass explicitly enabled/disabled.
>>>>
>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>> explicitly disabled.
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-01 17:24 ` Xinliang David Li
2011-06-05 17:25 ` Xinliang David Li
@ 2011-06-06 11:22 ` Richard Guenther
2011-06-06 15:54 ` Xinliang David Li
2011-06-06 19:21 ` Xinliang David Li
1 sibling, 2 replies; 30+ messages in thread
From: Richard Guenther @ 2011-06-06 11:22 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
On Wed, Jun 1, 2011 at 7:24 PM, Xinliang David Li <davidxl@google.com> wrote:
> The attached is the split #1 patch that enhances -fenable/disable.
>
> Ok after testing?
I expect the testcases will be quite fragile, so while I appreciate
test coverage for new options I think we should go without those
that involve any kind of UID. Those which use assembler names
also will fail randomly dependent on how targets mangle their
functions - so I think we have to drop all testcases.
Also
+/* A helper function to determine if an identifier is valid to
+ be an assembler name (better to use target specific hook). */
+
+static bool
+is_valid_assembler_name (const char *str)
+{
+ const char *p = str;
+ char c;
+
+ c = *p;
+ if (!((c >= 'a' && c <= 'z')
+ || (c >= 'A' && c <= 'Z')
+ || *p == '_'))
+ return false;
+
+ p++;
+ while ((c = *p))
+ {
+ if (!((c >= 'a' && c <= 'z')
+ || (c >= 'A' && c <= 'Z')
+ || (c >= '0' && c <= '9')
+ || *p == '_'))
+ return false;
+ p++;
+ }
+
+ return true;
+}
why all that complicated checks? Why not just check for p[0]
in [^0-9] and re-structure the range parsing to switch between
UIDs and assembler-names that way?
Thanks,
Richard.
> Thanks,
> David
>
> On Wed, Jun 1, 2011 at 9:16 AM, Xinliang David Li <davidxl@google.com> wrote:
>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> The following patch implements the a new option that dumps gcc PASS
>>>> configuration. The sample output is attached. There is one
>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>> note registered thus they are not listed. They are not important as
>>>> they can not be turned on/off anyway.
>>>>
>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>> of function assembler names to be specified.
>>>>
>>>> Ok for trunk?
>>>
>>> Please split the patch.
>>>
>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>> at pass execution time when it's not already dumped - that really looks
>>> gross.
>>
>> Yes, that was the original plan -- but it has problems
>> 1) the dumper needs to know the root pass lists -- which can change
>> frequently -- it can be a long term maintanance burden;
>> 2) the centralized dumper needs to be done after option processing
>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>
>> The proposed solutions IMHO is not that intrusive -- just three hooks
>> to do the dumping and tracking indentation.
>>
>>>
>>> The documentation should also link this option to the -fenable/disable
>>> options as obviously the pass names in that dump are those to be
>>> used for those flags (and not readily available anywhere else).
>>
>> Ok.
>>
>>>
>>> I also think that it would be way more useful to note in the individual
>>> dump files the functions (at the place they would usually appear) that
>>> have the pass explicitly enabled/disabled.
>>
>> Ok -- for ipa passes or tree/rtl passes where all functions are
>> explicitly disabled.
>>
>> Thanks,
>>
>> David
>>
>>>
>>> Richard.
>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-02 7:13 ` Xinliang David Li
2011-06-05 17:25 ` Xinliang David Li
@ 2011-06-06 11:38 ` Richard Guenther
2011-06-06 16:00 ` Xinliang David Li
1 sibling, 1 reply; 30+ messages in thread
From: Richard Guenther @ 2011-06-06 11:38 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davidxl@google.com> wrote:
> This is the version of the patch that walks through pass lists.
>
> Ok with this one?
+/* Dump all optimization passes. */
+
+void
+dump_passes (void)
+{
+ struct cgraph_node *n, *node = NULL;
+ tree save_fndecl = current_function_decl;
+
+ fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
this isn't accurate info - cloning can cause more cgraph nodes to
appear (it also looks completely unrelated to dump_passes ...).
Please drop it.
+ create_pass_tab();
+ gcc_assert (pass_tab);
you have quite many asserts of this kind - we don't want them when
the previous stmt as in this case indicates everything is ok.
+ push_cfun (DECL_STRUCT_FUNCTION (node->decl));
this has side-effects, I'm not sure we want this here. Why do you
need it? Probably because of
+ is_really_on = override_gate_status (pass, current_function_decl, is_on);
? But that is dependent on the function given which should have no
effect (unless it is overridden globally in which case override_gate_status
and friends should deal with a NULL cfun).
I don't understand why you need another table mapping pass to name
when pass->name is available and the info is trivially re-constructible.
Thanks,
Richard.
> David
>
> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>> configuration. The sample output is attached. There is one
>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>> note registered thus they are not listed. They are not important as
>>>>>> they can not be turned on/off anyway.
>>>>>>
>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>> of function assembler names to be specified.
>>>>>>
>>>>>> Ok for trunk?
>>>>>
>>>>> Please split the patch.
>>>>>
>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>> at pass execution time when it's not already dumped - that really looks
>>>>> gross.
>>>>
>>>> Yes, that was the original plan -- but it has problems
>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>> frequently -- it can be a long term maintanance burden;
>>>> 2) the centralized dumper needs to be done after option processing
>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>
>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>> to do the dumping and tracking indentation.
>>>
>>> Well, if you have a CU that is empty or optimized to nothing at some point
>>> you will not get a complete pass list. I suppose optimize attributes might
>>> also confuse output. Your solution might not be that intrusive
>>> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
>>> dumping from toplev_main before calling do_compile (), 3) gate functions
>>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>>> your option summary output will be bogus anyway.
>>>
>>> So - what is the output intended for if it isn't reliable?
>>
>> This needs to be cleaned up at some point -- the gate function should
>> behave the same for all functions and per-function decisions need to
>> be pushed down to the executor body. I will try to rework the patch
>> as you suggested to see if there are problems.
>>
>> David
>>
>>
>>>
>>> Richard.
>>>
>>>>>
>>>>> The documentation should also link this option to the -fenable/disable
>>>>> options as obviously the pass names in that dump are those to be
>>>>> used for those flags (and not readily available anywhere else).
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>> I also think that it would be way more useful to note in the individual
>>>>> dump files the functions (at the place they would usually appear) that
>>>>> have the pass explicitly enabled/disabled.
>>>>
>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>> explicitly disabled.
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-06 11:22 ` Richard Guenther
@ 2011-06-06 15:54 ` Xinliang David Li
2011-06-06 15:59 ` Richard Guenther
2011-06-06 19:21 ` Xinliang David Li
1 sibling, 1 reply; 30+ messages in thread
From: Xinliang David Li @ 2011-06-06 15:54 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
On Mon, Jun 6, 2011 at 4:22 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Wed, Jun 1, 2011 at 7:24 PM, Xinliang David Li <davidxl@google.com> wrote:
>> The attached is the split #1 patch that enhances -fenable/disable.
>>
>> Ok after testing?
>
> I expect the testcases will be quite fragile, so while I appreciate
> test coverage for new options I think we should go without those
> that involve any kind of UID. Those which use assembler names
> also will fail randomly dependent on how targets mangle their
> functions - so I think we have to drop all testcases.
Ok -- how about keeping tests with large uid range, and assembler name
for x86? A feature without testing is just to easy to break without
being noticed.
>
> Also
>
> +/* A helper function to determine if an identifier is valid to
> + be an assembler name (better to use target specific hook). */
> +
> +static bool
> +is_valid_assembler_name (const char *str)
> +{
> + const char *p = str;
> + char c;
> +
> + c = *p;
> + if (!((c >= 'a' && c <= 'z')
> + || (c >= 'A' && c <= 'Z')
> + || *p == '_'))
> + return false;
> +
> + p++;
> + while ((c = *p))
> + {
> + if (!((c >= 'a' && c <= 'z')
> + || (c >= 'A' && c <= 'Z')
> + || (c >= '0' && c <= '9')
> + || *p == '_'))
> + return false;
> + p++;
> + }
> +
> + return true;
> +}
>
> why all that complicated checks? Why not just check for p[0]
> in [^0-9] and re-structure the range parsing to switch between
> UIDs and assembler-names that way?
Ok.
David
>
> Thanks,
> Richard.
>
>> Thanks,
>> David
>>
>> On Wed, Jun 1, 2011 at 9:16 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>> configuration. The sample output is attached. There is one
>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>> note registered thus they are not listed. They are not important as
>>>>> they can not be turned on/off anyway.
>>>>>
>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>> of function assembler names to be specified.
>>>>>
>>>>> Ok for trunk?
>>>>
>>>> Please split the patch.
>>>>
>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>> at pass execution time when it's not already dumped - that really looks
>>>> gross.
>>>
>>> Yes, that was the original plan -- but it has problems
>>> 1) the dumper needs to know the root pass lists -- which can change
>>> frequently -- it can be a long term maintanance burden;
>>> 2) the centralized dumper needs to be done after option processing
>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>
>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>> to do the dumping and tracking indentation.
>>>
>>>>
>>>> The documentation should also link this option to the -fenable/disable
>>>> options as obviously the pass names in that dump are those to be
>>>> used for those flags (and not readily available anywhere else).
>>>
>>> Ok.
>>>
>>>>
>>>> I also think that it would be way more useful to note in the individual
>>>> dump files the functions (at the place they would usually appear) that
>>>> have the pass explicitly enabled/disabled.
>>>
>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>> explicitly disabled.
>>>
>>> Thanks,
>>>
>>> David
>>>
>>>>
>>>> Richard.
>>>>
>>>>> Thanks,
>>>>>
>>>>> David
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-06 15:54 ` Xinliang David Li
@ 2011-06-06 15:59 ` Richard Guenther
0 siblings, 0 replies; 30+ messages in thread
From: Richard Guenther @ 2011-06-06 15:59 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
On Mon, Jun 6, 2011 at 5:53 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Mon, Jun 6, 2011 at 4:22 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jun 1, 2011 at 7:24 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> The attached is the split #1 patch that enhances -fenable/disable.
>>>
>>> Ok after testing?
>>
>> I expect the testcases will be quite fragile, so while I appreciate
>> test coverage for new options I think we should go without those
>> that involve any kind of UID. Those which use assembler names
>> also will fail randomly dependent on how targets mangle their
>> functions - so I think we have to drop all testcases.
>
> Ok -- how about keeping tests with large uid range, and assembler name
> for x86? A feature without testing is just to easy to break without
> being noticed.
That's true. Running the tests on a few selected known-good targets
sounds good.
Richard.
>>
>> Also
>>
>> +/* A helper function to determine if an identifier is valid to
>> + be an assembler name (better to use target specific hook). */
>> +
>> +static bool
>> +is_valid_assembler_name (const char *str)
>> +{
>> + const char *p = str;
>> + char c;
>> +
>> + c = *p;
>> + if (!((c >= 'a' && c <= 'z')
>> + || (c >= 'A' && c <= 'Z')
>> + || *p == '_'))
>> + return false;
>> +
>> + p++;
>> + while ((c = *p))
>> + {
>> + if (!((c >= 'a' && c <= 'z')
>> + || (c >= 'A' && c <= 'Z')
>> + || (c >= '0' && c <= '9')
>> + || *p == '_'))
>> + return false;
>> + p++;
>> + }
>> +
>> + return true;
>> +}
>>
>> why all that complicated checks? Why not just check for p[0]
>> in [^0-9] and re-structure the range parsing to switch between
>> UIDs and assembler-names that way?
>
> Ok.
>
> David
>
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> David
>>>
>>> On Wed, Jun 1, 2011 at 9:16 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>> configuration. The sample output is attached. There is one
>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>> note registered thus they are not listed. They are not important as
>>>>>> they can not be turned on/off anyway.
>>>>>>
>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>> of function assembler names to be specified.
>>>>>>
>>>>>> Ok for trunk?
>>>>>
>>>>> Please split the patch.
>>>>>
>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>> at pass execution time when it's not already dumped - that really looks
>>>>> gross.
>>>>
>>>> Yes, that was the original plan -- but it has problems
>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>> frequently -- it can be a long term maintanance burden;
>>>> 2) the centralized dumper needs to be done after option processing
>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>
>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>> to do the dumping and tracking indentation.
>>>>
>>>>>
>>>>> The documentation should also link this option to the -fenable/disable
>>>>> options as obviously the pass names in that dump are those to be
>>>>> used for those flags (and not readily available anywhere else).
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>> I also think that it would be way more useful to note in the individual
>>>>> dump files the functions (at the place they would usually appear) that
>>>>> have the pass explicitly enabled/disabled.
>>>>
>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>> explicitly disabled.
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-06 11:38 ` Richard Guenther
@ 2011-06-06 16:00 ` Xinliang David Li
2011-06-06 19:23 ` Xinliang David Li
2011-06-07 10:10 ` Richard Guenther
0 siblings, 2 replies; 30+ messages in thread
From: Xinliang David Li @ 2011-06-06 16:00 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davidxl@google.com> wrote:
>> This is the version of the patch that walks through pass lists.
>>
>> Ok with this one?
>
> +/* Dump all optimization passes. */
> +
> +void
> +dump_passes (void)
> +{
> + struct cgraph_node *n, *node = NULL;
> + tree save_fndecl = current_function_decl;
> +
> + fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
>
> this isn't accurate info - cloning can cause more cgraph nodes to
> appear (it also looks completely unrelated to dump_passes ...).
> Please drop it.
Ok.
>
> + create_pass_tab();
> + gcc_assert (pass_tab);
>
> you have quite many asserts of this kind - we don't want them when
> the previous stmt as in this case indicates everything is ok.
ok.
>
> + push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>
> this has side-effects, I'm not sure we want this here. Why do you
> need it? Probably because of
>
> + is_really_on = override_gate_status (pass, current_function_decl, is_on);
>
> ? But that is dependent on the function given which should have no
> effect (unless it is overridden globally in which case override_gate_status
> and friends should deal with a NULL cfun).
As we discussed, currently some pass gate functions depend on per node
information -- those checks need to be pushed into execute functions.
I would like to clean those up later -- at which time, the push/pop
can be removed.
>
> I don't understand why you need another table mapping pass to name
> when pass->name is available and the info is trivially re-constructible.
This is needed as the pass->name is not the canonicalized name (i.e.,
not with number suffix etc), so the extra mapping from id to
normalized name is needed.
Thanks,
David
>
> Thanks,
> Richard.
>
>> David
>>
>> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>>> configuration. The sample output is attached. There is one
>>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>>> note registered thus they are not listed. They are not important as
>>>>>>> they can not be turned on/off anyway.
>>>>>>>
>>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>>> of function assembler names to be specified.
>>>>>>>
>>>>>>> Ok for trunk?
>>>>>>
>>>>>> Please split the patch.
>>>>>>
>>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>>> at pass execution time when it's not already dumped - that really looks
>>>>>> gross.
>>>>>
>>>>> Yes, that was the original plan -- but it has problems
>>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>>> frequently -- it can be a long term maintanance burden;
>>>>> 2) the centralized dumper needs to be done after option processing
>>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>>
>>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>>> to do the dumping and tracking indentation.
>>>>
>>>> Well, if you have a CU that is empty or optimized to nothing at some point
>>>> you will not get a complete pass list. I suppose optimize attributes might
>>>> also confuse output. Your solution might not be that intrusive
>>>> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
>>>> dumping from toplev_main before calling do_compile (), 3) gate functions
>>>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>>>> your option summary output will be bogus anyway.
>>>>
>>>> So - what is the output intended for if it isn't reliable?
>>>
>>> This needs to be cleaned up at some point -- the gate function should
>>> behave the same for all functions and per-function decisions need to
>>> be pushed down to the executor body. I will try to rework the patch
>>> as you suggested to see if there are problems.
>>>
>>> David
>>>
>>>
>>>>
>>>> Richard.
>>>>
>>>>>>
>>>>>> The documentation should also link this option to the -fenable/disable
>>>>>> options as obviously the pass names in that dump are those to be
>>>>>> used for those flags (and not readily available anywhere else).
>>>>>
>>>>> Ok.
>>>>>
>>>>>>
>>>>>> I also think that it would be way more useful to note in the individual
>>>>>> dump files the functions (at the place they would usually appear) that
>>>>>> have the pass explicitly enabled/disabled.
>>>>>
>>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>>> explicitly disabled.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> David
>>>>>
>>>>>>
>>>>>> Richard.
>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-06 11:22 ` Richard Guenther
2011-06-06 15:54 ` Xinliang David Li
@ 2011-06-06 19:21 ` Xinliang David Li
2011-06-07 10:11 ` Richard Guenther
1 sibling, 1 reply; 30+ messages in thread
From: Xinliang David Li @ 2011-06-06 19:21 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 3786 bytes --]
Please take a look at the revised one.
Thanks,
David
On Mon, Jun 6, 2011 at 4:22 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Wed, Jun 1, 2011 at 7:24 PM, Xinliang David Li <davidxl@google.com> wrote:
>> The attached is the split #1 patch that enhances -fenable/disable.
>>
>> Ok after testing?
>
> I expect the testcases will be quite fragile, so while I appreciate
> test coverage for new options I think we should go without those
> that involve any kind of UID. Those which use assembler names
> also will fail randomly dependent on how targets mangle their
> functions - so I think we have to drop all testcases.
>
> Also
>
> +/* A helper function to determine if an identifier is valid to
> + be an assembler name (better to use target specific hook). */
> +
> +static bool
> +is_valid_assembler_name (const char *str)
> +{
> + const char *p = str;
> + char c;
> +
> + c = *p;
> + if (!((c >= 'a' && c <= 'z')
> + || (c >= 'A' && c <= 'Z')
> + || *p == '_'))
> + return false;
> +
> + p++;
> + while ((c = *p))
> + {
> + if (!((c >= 'a' && c <= 'z')
> + || (c >= 'A' && c <= 'Z')
> + || (c >= '0' && c <= '9')
> + || *p == '_'))
> + return false;
> + p++;
> + }
> +
> + return true;
> +}
>
> why all that complicated checks? Why not just check for p[0]
> in [^0-9] and re-structure the range parsing to switch between
> UIDs and assembler-names that way?
>
> Thanks,
> Richard.
>
>> Thanks,
>> David
>>
>> On Wed, Jun 1, 2011 at 9:16 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>> configuration. The sample output is attached. There is one
>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>> note registered thus they are not listed. They are not important as
>>>>> they can not be turned on/off anyway.
>>>>>
>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>> of function assembler names to be specified.
>>>>>
>>>>> Ok for trunk?
>>>>
>>>> Please split the patch.
>>>>
>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>> at pass execution time when it's not already dumped - that really looks
>>>> gross.
>>>
>>> Yes, that was the original plan -- but it has problems
>>> 1) the dumper needs to know the root pass lists -- which can change
>>> frequently -- it can be a long term maintanance burden;
>>> 2) the centralized dumper needs to be done after option processing
>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>
>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>> to do the dumping and tracking indentation.
>>>
>>>>
>>>> The documentation should also link this option to the -fenable/disable
>>>> options as obviously the pass names in that dump are those to be
>>>> used for those flags (and not readily available anywhere else).
>>>
>>> Ok.
>>>
>>>>
>>>> I also think that it would be way more useful to note in the individual
>>>> dump files the functions (at the place they would usually appear) that
>>>> have the pass explicitly enabled/disabled.
>>>
>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>> explicitly disabled.
>>>
>>> Thanks,
>>>
>>> David
>>>
>>>>
>>>> Richard.
>>>>
>>>>> Thanks,
>>>>>
>>>>> David
>>>>>
>>>>
>>>
>>
>
[-- Attachment #2: enable-disable-funcname2.p --]
[-- Type: text/x-pascal, Size: 12575 bytes --]
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi (revision 174549)
+++ doc/invoke.texi (working copy)
@@ -5056,11 +5056,12 @@ appended with a sequential number starti
Disable rtl pass @var{pass}. @var{pass} is the pass name. If the same pass is
statically invoked in the compiler multiple times, the pass name should be
appended with a sequential number starting from 1. @var{range-list} is a comma
-seperated list of function ranges. Each range is a number pair seperated by a colon.
-The range is inclusive in both ends. If the range is trivial, the number pair can be
-simplified a a single number. If the function's cgraph node's @var{uid} is falling
-within one of the specified ranges, the @var{pass} is disabled for that function.
-The @var{uid} is shown in the function header of a dump file.
+seperated list of function ranges or assembler names. Each range is a number
+pair seperated by a colon. The range is inclusive in both ends. If the range
+is trivial, the number pair can be simplified as a single number. If the
+function's cgraph node's @var{uid} is falling within one of the specified ranges,
+the @var{pass} is disabled for that function. The @var{uid} is shown in the
+function header of a dump file.
@item -fdisable-tree-@var{pass}
@item -fdisable-tree-@var{pass}=@var{range-list}
@@ -5090,7 +5091,8 @@ of option arguments.
-fenable-tree-cunroll=1
# disable gcse2 for functions at the following ranges [1,1],
# [300,400], and [400,1000]
- -fdisable-rtl-gcse2=1:100,300,400:1000
+# disable gcse2 for functions foo and foo2
+ -fdisable-rtl-gcse2=foo,foo2
# disable early inlining
-fdisable-tree-einline
# disable ipa inlining
Index: testsuite/gcc.dg/inline_2.c
===================================================================
--- testsuite/gcc.dg/inline_2.c (revision 0)
+++ testsuite/gcc.dg/inline_2.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=0:100 -fdisable-ipa-inline" } */
+int g;
+__attribute__((always_inline)) void bar (void)
+{
+ g++;
+}
+
+int foo (void)
+{
+ bar ();
+ return g;
+}
+
+int foo2 (void)
+{
+ bar();
+ return g + 1;
+}
+
+/* { dg-final { scan-tree-dump-times "bar" 5 "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/unroll_2.c
===================================================================
--- testsuite/gcc.dg/unroll_2.c (revision 0)
+++ testsuite/gcc.dg/unroll_2.c (revision 0)
@@ -0,0 +1,33 @@
+/* { dg-do compile { target i?86-*-linux* x86_64-*-linux* } } */
+/* { dg-options "-O2 -fdump-rtl-loop2_unroll -fno-peel-loops -fdisable-tree-cunroll=foo -fdisable-tree-cunrolli=foo -fenable-rtl-loop2_unroll" } */
+
+unsigned a[100], b[100];
+inline void bar()
+{
+ a[10] = b[10];
+}
+
+int foo(void)
+{
+ int i;
+ bar();
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+int foo2(void)
+{
+ int i;
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+/* { dg-final { scan-rtl-dump-times "Decided to peel loop completely" 1 "loop2_unroll" } } */
+/* { dg-final { cleanup-rtl-dump "loop2_unroll" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/inline_3.c
===================================================================
--- testsuite/gcc.dg/inline_3.c (revision 0)
+++ testsuite/gcc.dg/inline_3.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile { target i?86-*-linux* x86_64-*-linux* } } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=foo,foo2 -fdisable-ipa-inline" } */
+int g;
+__attribute__((always_inline)) void bar (void)
+{
+ g++;
+}
+
+int foo (void)
+{
+ bar ();
+ return g;
+}
+
+int foo2 (void)
+{
+ bar();
+ return g + 1;
+}
+
+/* { dg-final { scan-tree-dump-times "bar" 5 "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/unroll_3.c
===================================================================
--- testsuite/gcc.dg/unroll_3.c (revision 0)
+++ testsuite/gcc.dg/unroll_3.c (revision 0)
@@ -0,0 +1,33 @@
+/* { dg-do compile { target i?86-*-linux* x86_64-*-linux* } } */
+/* { dg-options "-O2 -fdump-rtl-loop2_unroll -fno-peel-loops -fdisable-tree-cunroll -fdisable-tree-cunrolli -fenable-rtl-loop2_unroll=foo" } */
+
+unsigned a[100], b[100];
+inline void bar()
+{
+ a[10] = b[10];
+}
+
+int foo(void)
+{
+ int i;
+ bar();
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+int foo2(void)
+{
+ int i;
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+/* { dg-final { scan-rtl-dump-times "Decided to peel loop completely" 1 "loop2_unroll" } } */
+/* { dg-final { cleanup-rtl-dump "loop2_unroll" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/inline_4.c
===================================================================
--- testsuite/gcc.dg/inline_4.c (revision 0)
+++ testsuite/gcc.dg/inline_4.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile { target i?86-*-linux* x86_64-*-linux* } } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline=foo2 -fdisable-ipa-inline" } */
+int g;
+__attribute__((always_inline)) void bar (void)
+{
+ g++;
+}
+
+int foo (void)
+{
+ bar ();
+ return g;
+}
+
+int foo2 (void)
+{
+ bar();
+ return g + 1;
+}
+
+/* { dg-final { scan-tree-dump-times "bar" 4 "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/unroll_4.c
===================================================================
--- testsuite/gcc.dg/unroll_4.c (revision 0)
+++ testsuite/gcc.dg/unroll_4.c (revision 0)
@@ -0,0 +1,33 @@
+/* { dg-do compile { target i?86-*-linux* x86_64-*-linux* } } */
+/* { dg-options "-O2 -fdump-rtl-loop2_unroll -fno-peel-loops -fdisable-tree-cunroll -fdisable-tree-cunrolli -fenable-rtl-loop2_unroll=foo2" } */
+
+unsigned a[100], b[100];
+inline void bar()
+{
+ a[10] = b[10];
+}
+
+int foo(void)
+{
+ int i;
+ bar();
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+int foo2(void)
+{
+ int i;
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+/* { dg-final { scan-rtl-dump-times "Decided to peel loop completely" 1 "loop2_unroll" } } */
+/* { dg-final { cleanup-rtl-dump "loop2_unroll" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/inline_1.c
===================================================================
--- testsuite/gcc.dg/inline_1.c (revision 0)
+++ testsuite/gcc.dg/inline_1.c (revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-optimized -fdisable-tree-einline -fdisable-ipa-inline" } */
+int g;
+__attribute__((always_inline)) void bar (void)
+{
+ g++;
+}
+
+int foo (void)
+{
+ bar ();
+ return g;
+}
+
+int foo2 (void)
+{
+ bar();
+ return g + 1;
+}
+
+/* { dg-final { scan-tree-dump-times "bar" 5 "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
+/* { dg-excess-errors "extra notes" } */
Index: testsuite/gcc.dg/unroll_1.c
===================================================================
--- testsuite/gcc.dg/unroll_1.c (revision 0)
+++ testsuite/gcc.dg/unroll_1.c (revision 0)
@@ -0,0 +1,33 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-rtl-loop2_unroll -fno-peel-loops -fdisable-tree-cunroll -fdisable-tree-cunrolli -fenable-rtl-loop2_unroll" } */
+
+unsigned a[100], b[100];
+inline void bar()
+{
+ a[10] = b[10];
+}
+
+int foo(void)
+{
+ int i;
+ bar();
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+int foo2(void)
+{
+ int i;
+ for (i = 0; i < 2; i++)
+ {
+ a[i]= b[i] + 1;
+ }
+ return 1;
+}
+
+/* { dg-final { scan-rtl-dump-times "Decided to peel loop completely" 2 "loop2_unroll" } } */
+/* { dg-final { cleanup-rtl-dump "loop2_unroll" } } */
+/* { dg-excess-errors "extra notes" } */
Index: passes.c
===================================================================
--- passes.c (revision 174549)
+++ passes.c (working copy)
@@ -531,6 +531,7 @@ struct uid_range
{
unsigned int start;
unsigned int last;
+ const char *assem_name;
struct uid_range *next;
};
@@ -542,6 +543,7 @@ DEF_VEC_ALLOC_P(uid_range_p, heap);
static VEC(uid_range_p, heap) *enabled_pass_uid_range_tab = NULL;
static VEC(uid_range_p, heap) *disabled_pass_uid_range_tab = NULL;
+
/* Parse option string for -fdisable- and -fenable-
The syntax of the options:
@@ -628,6 +630,7 @@ enable_disable_pass (const char *arg, bo
uid_range_p new_range;
char *invalid = NULL;
long start;
+ char *func_name = NULL;
next_range = strchr (one_range, ',');
if (next_range)
@@ -645,17 +648,31 @@ enable_disable_pass (const char *arg, bo
start = strtol (one_range, &invalid, 10);
if (*invalid || start < 0)
{
- error ("Invalid range %s in option %s",
- one_range,
- is_enable ? "-fenable" : "-fdisable");
- free (argstr);
- return;
+ if (end_val || (one_range[0] >= '0'
+ && one_range[0] <= '9'))
+ {
+ error ("Invalid range %s in option %s",
+ one_range,
+ is_enable ? "-fenable" : "-fdisable");
+ free (argstr);
+ return;
+ }
+ func_name = one_range;
}
if (!end_val)
{
new_range = XCNEW (struct uid_range);
- new_range->start = (unsigned) start;
- new_range->last = (unsigned) start;
+ if (!func_name)
+ {
+ new_range->start = (unsigned) start;
+ new_range->last = (unsigned) start;
+ }
+ else
+ {
+ new_range->start = (unsigned) -1;
+ new_range->last = (unsigned) -1;
+ new_range->assem_name = xstrdup (func_name);
+ }
}
else
{
@@ -677,15 +694,28 @@ enable_disable_pass (const char *arg, bo
new_range->next = slot;
VEC_replace (uid_range_p, *tab, pass->static_pass_number,
new_range);
-
if (is_enable)
- inform (UNKNOWN_LOCATION,
- "enable pass %s for functions in the range of [%u, %u]",
- phase_name, new_range->start, new_range->last);
+ {
+ if (new_range->assem_name)
+ inform (UNKNOWN_LOCATION,
+ "enable pass %s for function %s",
+ phase_name, new_range->assem_name);
+ else
+ inform (UNKNOWN_LOCATION,
+ "enable pass %s for functions in the range of [%u, %u]",
+ phase_name, new_range->start, new_range->last);
+ }
else
- inform (UNKNOWN_LOCATION,
- "disable pass %s for functions in the range of [%u, %u]",
- phase_name, new_range->start, new_range->last);
+ {
+ if (new_range->assem_name)
+ inform (UNKNOWN_LOCATION,
+ "disable pass %s for function %s",
+ phase_name, new_range->assem_name);
+ else
+ inform (UNKNOWN_LOCATION,
+ "disable pass %s for functions in the range of [%u, %u]",
+ phase_name, new_range->start, new_range->last);
+ }
one_range = next_range;
} while (next_range);
@@ -719,6 +749,7 @@ is_pass_explicitly_enabled_or_disabled (
{
uid_range_p slot, range;
int cgraph_uid;
+ const char *aname = NULL;
if (!tab
|| (unsigned) pass->static_pass_number >= VEC_length (uid_range_p, tab)
@@ -730,6 +761,8 @@ is_pass_explicitly_enabled_or_disabled (
return false;
cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
+ if (func && DECL_ASSEMBLER_NAME_SET_P (func))
+ aname = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (func));
range = slot;
while (range)
@@ -737,6 +770,9 @@ is_pass_explicitly_enabled_or_disabled (
if ((unsigned) cgraph_uid >= range->start
&& (unsigned) cgraph_uid <= range->last)
return true;
+ if (range->assem_name && aname
+ && !strcmp (range->assem_name, aname))
+ return true;
range = range->next;
}
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-06 16:00 ` Xinliang David Li
@ 2011-06-06 19:23 ` Xinliang David Li
2011-06-07 10:10 ` Richard Guenther
1 sibling, 0 replies; 30+ messages in thread
From: Xinliang David Li @ 2011-06-06 19:23 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 5709 bytes --]
This is the patch with max id removed.
Thanks,
David
On Mon, Jun 6, 2011 at 9:00 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> This is the version of the patch that walks through pass lists.
>>>
>>> Ok with this one?
>>
>> +/* Dump all optimization passes. */
>> +
>> +void
>> +dump_passes (void)
>> +{
>> + struct cgraph_node *n, *node = NULL;
>> + tree save_fndecl = current_function_decl;
>> +
>> + fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
>>
>> this isn't accurate info - cloning can cause more cgraph nodes to
>> appear (it also looks completely unrelated to dump_passes ...).
>> Please drop it.
>
> Ok.
>
>
>>
>> + create_pass_tab();
>> + gcc_assert (pass_tab);
>>
>> you have quite many asserts of this kind - we don't want them when
>> the previous stmt as in this case indicates everything is ok.
>
> ok.
>
>>
>> + push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>>
>> this has side-effects, I'm not sure we want this here. Why do you
>> need it? Probably because of
>>
>> + is_really_on = override_gate_status (pass, current_function_decl, is_on);
>>
>> ? But that is dependent on the function given which should have no
>> effect (unless it is overridden globally in which case override_gate_status
>> and friends should deal with a NULL cfun).
>
> As we discussed, currently some pass gate functions depend on per node
> information -- those checks need to be pushed into execute functions.
> I would like to clean those up later -- at which time, the push/pop
> can be removed.
>
>>
>> I don't understand why you need another table mapping pass to name
>> when pass->name is available and the info is trivially re-constructible.
>
> This is needed as the pass->name is not the canonicalized name (i.e.,
> not with number suffix etc), so the extra mapping from id to
> normalized name is needed.
>
> Thanks,
>
> David
>
>>
>> Thanks,
>> Richard.
>>
>>> David
>>>
>>> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>>>> configuration. The sample output is attached. There is one
>>>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>>>> note registered thus they are not listed. They are not important as
>>>>>>>> they can not be turned on/off anyway.
>>>>>>>>
>>>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>>>> of function assembler names to be specified.
>>>>>>>>
>>>>>>>> Ok for trunk?
>>>>>>>
>>>>>>> Please split the patch.
>>>>>>>
>>>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>>>> at pass execution time when it's not already dumped - that really looks
>>>>>>> gross.
>>>>>>
>>>>>> Yes, that was the original plan -- but it has problems
>>>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>>>> frequently -- it can be a long term maintanance burden;
>>>>>> 2) the centralized dumper needs to be done after option processing
>>>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>>>
>>>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>>>> to do the dumping and tracking indentation.
>>>>>
>>>>> Well, if you have a CU that is empty or optimized to nothing at some point
>>>>> you will not get a complete pass list. I suppose optimize attributes might
>>>>> also confuse output. Your solution might not be that intrusive
>>>>> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
>>>>> dumping from toplev_main before calling do_compile (), 3) gate functions
>>>>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>>>>> your option summary output will be bogus anyway.
>>>>>
>>>>> So - what is the output intended for if it isn't reliable?
>>>>
>>>> This needs to be cleaned up at some point -- the gate function should
>>>> behave the same for all functions and per-function decisions need to
>>>> be pushed down to the executor body. I will try to rework the patch
>>>> as you suggested to see if there are problems.
>>>>
>>>> David
>>>>
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>>>
>>>>>>> The documentation should also link this option to the -fenable/disable
>>>>>>> options as obviously the pass names in that dump are those to be
>>>>>>> used for those flags (and not readily available anywhere else).
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>>>
>>>>>>> I also think that it would be way more useful to note in the individual
>>>>>>> dump files the functions (at the place they would usually appear) that
>>>>>>> have the pass explicitly enabled/disabled.
>>>>>>
>>>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>>>> explicitly disabled.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> Richard.
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
[-- Attachment #2: dump-pass4.p --]
[-- Type: text/x-pascal, Size: 7126 bytes --]
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi (revision 174550)
+++ doc/invoke.texi (working copy)
@@ -291,6 +291,7 @@ Objective-C and Objective-C++ Dialects}.
-fdump-translation-unit@r{[}-@var{n}@r{]} @gol
-fdump-class-hierarchy@r{[}-@var{n}@r{]} @gol
-fdump-ipa-all -fdump-ipa-cgraph -fdump-ipa-inline @gol
+-fdump-passes @gol
-fdump-statistics @gol
-fdump-tree-all @gol
-fdump-tree-original@r{[}-@var{n}@r{]} @gol
@@ -5060,7 +5061,8 @@ seperated list of function ranges. Each
The range is inclusive in both ends. If the range is trivial, the number pair can be
simplified a a single number. If the function's cgraph node's @var{uid} is falling
within one of the specified ranges, the @var{pass} is disabled for that function.
-The @var{uid} is shown in the function header of a dump file.
+The @var{uid} is shown in the function header of a dump file, and pass names can be
+dumped by using option @option{-fdump-passes}.
@item -fdisable-tree-@var{pass}
@item -fdisable-tree-@var{pass}=@var{range-list}
@@ -5483,6 +5485,11 @@ Dump after function inlining.
@end table
+@item -fdump-passes
+@opindex fdump-passes
+Dump the list of optimization passes that are turned on and off by
+the current command line options.
+
@item -fdump-statistics-@var{option}
@opindex fdump-statistics
Enable and control dumping of pass statistics in a separate file. The
Index: tree-pass.h
===================================================================
--- tree-pass.h (revision 174550)
+++ tree-pass.h (working copy)
@@ -639,5 +639,6 @@ extern void do_per_function_toporder (vo
extern void disable_pass (const char *);
extern void enable_pass (const char *);
+extern void dump_passes (void);
#endif /* GCC_TREE_PASS_H */
Index: cgraphunit.c
===================================================================
--- cgraphunit.c (revision 174550)
+++ cgraphunit.c (working copy)
@@ -1112,6 +1112,9 @@ cgraph_finalize_compilation_unit (void)
fflush (stderr);
}
+ if (flag_dump_passes)
+ dump_passes ();
+
/* Gimplify and lower all functions, compute reachability and
remove unreachable nodes. */
cgraph_analyze_functions ();
Index: common.opt
===================================================================
--- common.opt (revision 174550)
+++ common.opt (working copy)
@@ -1012,6 +1012,10 @@ fdump-noaddr
Common Report Var(flag_dump_noaddr)
Suppress output of addresses in debugging dumps
+fdump-passes
+Common Var(flag_dump_passes) Init(0)
+Dump optimization passes
+
fdump-unnumbered
Common Report Var(flag_dump_unnumbered)
Suppress output of instruction numbers, line number notes and addresses in debugging dumps
Index: passes.c
===================================================================
--- passes.c (revision 174550)
+++ passes.c (working copy)
@@ -478,7 +478,7 @@ passr_eq (const void *p1, const void *p2
return !strcmp (s1->unique_name, s2->unique_name);
}
-static htab_t pass_name_tab = NULL;
+static htab_t name_to_pass_map = NULL;
/* Register PASS with NAME. */
@@ -488,11 +488,11 @@ register_pass_name (struct opt_pass *pas
struct pass_registry **slot;
struct pass_registry pr;
- if (!pass_name_tab)
- pass_name_tab = htab_create (256, passr_hash, passr_eq, NULL);
+ if (!name_to_pass_map)
+ name_to_pass_map = htab_create (256, passr_hash, passr_eq, NULL);
pr.unique_name = name;
- slot = (struct pass_registry **) htab_find_slot (pass_name_tab, &pr, INSERT);
+ slot = (struct pass_registry **) htab_find_slot (name_to_pass_map, &pr, INSERT);
if (!*slot)
{
struct pass_registry *new_pr;
@@ -506,6 +506,124 @@ register_pass_name (struct opt_pass *pas
return; /* Ignore plugin passes. */
}
+/* Map from pass id to canonicalized pass name. */
+
+typedef const char *char_ptr;
+DEF_VEC_P(char_ptr);
+DEF_VEC_ALLOC_P(char_ptr, heap);
+static VEC(char_ptr, heap) *pass_tab = NULL;
+
+/* Callback function for traversing NAME_TO_PASS_MAP. */
+
+static int
+pass_traverse (void **slot, void *data ATTRIBUTE_UNUSED)
+{
+ struct pass_registry **p = (struct pass_registry **)slot;
+ struct opt_pass *pass = (*p)->pass;
+
+ gcc_assert (pass->static_pass_number > 0);
+ gcc_assert (pass_tab);
+
+ VEC_replace (char_ptr, pass_tab, pass->static_pass_number,
+ (*p)->unique_name);
+
+ return 1;
+}
+
+/* The function traverses NAME_TO_PASS_MAP and creates a pass info
+ table for dumping purpose. */
+
+static void
+create_pass_tab (void)
+{
+ if (!flag_dump_passes)
+ return;
+
+ VEC_safe_grow_cleared (char_ptr, heap,
+ pass_tab, passes_by_id_size + 1);
+ htab_traverse (name_to_pass_map, pass_traverse, NULL);
+}
+
+static bool override_gate_status (struct opt_pass *, tree, bool);
+
+/* Dump the instantiated name for PASS. IS_ON indicates if PASS
+ is turned on or not. */
+
+static void
+dump_one_pass (struct opt_pass *pass, int pass_indent)
+{
+ int indent = 3 * pass_indent;
+ const char *pn;
+ bool is_on, is_really_on;
+
+ is_on = (pass->gate == NULL) ? true : pass->gate();
+ is_really_on = override_gate_status (pass, current_function_decl, is_on);
+
+ if (pass->static_pass_number <= 0)
+ pn = pass->name;
+ else
+ pn = VEC_index (char_ptr, pass_tab, pass->static_pass_number);
+
+ fprintf (stderr, "%*s%-40s%*s:%s%s\n", indent, " ", pn,
+ (15 - indent < 0 ? 0 : 15 - indent), " ",
+ is_on ? " ON" : " OFF",
+ ((!is_on) == (!is_really_on) ? ""
+ : (is_really_on ? " (FORCED_ON)" : " (FORCED_OFF)")));
+}
+
+/* Dump pass list PASS with indentation INDENT. */
+
+static void
+dump_pass_list (struct opt_pass *pass, int indent)
+{
+ do
+ {
+ dump_one_pass (pass, indent);
+ if (pass->sub)
+ dump_pass_list (pass->sub, indent + 1);
+ pass = pass->next;
+ }
+ while (pass);
+}
+
+/* Dump all optimization passes. */
+
+void
+dump_passes (void)
+{
+ struct cgraph_node *n, *node = NULL;
+ tree save_fndecl = current_function_decl;
+
+ create_pass_tab();
+
+ n = cgraph_nodes;
+ while (n)
+ {
+ if (DECL_STRUCT_FUNCTION (n->decl))
+ {
+ node = n;
+ break;
+ }
+ n = n->next;
+ }
+
+ if (!node)
+ return;
+
+ push_cfun (DECL_STRUCT_FUNCTION (node->decl));
+ current_function_decl = node->decl;
+
+ dump_pass_list (all_lowering_passes, 1);
+ dump_pass_list (all_small_ipa_passes, 1);
+ dump_pass_list (all_regular_ipa_passes, 1);
+ dump_pass_list (all_lto_gen_passes, 1);
+ dump_pass_list (all_passes, 1);
+
+ pop_cfun ();
+ current_function_decl = save_fndecl;
+}
+
+
/* Returns the pass with NAME. */
static struct opt_pass *
@@ -513,9 +631,8 @@ get_pass_by_name (const char *name)
{
struct pass_registry **slot, pr;
- gcc_assert (pass_name_tab);
pr.unique_name = name;
- slot = (struct pass_registry **) htab_find_slot (pass_name_tab,
+ slot = (struct pass_registry **) htab_find_slot (name_to_pass_map,
&pr, NO_INSERT);
if (!slot || !*slot)
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-06 16:00 ` Xinliang David Li
2011-06-06 19:23 ` Xinliang David Li
@ 2011-06-07 10:10 ` Richard Guenther
2011-06-07 16:24 ` Xinliang David Li
1 sibling, 1 reply; 30+ messages in thread
From: Richard Guenther @ 2011-06-07 10:10 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
On Mon, Jun 6, 2011 at 6:00 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> This is the version of the patch that walks through pass lists.
>>>
>>> Ok with this one?
>>
>> +/* Dump all optimization passes. */
>> +
>> +void
>> +dump_passes (void)
>> +{
>> + struct cgraph_node *n, *node = NULL;
>> + tree save_fndecl = current_function_decl;
>> +
>> + fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
>>
>> this isn't accurate info - cloning can cause more cgraph nodes to
>> appear (it also looks completely unrelated to dump_passes ...).
>> Please drop it.
>
> Ok.
>
>
>>
>> + create_pass_tab();
>> + gcc_assert (pass_tab);
>>
>> you have quite many asserts of this kind - we don't want them when
>> the previous stmt as in this case indicates everything is ok.
>
> ok.
>
>>
>> + push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>>
>> this has side-effects, I'm not sure we want this here. Why do you
>> need it? Probably because of
>>
>> + is_really_on = override_gate_status (pass, current_function_decl, is_on);
>>
>> ? But that is dependent on the function given which should have no
>> effect (unless it is overridden globally in which case override_gate_status
>> and friends should deal with a NULL cfun).
>
> As we discussed, currently some pass gate functions depend on per node
> information -- those checks need to be pushed into execute functions.
> I would like to clean those up later -- at which time, the push/pop
> can be removed.
I'd like to do it the other way around, first clean up the gate functions then
drop in this patch without the cfun push/pop. The revised patch looks ok
to me with the cfun push/pop removed.
Thanks,
Richard.
>>
>> I don't understand why you need another table mapping pass to name
>> when pass->name is available and the info is trivially re-constructible.
>
> This is needed as the pass->name is not the canonicalized name (i.e.,
> not with number suffix etc), so the extra mapping from id to
> normalized name is needed.
>
> Thanks,
>
> David
>
>>
>> Thanks,
>> Richard.
>>
>>> David
>>>
>>> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>>>> configuration. The sample output is attached. There is one
>>>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>>>> note registered thus they are not listed. They are not important as
>>>>>>>> they can not be turned on/off anyway.
>>>>>>>>
>>>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>>>> of function assembler names to be specified.
>>>>>>>>
>>>>>>>> Ok for trunk?
>>>>>>>
>>>>>>> Please split the patch.
>>>>>>>
>>>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>>>> at pass execution time when it's not already dumped - that really looks
>>>>>>> gross.
>>>>>>
>>>>>> Yes, that was the original plan -- but it has problems
>>>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>>>> frequently -- it can be a long term maintanance burden;
>>>>>> 2) the centralized dumper needs to be done after option processing
>>>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>>>
>>>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>>>> to do the dumping and tracking indentation.
>>>>>
>>>>> Well, if you have a CU that is empty or optimized to nothing at some point
>>>>> you will not get a complete pass list. I suppose optimize attributes might
>>>>> also confuse output. Your solution might not be that intrusive
>>>>> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
>>>>> dumping from toplev_main before calling do_compile (), 3) gate functions
>>>>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>>>>> your option summary output will be bogus anyway.
>>>>>
>>>>> So - what is the output intended for if it isn't reliable?
>>>>
>>>> This needs to be cleaned up at some point -- the gate function should
>>>> behave the same for all functions and per-function decisions need to
>>>> be pushed down to the executor body. I will try to rework the patch
>>>> as you suggested to see if there are problems.
>>>>
>>>> David
>>>>
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>>>
>>>>>>> The documentation should also link this option to the -fenable/disable
>>>>>>> options as obviously the pass names in that dump are those to be
>>>>>>> used for those flags (and not readily available anywhere else).
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>>>
>>>>>>> I also think that it would be way more useful to note in the individual
>>>>>>> dump files the functions (at the place they would usually appear) that
>>>>>>> have the pass explicitly enabled/disabled.
>>>>>>
>>>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>>>> explicitly disabled.
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> Richard.
>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-06 19:21 ` Xinliang David Li
@ 2011-06-07 10:11 ` Richard Guenther
0 siblings, 0 replies; 30+ messages in thread
From: Richard Guenther @ 2011-06-07 10:11 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
On Mon, Jun 6, 2011 at 9:20 PM, Xinliang David Li <davidxl@google.com> wrote:
> Please take a look at the revised one.
Ok.
Thanks,
Richard.
> Thanks,
>
> David
>
> On Mon, Jun 6, 2011 at 4:22 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Jun 1, 2011 at 7:24 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> The attached is the split #1 patch that enhances -fenable/disable.
>>>
>>> Ok after testing?
>>
>> I expect the testcases will be quite fragile, so while I appreciate
>> test coverage for new options I think we should go without those
>> that involve any kind of UID. Those which use assembler names
>> also will fail randomly dependent on how targets mangle their
>> functions - so I think we have to drop all testcases.
>>
>> Also
>>
>> +/* A helper function to determine if an identifier is valid to
>> + be an assembler name (better to use target specific hook). */
>> +
>> +static bool
>> +is_valid_assembler_name (const char *str)
>> +{
>> + const char *p = str;
>> + char c;
>> +
>> + c = *p;
>> + if (!((c >= 'a' && c <= 'z')
>> + || (c >= 'A' && c <= 'Z')
>> + || *p == '_'))
>> + return false;
>> +
>> + p++;
>> + while ((c = *p))
>> + {
>> + if (!((c >= 'a' && c <= 'z')
>> + || (c >= 'A' && c <= 'Z')
>> + || (c >= '0' && c <= '9')
>> + || *p == '_'))
>> + return false;
>> + p++;
>> + }
>> +
>> + return true;
>> +}
>>
>> why all that complicated checks? Why not just check for p[0]
>> in [^0-9] and re-structure the range parsing to switch between
>> UIDs and assembler-names that way?
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>> David
>>>
>>> On Wed, Jun 1, 2011 at 9:16 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>> configuration. The sample output is attached. There is one
>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>> note registered thus they are not listed. They are not important as
>>>>>> they can not be turned on/off anyway.
>>>>>>
>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>> of function assembler names to be specified.
>>>>>>
>>>>>> Ok for trunk?
>>>>>
>>>>> Please split the patch.
>>>>>
>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>> at pass execution time when it's not already dumped - that really looks
>>>>> gross.
>>>>
>>>> Yes, that was the original plan -- but it has problems
>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>> frequently -- it can be a long term maintanance burden;
>>>> 2) the centralized dumper needs to be done after option processing
>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>
>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>> to do the dumping and tracking indentation.
>>>>
>>>>>
>>>>> The documentation should also link this option to the -fenable/disable
>>>>> options as obviously the pass names in that dump are those to be
>>>>> used for those flags (and not readily available anywhere else).
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>> I also think that it would be way more useful to note in the individual
>>>>> dump files the functions (at the place they would usually appear) that
>>>>> have the pass explicitly enabled/disabled.
>>>>
>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>> explicitly disabled.
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>>>
>>>>> Richard.
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-07 10:10 ` Richard Guenther
@ 2011-06-07 16:24 ` Xinliang David Li
2011-06-07 19:09 ` Xinliang David Li
0 siblings, 1 reply; 30+ messages in thread
From: Xinliang David Li @ 2011-06-07 16:24 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
Ok -- that sounds good.
David
On Tue, Jun 7, 2011 at 3:10 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, Jun 6, 2011 at 6:00 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> This is the version of the patch that walks through pass lists.
>>>>
>>>> Ok with this one?
>>>
>>> +/* Dump all optimization passes. */
>>> +
>>> +void
>>> +dump_passes (void)
>>> +{
>>> + struct cgraph_node *n, *node = NULL;
>>> + tree save_fndecl = current_function_decl;
>>> +
>>> + fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
>>>
>>> this isn't accurate info - cloning can cause more cgraph nodes to
>>> appear (it also looks completely unrelated to dump_passes ...).
>>> Please drop it.
>>
>> Ok.
>>
>>
>>>
>>> + create_pass_tab();
>>> + gcc_assert (pass_tab);
>>>
>>> you have quite many asserts of this kind - we don't want them when
>>> the previous stmt as in this case indicates everything is ok.
>>
>> ok.
>>
>>>
>>> + push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>>>
>>> this has side-effects, I'm not sure we want this here. Why do you
>>> need it? Probably because of
>>>
>>> + is_really_on = override_gate_status (pass, current_function_decl, is_on);
>>>
>>> ? But that is dependent on the function given which should have no
>>> effect (unless it is overridden globally in which case override_gate_status
>>> and friends should deal with a NULL cfun).
>>
>> As we discussed, currently some pass gate functions depend on per node
>> information -- those checks need to be pushed into execute functions.
>> I would like to clean those up later -- at which time, the push/pop
>> can be removed.
>
> I'd like to do it the other way around, first clean up the gate functions then
> drop in this patch without the cfun push/pop. The revised patch looks ok
> to me with the cfun push/pop removed.
>
> Thanks,
> Richard.
>
>>>
>>> I don't understand why you need another table mapping pass to name
>>> when pass->name is available and the info is trivially re-constructible.
>>
>> This is needed as the pass->name is not the canonicalized name (i.e.,
>> not with number suffix etc), so the extra mapping from id to
>> normalized name is needed.
>>
>> Thanks,
>>
>> David
>>
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> David
>>>>
>>>> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>>>>> configuration. The sample output is attached. There is one
>>>>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>>>>> note registered thus they are not listed. They are not important as
>>>>>>>>> they can not be turned on/off anyway.
>>>>>>>>>
>>>>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>>>>> of function assembler names to be specified.
>>>>>>>>>
>>>>>>>>> Ok for trunk?
>>>>>>>>
>>>>>>>> Please split the patch.
>>>>>>>>
>>>>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>>>>> at pass execution time when it's not already dumped - that really looks
>>>>>>>> gross.
>>>>>>>
>>>>>>> Yes, that was the original plan -- but it has problems
>>>>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>>>>> frequently -- it can be a long term maintanance burden;
>>>>>>> 2) the centralized dumper needs to be done after option processing
>>>>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>>>>
>>>>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>>>>> to do the dumping and tracking indentation.
>>>>>>
>>>>>> Well, if you have a CU that is empty or optimized to nothing at some point
>>>>>> you will not get a complete pass list. I suppose optimize attributes might
>>>>>> also confuse output. Your solution might not be that intrusive
>>>>>> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
>>>>>> dumping from toplev_main before calling do_compile (), 3) gate functions
>>>>>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>>>>>> your option summary output will be bogus anyway.
>>>>>>
>>>>>> So - what is the output intended for if it isn't reliable?
>>>>>
>>>>> This needs to be cleaned up at some point -- the gate function should
>>>>> behave the same for all functions and per-function decisions need to
>>>>> be pushed down to the executor body. I will try to rework the patch
>>>>> as you suggested to see if there are problems.
>>>>>
>>>>> David
>>>>>
>>>>>
>>>>>>
>>>>>> Richard.
>>>>>>
>>>>>>>>
>>>>>>>> The documentation should also link this option to the -fenable/disable
>>>>>>>> options as obviously the pass names in that dump are those to be
>>>>>>>> used for those flags (and not readily available anywhere else).
>>>>>>>
>>>>>>> Ok.
>>>>>>>
>>>>>>>>
>>>>>>>> I also think that it would be way more useful to note in the individual
>>>>>>>> dump files the functions (at the place they would usually appear) that
>>>>>>>> have the pass explicitly enabled/disabled.
>>>>>>>
>>>>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>>>>> explicitly disabled.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>>
>>>>>>>> Richard.
>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-07 16:24 ` Xinliang David Li
@ 2011-06-07 19:09 ` Xinliang David Li
2011-06-07 20:39 ` Xinliang David Li
` (2 more replies)
0 siblings, 3 replies; 30+ messages in thread
From: Xinliang David Li @ 2011-06-07 19:09 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 6935 bytes --]
Please review the attached two patches.
In the first patch, gate functions are cleaned up. All the per
function legality checks are moved into the executor and the
optimization heuristic checks (optimize for size) remain in the
gators. These allow the the following overriding order:
common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
options <--- legality check
Testing under going. Ok for trunk?
Thanks,
David
On Tue, Jun 7, 2011 at 9:24 AM, Xinliang David Li <davidxl@google.com> wrote:
> Ok -- that sounds good.
>
> David
>
> On Tue, Jun 7, 2011 at 3:10 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Mon, Jun 6, 2011 at 6:00 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> This is the version of the patch that walks through pass lists.
>>>>>
>>>>> Ok with this one?
>>>>
>>>> +/* Dump all optimization passes. */
>>>> +
>>>> +void
>>>> +dump_passes (void)
>>>> +{
>>>> + struct cgraph_node *n, *node = NULL;
>>>> + tree save_fndecl = current_function_decl;
>>>> +
>>>> + fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
>>>>
>>>> this isn't accurate info - cloning can cause more cgraph nodes to
>>>> appear (it also looks completely unrelated to dump_passes ...).
>>>> Please drop it.
>>>
>>> Ok.
>>>
>>>
>>>>
>>>> + create_pass_tab();
>>>> + gcc_assert (pass_tab);
>>>>
>>>> you have quite many asserts of this kind - we don't want them when
>>>> the previous stmt as in this case indicates everything is ok.
>>>
>>> ok.
>>>
>>>>
>>>> + push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>>>>
>>>> this has side-effects, I'm not sure we want this here. Why do you
>>>> need it? Probably because of
>>>>
>>>> + is_really_on = override_gate_status (pass, current_function_decl, is_on);
>>>>
>>>> ? But that is dependent on the function given which should have no
>>>> effect (unless it is overridden globally in which case override_gate_status
>>>> and friends should deal with a NULL cfun).
>>>
>>> As we discussed, currently some pass gate functions depend on per node
>>> information -- those checks need to be pushed into execute functions.
>>> I would like to clean those up later -- at which time, the push/pop
>>> can be removed.
>>
>> I'd like to do it the other way around, first clean up the gate functions then
>> drop in this patch without the cfun push/pop. The revised patch looks ok
>> to me with the cfun push/pop removed.
>>
>> Thanks,
>> Richard.
>>
>>>>
>>>> I don't understand why you need another table mapping pass to name
>>>> when pass->name is available and the info is trivially re-constructible.
>>>
>>> This is needed as the pass->name is not the canonicalized name (i.e.,
>>> not with number suffix etc), so the extra mapping from id to
>>> normalized name is needed.
>>>
>>> Thanks,
>>>
>>> David
>>>
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> David
>>>>>
>>>>> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>>>>>> configuration. The sample output is attached. There is one
>>>>>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>>>>>> note registered thus they are not listed. They are not important as
>>>>>>>>>> they can not be turned on/off anyway.
>>>>>>>>>>
>>>>>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>>>>>> of function assembler names to be specified.
>>>>>>>>>>
>>>>>>>>>> Ok for trunk?
>>>>>>>>>
>>>>>>>>> Please split the patch.
>>>>>>>>>
>>>>>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>>>>>> at pass execution time when it's not already dumped - that really looks
>>>>>>>>> gross.
>>>>>>>>
>>>>>>>> Yes, that was the original plan -- but it has problems
>>>>>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>>>>>> frequently -- it can be a long term maintanance burden;
>>>>>>>> 2) the centralized dumper needs to be done after option processing
>>>>>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>>>>>
>>>>>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>>>>>> to do the dumping and tracking indentation.
>>>>>>>
>>>>>>> Well, if you have a CU that is empty or optimized to nothing at some point
>>>>>>> you will not get a complete pass list. I suppose optimize attributes might
>>>>>>> also confuse output. Your solution might not be that intrusive
>>>>>>> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
>>>>>>> dumping from toplev_main before calling do_compile (), 3) gate functions
>>>>>>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>>>>>>> your option summary output will be bogus anyway.
>>>>>>>
>>>>>>> So - what is the output intended for if it isn't reliable?
>>>>>>
>>>>>> This needs to be cleaned up at some point -- the gate function should
>>>>>> behave the same for all functions and per-function decisions need to
>>>>>> be pushed down to the executor body. I will try to rework the patch
>>>>>> as you suggested to see if there are problems.
>>>>>>
>>>>>> David
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Richard.
>>>>>>>
>>>>>>>>>
>>>>>>>>> The documentation should also link this option to the -fenable/disable
>>>>>>>>> options as obviously the pass names in that dump are those to be
>>>>>>>>> used for those flags (and not readily available anywhere else).
>>>>>>>>
>>>>>>>> Ok.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> I also think that it would be way more useful to note in the individual
>>>>>>>>> dump files the functions (at the place they would usually appear) that
>>>>>>>>> have the pass explicitly enabled/disabled.
>>>>>>>>
>>>>>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>>>>>> explicitly disabled.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Richard.
>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
[-- Attachment #2: gate_cleanup.p --]
[-- Type: text/x-pascal, Size: 6445 bytes --]
Index: tree-complex.c
===================================================================
--- tree-complex.c (revision 174759)
+++ tree-complex.c (working copy)
@@ -1569,6 +1569,11 @@ tree_lower_complex (void)
gimple_stmt_iterator gsi;
basic_block bb;
+ /* With errors, normal optimization passes are not run. If we don't
+ lower complex operations at all, rtl expansion will abort. */
+ if (cfun->curr_properties & PROP_gimple_lcx)
+ return 0;
+
if (!init_dont_simulate_again ())
return 0;
@@ -1634,9 +1639,7 @@ struct gimple_opt_pass pass_lower_comple
static bool
gate_no_optimization (void)
{
- /* With errors, normal optimization passes are not run. If we don't
- lower complex operations at all, rtl expansion will abort. */
- return !(cfun->curr_properties & PROP_gimple_lcx);
+ return true;
}
struct gimple_opt_pass pass_lower_complex_O0 =
Index: tree-stdarg.c
===================================================================
--- tree-stdarg.c (revision 174759)
+++ tree-stdarg.c (working copy)
@@ -627,8 +627,7 @@ check_all_va_list_escapes (struct stdarg
static bool
gate_optimize_stdarg (void)
{
- /* This optimization is only for stdarg functions. */
- return cfun->stdarg != 0;
+ return true;
}
@@ -645,6 +644,10 @@ execute_optimize_stdarg (void)
const char *funcname = NULL;
tree cfun_va_list;
+ /* This optimization is only for stdarg functions. */
+ if (cfun->stdarg == 0)
+ return 0;
+
cfun->va_list_gpr_size = 0;
cfun->va_list_fpr_size = 0;
memset (&si, 0, sizeof (si));
Index: tree-eh.c
===================================================================
--- tree-eh.c (revision 174759)
+++ tree-eh.c (working copy)
@@ -3234,6 +3234,9 @@ execute_lower_eh_dispatch (void)
bool any_rewritten = false;
bool redirected = false;
+ if (cfun->eh->region_tree == NULL)
+ return 0;
+
assign_filter_values ();
FOR_EACH_BB (bb)
@@ -3254,7 +3257,7 @@ execute_lower_eh_dispatch (void)
static bool
gate_lower_eh_dispatch (void)
{
- return cfun->eh->region_tree != NULL;
+ return true;
}
struct gimple_opt_pass pass_lower_eh_dispatch =
@@ -3983,8 +3986,12 @@ execute_cleanup_eh_1 (void)
static unsigned int
execute_cleanup_eh (void)
{
- int ret = execute_cleanup_eh_1 ();
+ int ret;
+ if (cfun->eh == NULL || cfun->eh->region_tree == NULL)
+ return 0;
+
+ ret = execute_cleanup_eh_1 ();
/* If the function no longer needs an EH personality routine
clear it. This exposes cross-language inlining opportunities
and avoids references to a never defined personality routine. */
@@ -3998,7 +4005,7 @@ execute_cleanup_eh (void)
static bool
gate_cleanup_eh (void)
{
- return cfun->eh != NULL && cfun->eh->region_tree != NULL;
+ return true;
}
struct gimple_opt_pass pass_cleanup_eh = {
Index: gcse.c
===================================================================
--- gcse.c (revision 174759)
+++ gcse.c (working copy)
@@ -3713,15 +3713,17 @@ static bool
gate_rtl_pre (void)
{
return optimize > 0 && flag_gcse
- && !cfun->calls_setjmp
- && optimize_function_for_speed_p (cfun)
- && dbg_cnt (pre);
+ && optimize_function_for_speed_p (cfun);
}
static unsigned int
execute_rtl_pre (void)
{
int changed;
+
+ if (cfun->calls_setjmp || !dbg_cnt (pre))
+ return 0;
+
delete_unreachable_blocks ();
df_analyze ();
changed = one_pre_gcse_pass ();
@@ -3735,18 +3737,20 @@ static bool
gate_rtl_hoist (void)
{
return optimize > 0 && flag_gcse
- && !cfun->calls_setjmp
- /* It does not make sense to run code hoisting unless we are optimizing
- for code size -- it rarely makes programs faster, and can make then
- bigger if we did PRE (when optimizing for space, we don't run PRE). */
- && optimize_function_for_size_p (cfun)
- && dbg_cnt (hoist);
+ /* It does not make sense to run code hoisting unless we are optimizing
+ for code size -- it rarely makes programs faster, and can make then
+ bigger if we did PRE (when optimizing for space, we don't run PRE). */
+ && optimize_function_for_size_p (cfun);
}
static unsigned int
execute_rtl_hoist (void)
{
int changed;
+
+ if (cfun->calls_setjmp || !dbg_cnt (hoist))
+ return 0;
+
delete_unreachable_blocks ();
df_analyze ();
changed = one_code_hoisting_pass ();
@@ -3799,4 +3803,3 @@ struct rtl_opt_pass pass_rtl_hoist =
};
#include "gt-gcse.h"
-
Index: except.c
===================================================================
--- except.c (revision 174759)
+++ except.c (working copy)
@@ -1440,14 +1440,17 @@ finish_eh_generation (void)
static bool
gate_handle_eh (void)
{
- /* Nothing to do if no regions created. */
- return cfun->eh->region_tree != NULL;
+ return true;
}
/* Complete generation of exception handling code. */
static unsigned int
rest_of_handle_eh (void)
{
+ /* Nothing to do if no regions created. */
+ if (cfun->eh->region_tree == NULL)
+ return 0;
+
finish_eh_generation ();
cleanup_cfg (CLEANUP_NO_INSN_DEL);
return 0;
@@ -2392,6 +2395,9 @@ convert_to_eh_region_ranges (void)
int min_labelno = 0, max_labelno = 0;
int saved_call_site_base = call_site_base;
+ if (cfun->eh->region_tree == NULL)
+ return 0;
+
crtl->eh.action_record_data = VEC_alloc (uchar, gc, 64);
ar_hash = htab_create (31, action_record_hash, action_record_eq, free);
@@ -2643,8 +2649,6 @@ static bool
gate_convert_to_eh_region_ranges (void)
{
/* Nothing to do for SJLJ exceptions or if no regions created. */
- if (cfun->eh->region_tree == NULL)
- return false;
if (targetm.except_unwind_info (&global_options) == UI_SJLJ)
return false;
return true;
Index: cprop.c
===================================================================
--- cprop.c (revision 174759)
+++ cprop.c (working copy)
@@ -1843,15 +1843,17 @@ one_cprop_pass (void)
static bool
gate_rtl_cprop (void)
{
- return optimize > 0 && flag_gcse
- && !cfun->calls_setjmp
- && dbg_cnt (cprop);
+ return optimize > 0 && flag_gcse;
}
static unsigned int
execute_rtl_cprop (void)
{
int changed;
+
+ if (cfun->calls_setjmp || !dbg_cnt (cprop))
+ return 0;
+
delete_unreachable_blocks ();
df_set_flags (DF_LR_RUN_DCE);
df_analyze ();
@@ -1882,4 +1884,3 @@ struct rtl_opt_pass pass_rtl_cprop =
TODO_verify_flow | TODO_ggc_collect /* todo_flags_finish */
}
};
-
[-- Attachment #3: dump-pass5.p --]
[-- Type: text/x-pascal, Size: 6670 bytes --]
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi (revision 174759)
+++ doc/invoke.texi (working copy)
@@ -291,6 +291,7 @@ Objective-C and Objective-C++ Dialects}.
-fdump-translation-unit@r{[}-@var{n}@r{]} @gol
-fdump-class-hierarchy@r{[}-@var{n}@r{]} @gol
-fdump-ipa-all -fdump-ipa-cgraph -fdump-ipa-inline @gol
+-fdump-passes @gol
-fdump-statistics @gol
-fdump-tree-all @gol
-fdump-tree-original@r{[}-@var{n}@r{]} @gol
@@ -5069,7 +5070,8 @@ seperated list of function ranges. Each
The range is inclusive in both ends. If the range is trivial, the number pair can be
simplified a a single number. If the function's cgraph node's @var{uid} is falling
within one of the specified ranges, the @var{pass} is disabled for that function.
-The @var{uid} is shown in the function header of a dump file.
+The @var{uid} is shown in the function header of a dump file, and pass names can be
+dumped by using option @option{-fdump-passes}.
@item -fdisable-tree-@var{pass}
@item -fdisable-tree-@var{pass}=@var{range-list}
@@ -5492,6 +5494,11 @@ Dump after function inlining.
@end table
+@item -fdump-passes
+@opindex fdump-passes
+Dump the list of optimization passes that are turned on and off by
+the current command line options.
+
@item -fdump-statistics-@var{option}
@opindex fdump-statistics
Enable and control dumping of pass statistics in a separate file. The
Index: tree-pass.h
===================================================================
--- tree-pass.h (revision 174759)
+++ tree-pass.h (working copy)
@@ -639,5 +639,6 @@ extern void do_per_function_toporder (vo
extern void disable_pass (const char *);
extern void enable_pass (const char *);
+extern void dump_passes (void);
#endif /* GCC_TREE_PASS_H */
Index: cgraphunit.c
===================================================================
--- cgraphunit.c (revision 174759)
+++ cgraphunit.c (working copy)
@@ -1117,6 +1117,9 @@ cgraph_finalize_compilation_unit (void)
fflush (stderr);
}
+ if (flag_dump_passes)
+ dump_passes ();
+
/* Gimplify and lower all functions, compute reachability and
remove unreachable nodes. */
cgraph_analyze_functions ();
Index: common.opt
===================================================================
--- common.opt (revision 174759)
+++ common.opt (working copy)
@@ -1012,6 +1012,10 @@ fdump-noaddr
Common Report Var(flag_dump_noaddr)
Suppress output of addresses in debugging dumps
+fdump-passes
+Common Var(flag_dump_passes) Init(0)
+Dump optimization passes
+
fdump-unnumbered
Common Report Var(flag_dump_unnumbered)
Suppress output of instruction numbers, line number notes and addresses in debugging dumps
Index: passes.c
===================================================================
--- passes.c (revision 174759)
+++ passes.c (working copy)
@@ -478,7 +478,7 @@ passr_eq (const void *p1, const void *p2
return !strcmp (s1->unique_name, s2->unique_name);
}
-static htab_t pass_name_tab = NULL;
+static htab_t name_to_pass_map = NULL;
/* Register PASS with NAME. */
@@ -488,11 +488,11 @@ register_pass_name (struct opt_pass *pas
struct pass_registry **slot;
struct pass_registry pr;
- if (!pass_name_tab)
- pass_name_tab = htab_create (256, passr_hash, passr_eq, NULL);
+ if (!name_to_pass_map)
+ name_to_pass_map = htab_create (256, passr_hash, passr_eq, NULL);
pr.unique_name = name;
- slot = (struct pass_registry **) htab_find_slot (pass_name_tab, &pr, INSERT);
+ slot = (struct pass_registry **) htab_find_slot (name_to_pass_map, &pr, INSERT);
if (!*slot)
{
struct pass_registry *new_pr;
@@ -506,6 +506,101 @@ register_pass_name (struct opt_pass *pas
return; /* Ignore plugin passes. */
}
+/* Map from pass id to canonicalized pass name. */
+
+typedef const char *char_ptr;
+DEF_VEC_P(char_ptr);
+DEF_VEC_ALLOC_P(char_ptr, heap);
+static VEC(char_ptr, heap) *pass_tab = NULL;
+
+/* Callback function for traversing NAME_TO_PASS_MAP. */
+
+static int
+pass_traverse (void **slot, void *data ATTRIBUTE_UNUSED)
+{
+ struct pass_registry **p = (struct pass_registry **)slot;
+ struct opt_pass *pass = (*p)->pass;
+
+ gcc_assert (pass->static_pass_number > 0);
+ gcc_assert (pass_tab);
+
+ VEC_replace (char_ptr, pass_tab, pass->static_pass_number,
+ (*p)->unique_name);
+
+ return 1;
+}
+
+/* The function traverses NAME_TO_PASS_MAP and creates a pass info
+ table for dumping purpose. */
+
+static void
+create_pass_tab (void)
+{
+ if (!flag_dump_passes)
+ return;
+
+ VEC_safe_grow_cleared (char_ptr, heap,
+ pass_tab, passes_by_id_size + 1);
+ htab_traverse (name_to_pass_map, pass_traverse, NULL);
+}
+
+static bool override_gate_status (struct opt_pass *, tree, bool);
+
+/* Dump the instantiated name for PASS. IS_ON indicates if PASS
+ is turned on or not. */
+
+static void
+dump_one_pass (struct opt_pass *pass, int pass_indent)
+{
+ int indent = 3 * pass_indent;
+ const char *pn;
+ bool is_on, is_really_on;
+
+ is_on = (pass->gate == NULL) ? true : pass->gate();
+ is_really_on = override_gate_status (pass, NULL, is_on);
+
+ if (pass->static_pass_number <= 0)
+ pn = pass->name;
+ else
+ pn = VEC_index (char_ptr, pass_tab, pass->static_pass_number);
+
+ fprintf (stderr, "%*s%-40s%*s:%s%s\n", indent, " ", pn,
+ (15 - indent < 0 ? 0 : 15 - indent), " ",
+ is_on ? " ON" : " OFF",
+ ((!is_on) == (!is_really_on) ? ""
+ : (is_really_on ? " (FORCED_ON)" : " (FORCED_OFF)")));
+}
+
+/* Dump pass list PASS with indentation INDENT. */
+
+static void
+dump_pass_list (struct opt_pass *pass, int indent)
+{
+ do
+ {
+ dump_one_pass (pass, indent);
+ if (pass->sub)
+ dump_pass_list (pass->sub, indent + 1);
+ pass = pass->next;
+ }
+ while (pass);
+}
+
+/* Dump all optimization passes. */
+
+void
+dump_passes (void)
+{
+ create_pass_tab();
+
+ dump_pass_list (all_lowering_passes, 1);
+ dump_pass_list (all_small_ipa_passes, 1);
+ dump_pass_list (all_regular_ipa_passes, 1);
+ dump_pass_list (all_lto_gen_passes, 1);
+ dump_pass_list (all_passes, 1);
+}
+
+
/* Returns the pass with NAME. */
static struct opt_pass *
@@ -513,9 +608,8 @@ get_pass_by_name (const char *name)
{
struct pass_registry **slot, pr;
- gcc_assert (pass_name_tab);
pr.unique_name = name;
- slot = (struct pass_registry **) htab_find_slot (pass_name_tab,
+ slot = (struct pass_registry **) htab_find_slot (name_to_pass_map,
&pr, NO_INSERT);
if (!slot || !*slot)
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-07 19:09 ` Xinliang David Li
@ 2011-06-07 20:39 ` Xinliang David Li
2011-06-08 9:06 ` Richard Guenther
2011-06-08 8:54 ` Richard Guenther
2011-06-09 22:16 ` H.J. Lu
2 siblings, 1 reply; 30+ messages in thread
From: Xinliang David Li @ 2011-06-07 20:39 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 7267 bytes --]
The dump-pass patch with test case.
David
On Tue, Jun 7, 2011 at 11:54 AM, Xinliang David Li <davidxl@google.com> wrote:
> Please review the attached two patches.
>
> In the first patch, gate functions are cleaned up. All the per
> function legality checks are moved into the executor and the
> optimization heuristic checks (optimize for size) remain in the
> gators. These allow the the following overriding order:
>
> common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
> heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
> options <--- legality check
>
> Testing under going. Ok for trunk?
>
> Thanks,
>
> David
>
> On Tue, Jun 7, 2011 at 9:24 AM, Xinliang David Li <davidxl@google.com> wrote:
>> Ok -- that sounds good.
>>
>> David
>>
>> On Tue, Jun 7, 2011 at 3:10 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Mon, Jun 6, 2011 at 6:00 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> This is the version of the patch that walks through pass lists.
>>>>>>
>>>>>> Ok with this one?
>>>>>
>>>>> +/* Dump all optimization passes. */
>>>>> +
>>>>> +void
>>>>> +dump_passes (void)
>>>>> +{
>>>>> + struct cgraph_node *n, *node = NULL;
>>>>> + tree save_fndecl = current_function_decl;
>>>>> +
>>>>> + fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
>>>>>
>>>>> this isn't accurate info - cloning can cause more cgraph nodes to
>>>>> appear (it also looks completely unrelated to dump_passes ...).
>>>>> Please drop it.
>>>>
>>>> Ok.
>>>>
>>>>
>>>>>
>>>>> + create_pass_tab();
>>>>> + gcc_assert (pass_tab);
>>>>>
>>>>> you have quite many asserts of this kind - we don't want them when
>>>>> the previous stmt as in this case indicates everything is ok.
>>>>
>>>> ok.
>>>>
>>>>>
>>>>> + push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>>>>>
>>>>> this has side-effects, I'm not sure we want this here. Why do you
>>>>> need it? Probably because of
>>>>>
>>>>> + is_really_on = override_gate_status (pass, current_function_decl, is_on);
>>>>>
>>>>> ? But that is dependent on the function given which should have no
>>>>> effect (unless it is overridden globally in which case override_gate_status
>>>>> and friends should deal with a NULL cfun).
>>>>
>>>> As we discussed, currently some pass gate functions depend on per node
>>>> information -- those checks need to be pushed into execute functions.
>>>> I would like to clean those up later -- at which time, the push/pop
>>>> can be removed.
>>>
>>> I'd like to do it the other way around, first clean up the gate functions then
>>> drop in this patch without the cfun push/pop. The revised patch looks ok
>>> to me with the cfun push/pop removed.
>>>
>>> Thanks,
>>> Richard.
>>>
>>>>>
>>>>> I don't understand why you need another table mapping pass to name
>>>>> when pass->name is available and the info is trivially re-constructible.
>>>>
>>>> This is needed as the pass->name is not the canonicalized name (i.e.,
>>>> not with number suffix etc), so the extra mapping from id to
>>>> normalized name is needed.
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>>>
>>>>> Thanks,
>>>>> Richard.
>>>>>
>>>>>> David
>>>>>>
>>>>>> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>>>>>>> configuration. The sample output is attached. There is one
>>>>>>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>>>>>>> note registered thus they are not listed. They are not important as
>>>>>>>>>>> they can not be turned on/off anyway.
>>>>>>>>>>>
>>>>>>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>>>>>>> of function assembler names to be specified.
>>>>>>>>>>>
>>>>>>>>>>> Ok for trunk?
>>>>>>>>>>
>>>>>>>>>> Please split the patch.
>>>>>>>>>>
>>>>>>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>>>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>>>>>>> at pass execution time when it's not already dumped - that really looks
>>>>>>>>>> gross.
>>>>>>>>>
>>>>>>>>> Yes, that was the original plan -- but it has problems
>>>>>>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>>>>>>> frequently -- it can be a long term maintanance burden;
>>>>>>>>> 2) the centralized dumper needs to be done after option processing
>>>>>>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>>>>>>
>>>>>>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>>>>>>> to do the dumping and tracking indentation.
>>>>>>>>
>>>>>>>> Well, if you have a CU that is empty or optimized to nothing at some point
>>>>>>>> you will not get a complete pass list. I suppose optimize attributes might
>>>>>>>> also confuse output. Your solution might not be that intrusive
>>>>>>>> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
>>>>>>>> dumping from toplev_main before calling do_compile (), 3) gate functions
>>>>>>>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>>>>>>>> your option summary output will be bogus anyway.
>>>>>>>>
>>>>>>>> So - what is the output intended for if it isn't reliable?
>>>>>>>
>>>>>>> This needs to be cleaned up at some point -- the gate function should
>>>>>>> behave the same for all functions and per-function decisions need to
>>>>>>> be pushed down to the executor body. I will try to rework the patch
>>>>>>> as you suggested to see if there are problems.
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Richard.
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The documentation should also link this option to the -fenable/disable
>>>>>>>>>> options as obviously the pass names in that dump are those to be
>>>>>>>>>> used for those flags (and not readily available anywhere else).
>>>>>>>>>
>>>>>>>>> Ok.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I also think that it would be way more useful to note in the individual
>>>>>>>>>> dump files the functions (at the place they would usually appear) that
>>>>>>>>>> have the pass explicitly enabled/disabled.
>>>>>>>>>
>>>>>>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>>>>>>> explicitly disabled.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Richard.
>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> David
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
[-- Attachment #2: dump-pass6.p --]
[-- Type: text/x-pascal, Size: 7106 bytes --]
Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi (revision 174759)
+++ doc/invoke.texi (working copy)
@@ -291,6 +291,7 @@ Objective-C and Objective-C++ Dialects}.
-fdump-translation-unit@r{[}-@var{n}@r{]} @gol
-fdump-class-hierarchy@r{[}-@var{n}@r{]} @gol
-fdump-ipa-all -fdump-ipa-cgraph -fdump-ipa-inline @gol
+-fdump-passes @gol
-fdump-statistics @gol
-fdump-tree-all @gol
-fdump-tree-original@r{[}-@var{n}@r{]} @gol
@@ -5069,7 +5070,8 @@ seperated list of function ranges. Each
The range is inclusive in both ends. If the range is trivial, the number pair can be
simplified a a single number. If the function's cgraph node's @var{uid} is falling
within one of the specified ranges, the @var{pass} is disabled for that function.
-The @var{uid} is shown in the function header of a dump file.
+The @var{uid} is shown in the function header of a dump file, and pass names can be
+dumped by using option @option{-fdump-passes}.
@item -fdisable-tree-@var{pass}
@item -fdisable-tree-@var{pass}=@var{range-list}
@@ -5492,6 +5494,11 @@ Dump after function inlining.
@end table
+@item -fdump-passes
+@opindex fdump-passes
+Dump the list of optimization passes that are turned on and off by
+the current command line options.
+
@item -fdump-statistics-@var{option}
@opindex fdump-statistics
Enable and control dumping of pass statistics in a separate file. The
Index: tree-pass.h
===================================================================
--- tree-pass.h (revision 174759)
+++ tree-pass.h (working copy)
@@ -639,5 +639,6 @@ extern void do_per_function_toporder (vo
extern void disable_pass (const char *);
extern void enable_pass (const char *);
+extern void dump_passes (void);
#endif /* GCC_TREE_PASS_H */
Index: cgraphunit.c
===================================================================
--- cgraphunit.c (revision 174759)
+++ cgraphunit.c (working copy)
@@ -1117,6 +1117,9 @@ cgraph_finalize_compilation_unit (void)
fflush (stderr);
}
+ if (flag_dump_passes)
+ dump_passes ();
+
/* Gimplify and lower all functions, compute reachability and
remove unreachable nodes. */
cgraph_analyze_functions ();
Index: common.opt
===================================================================
--- common.opt (revision 174759)
+++ common.opt (working copy)
@@ -1012,6 +1012,10 @@ fdump-noaddr
Common Report Var(flag_dump_noaddr)
Suppress output of addresses in debugging dumps
+fdump-passes
+Common Var(flag_dump_passes) Init(0)
+Dump optimization passes
+
fdump-unnumbered
Common Report Var(flag_dump_unnumbered)
Suppress output of instruction numbers, line number notes and addresses in debugging dumps
Index: passes.c
===================================================================
--- passes.c (revision 174759)
+++ passes.c (working copy)
@@ -478,7 +478,7 @@ passr_eq (const void *p1, const void *p2
return !strcmp (s1->unique_name, s2->unique_name);
}
-static htab_t pass_name_tab = NULL;
+static htab_t name_to_pass_map = NULL;
/* Register PASS with NAME. */
@@ -488,11 +488,11 @@ register_pass_name (struct opt_pass *pas
struct pass_registry **slot;
struct pass_registry pr;
- if (!pass_name_tab)
- pass_name_tab = htab_create (256, passr_hash, passr_eq, NULL);
+ if (!name_to_pass_map)
+ name_to_pass_map = htab_create (256, passr_hash, passr_eq, NULL);
pr.unique_name = name;
- slot = (struct pass_registry **) htab_find_slot (pass_name_tab, &pr, INSERT);
+ slot = (struct pass_registry **) htab_find_slot (name_to_pass_map, &pr, INSERT);
if (!*slot)
{
struct pass_registry *new_pr;
@@ -506,6 +506,101 @@ register_pass_name (struct opt_pass *pas
return; /* Ignore plugin passes. */
}
+/* Map from pass id to canonicalized pass name. */
+
+typedef const char *char_ptr;
+DEF_VEC_P(char_ptr);
+DEF_VEC_ALLOC_P(char_ptr, heap);
+static VEC(char_ptr, heap) *pass_tab = NULL;
+
+/* Callback function for traversing NAME_TO_PASS_MAP. */
+
+static int
+pass_traverse (void **slot, void *data ATTRIBUTE_UNUSED)
+{
+ struct pass_registry **p = (struct pass_registry **)slot;
+ struct opt_pass *pass = (*p)->pass;
+
+ gcc_assert (pass->static_pass_number > 0);
+ gcc_assert (pass_tab);
+
+ VEC_replace (char_ptr, pass_tab, pass->static_pass_number,
+ (*p)->unique_name);
+
+ return 1;
+}
+
+/* The function traverses NAME_TO_PASS_MAP and creates a pass info
+ table for dumping purpose. */
+
+static void
+create_pass_tab (void)
+{
+ if (!flag_dump_passes)
+ return;
+
+ VEC_safe_grow_cleared (char_ptr, heap,
+ pass_tab, passes_by_id_size + 1);
+ htab_traverse (name_to_pass_map, pass_traverse, NULL);
+}
+
+static bool override_gate_status (struct opt_pass *, tree, bool);
+
+/* Dump the instantiated name for PASS. IS_ON indicates if PASS
+ is turned on or not. */
+
+static void
+dump_one_pass (struct opt_pass *pass, int pass_indent)
+{
+ int indent = 3 * pass_indent;
+ const char *pn;
+ bool is_on, is_really_on;
+
+ is_on = (pass->gate == NULL) ? true : pass->gate();
+ is_really_on = override_gate_status (pass, NULL, is_on);
+
+ if (pass->static_pass_number <= 0)
+ pn = pass->name;
+ else
+ pn = VEC_index (char_ptr, pass_tab, pass->static_pass_number);
+
+ fprintf (stderr, "%*s%-40s%*s:%s%s\n", indent, " ", pn,
+ (15 - indent < 0 ? 0 : 15 - indent), " ",
+ is_on ? " ON" : " OFF",
+ ((!is_on) == (!is_really_on) ? ""
+ : (is_really_on ? " (FORCED_ON)" : " (FORCED_OFF)")));
+}
+
+/* Dump pass list PASS with indentation INDENT. */
+
+static void
+dump_pass_list (struct opt_pass *pass, int indent)
+{
+ do
+ {
+ dump_one_pass (pass, indent);
+ if (pass->sub)
+ dump_pass_list (pass->sub, indent + 1);
+ pass = pass->next;
+ }
+ while (pass);
+}
+
+/* Dump all optimization passes. */
+
+void
+dump_passes (void)
+{
+ create_pass_tab();
+
+ dump_pass_list (all_lowering_passes, 1);
+ dump_pass_list (all_small_ipa_passes, 1);
+ dump_pass_list (all_regular_ipa_passes, 1);
+ dump_pass_list (all_lto_gen_passes, 1);
+ dump_pass_list (all_passes, 1);
+}
+
+
/* Returns the pass with NAME. */
static struct opt_pass *
@@ -513,9 +608,8 @@ get_pass_by_name (const char *name)
{
struct pass_registry **slot, pr;
- gcc_assert (pass_name_tab);
pr.unique_name = name;
- slot = (struct pass_registry **) htab_find_slot (pass_name_tab,
+ slot = (struct pass_registry **) htab_find_slot (name_to_pass_map,
&pr, NO_INSERT);
if (!slot || !*slot)
Index: testsuite/gcc.dg/dump-pass.c
===================================================================
--- testsuite/gcc.dg/dump-pass.c (revision 0)
+++ testsuite/gcc.dg/dump-pass.c (revision 0)
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-passes" } */
+
+unsigned res;
+
+void
+foo (unsigned code, int len)
+{
+ int i;
+ for (i = 0; i < len; i++)
+ res |= code & 1;
+}
+
+/* { dg-prune-output ".*" } */
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-07 19:09 ` Xinliang David Li
2011-06-07 20:39 ` Xinliang David Li
@ 2011-06-08 8:54 ` Richard Guenther
2011-06-09 22:16 ` H.J. Lu
2 siblings, 0 replies; 30+ messages in thread
From: Richard Guenther @ 2011-06-08 8:54 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
On Tue, Jun 7, 2011 at 8:54 PM, Xinliang David Li <davidxl@google.com> wrote:
> Please review the attached two patches.
>
> In the first patch, gate functions are cleaned up. All the per
> function legality checks are moved into the executor and the
> optimization heuristic checks (optimize for size) remain in the
> gators. These allow the the following overriding order:
>
> common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
> heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
> options <--- legality check
>
> Testing under going. Ok for trunk?
It's somewhat odd that you keep the optimize for size/speed checks
in the gate - yes, they also work with a NULL cfun, returning a "default"
(previously they were just optimize_size checks). So probably it
makes sense to keep those in the gates.
I notice that with the patch we will now unconditionally execute
TODOs for those passes that were disabled per function which
will eventually cause some compile-time regression, mostly
restricted to checking enabled builds (which is ok I guess). I
suppose we could allow the execute function to return a special
value that says "I really didn't do anything". OTOH most of the
pass machinery needs some cleanup anyway which can be done
as followup.
The gate cleanup patch looks ok to me, please give others a chance
to comment on this change.
Thanks,
Richard.
> Thanks,
>
> David
>
> On Tue, Jun 7, 2011 at 9:24 AM, Xinliang David Li <davidxl@google.com> wrote:
>> Ok -- that sounds good.
>>
>> David
>>
>> On Tue, Jun 7, 2011 at 3:10 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Mon, Jun 6, 2011 at 6:00 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> This is the version of the patch that walks through pass lists.
>>>>>>
>>>>>> Ok with this one?
>>>>>
>>>>> +/* Dump all optimization passes. */
>>>>> +
>>>>> +void
>>>>> +dump_passes (void)
>>>>> +{
>>>>> + struct cgraph_node *n, *node = NULL;
>>>>> + tree save_fndecl = current_function_decl;
>>>>> +
>>>>> + fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
>>>>>
>>>>> this isn't accurate info - cloning can cause more cgraph nodes to
>>>>> appear (it also looks completely unrelated to dump_passes ...).
>>>>> Please drop it.
>>>>
>>>> Ok.
>>>>
>>>>
>>>>>
>>>>> + create_pass_tab();
>>>>> + gcc_assert (pass_tab);
>>>>>
>>>>> you have quite many asserts of this kind - we don't want them when
>>>>> the previous stmt as in this case indicates everything is ok.
>>>>
>>>> ok.
>>>>
>>>>>
>>>>> + push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>>>>>
>>>>> this has side-effects, I'm not sure we want this here. Why do you
>>>>> need it? Probably because of
>>>>>
>>>>> + is_really_on = override_gate_status (pass, current_function_decl, is_on);
>>>>>
>>>>> ? But that is dependent on the function given which should have no
>>>>> effect (unless it is overridden globally in which case override_gate_status
>>>>> and friends should deal with a NULL cfun).
>>>>
>>>> As we discussed, currently some pass gate functions depend on per node
>>>> information -- those checks need to be pushed into execute functions.
>>>> I would like to clean those up later -- at which time, the push/pop
>>>> can be removed.
>>>
>>> I'd like to do it the other way around, first clean up the gate functions then
>>> drop in this patch without the cfun push/pop. The revised patch looks ok
>>> to me with the cfun push/pop removed.
>>>
>>> Thanks,
>>> Richard.
>>>
>>>>>
>>>>> I don't understand why you need another table mapping pass to name
>>>>> when pass->name is available and the info is trivially re-constructible.
>>>>
>>>> This is needed as the pass->name is not the canonicalized name (i.e.,
>>>> not with number suffix etc), so the extra mapping from id to
>>>> normalized name is needed.
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>>>
>>>>> Thanks,
>>>>> Richard.
>>>>>
>>>>>> David
>>>>>>
>>>>>> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>>>>>>> configuration. The sample output is attached. There is one
>>>>>>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>>>>>>> note registered thus they are not listed. They are not important as
>>>>>>>>>>> they can not be turned on/off anyway.
>>>>>>>>>>>
>>>>>>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>>>>>>> of function assembler names to be specified.
>>>>>>>>>>>
>>>>>>>>>>> Ok for trunk?
>>>>>>>>>>
>>>>>>>>>> Please split the patch.
>>>>>>>>>>
>>>>>>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>>>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>>>>>>> at pass execution time when it's not already dumped - that really looks
>>>>>>>>>> gross.
>>>>>>>>>
>>>>>>>>> Yes, that was the original plan -- but it has problems
>>>>>>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>>>>>>> frequently -- it can be a long term maintanance burden;
>>>>>>>>> 2) the centralized dumper needs to be done after option processing
>>>>>>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>>>>>>
>>>>>>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>>>>>>> to do the dumping and tracking indentation.
>>>>>>>>
>>>>>>>> Well, if you have a CU that is empty or optimized to nothing at some point
>>>>>>>> you will not get a complete pass list. I suppose optimize attributes might
>>>>>>>> also confuse output. Your solution might not be that intrusive
>>>>>>>> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
>>>>>>>> dumping from toplev_main before calling do_compile (), 3) gate functions
>>>>>>>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>>>>>>>> your option summary output will be bogus anyway.
>>>>>>>>
>>>>>>>> So - what is the output intended for if it isn't reliable?
>>>>>>>
>>>>>>> This needs to be cleaned up at some point -- the gate function should
>>>>>>> behave the same for all functions and per-function decisions need to
>>>>>>> be pushed down to the executor body. I will try to rework the patch
>>>>>>> as you suggested to see if there are problems.
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Richard.
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> The documentation should also link this option to the -fenable/disable
>>>>>>>>>> options as obviously the pass names in that dump are those to be
>>>>>>>>>> used for those flags (and not readily available anywhere else).
>>>>>>>>>
>>>>>>>>> Ok.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> I also think that it would be way more useful to note in the individual
>>>>>>>>>> dump files the functions (at the place they would usually appear) that
>>>>>>>>>> have the pass explicitly enabled/disabled.
>>>>>>>>>
>>>>>>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>>>>>>> explicitly disabled.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>> David
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Richard.
>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>> David
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-07 20:39 ` Xinliang David Li
@ 2011-06-08 9:06 ` Richard Guenther
0 siblings, 0 replies; 30+ messages in thread
From: Richard Guenther @ 2011-06-08 9:06 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches
On Tue, Jun 7, 2011 at 10:20 PM, Xinliang David Li <davidxl@google.com> wrote:
> The dump-pass patch with test case.
Ok.
Thanks,
Richard.
> David
>
> On Tue, Jun 7, 2011 at 11:54 AM, Xinliang David Li <davidxl@google.com> wrote:
>> Please review the attached two patches.
>>
>> In the first patch, gate functions are cleaned up. All the per
>> function legality checks are moved into the executor and the
>> optimization heuristic checks (optimize for size) remain in the
>> gators. These allow the the following overriding order:
>>
>> common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
>> heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
>> options <--- legality check
>>
>> Testing under going. Ok for trunk?
>>
>> Thanks,
>>
>> David
>>
>> On Tue, Jun 7, 2011 at 9:24 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> Ok -- that sounds good.
>>>
>>> David
>>>
>>> On Tue, Jun 7, 2011 at 3:10 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Mon, Jun 6, 2011 at 6:00 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> On Mon, Jun 6, 2011 at 4:38 AM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Thu, Jun 2, 2011 at 9:12 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>> This is the version of the patch that walks through pass lists.
>>>>>>>
>>>>>>> Ok with this one?
>>>>>>
>>>>>> +/* Dump all optimization passes. */
>>>>>> +
>>>>>> +void
>>>>>> +dump_passes (void)
>>>>>> +{
>>>>>> + struct cgraph_node *n, *node = NULL;
>>>>>> + tree save_fndecl = current_function_decl;
>>>>>> +
>>>>>> + fprintf (stderr, "MAX_UID = %d\n", cgraph_max_uid);
>>>>>>
>>>>>> this isn't accurate info - cloning can cause more cgraph nodes to
>>>>>> appear (it also looks completely unrelated to dump_passes ...).
>>>>>> Please drop it.
>>>>>
>>>>> Ok.
>>>>>
>>>>>
>>>>>>
>>>>>> + create_pass_tab();
>>>>>> + gcc_assert (pass_tab);
>>>>>>
>>>>>> you have quite many asserts of this kind - we don't want them when
>>>>>> the previous stmt as in this case indicates everything is ok.
>>>>>
>>>>> ok.
>>>>>
>>>>>>
>>>>>> + push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>>>>>>
>>>>>> this has side-effects, I'm not sure we want this here. Why do you
>>>>>> need it? Probably because of
>>>>>>
>>>>>> + is_really_on = override_gate_status (pass, current_function_decl, is_on);
>>>>>>
>>>>>> ? But that is dependent on the function given which should have no
>>>>>> effect (unless it is overridden globally in which case override_gate_status
>>>>>> and friends should deal with a NULL cfun).
>>>>>
>>>>> As we discussed, currently some pass gate functions depend on per node
>>>>> information -- those checks need to be pushed into execute functions.
>>>>> I would like to clean those up later -- at which time, the push/pop
>>>>> can be removed.
>>>>
>>>> I'd like to do it the other way around, first clean up the gate functions then
>>>> drop in this patch without the cfun push/pop. The revised patch looks ok
>>>> to me with the cfun push/pop removed.
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>>>
>>>>>> I don't understand why you need another table mapping pass to name
>>>>>> when pass->name is available and the info is trivially re-constructible.
>>>>>
>>>>> This is needed as the pass->name is not the canonicalized name (i.e.,
>>>>> not with number suffix etc), so the extra mapping from id to
>>>>> normalized name is needed.
>>>>>
>>>>> Thanks,
>>>>>
>>>>> David
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Richard.
>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>> On Wed, Jun 1, 2011 at 12:45 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>> On Wed, Jun 1, 2011 at 12:29 PM, Richard Guenther
>>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>>> On Wed, Jun 1, 2011 at 6:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>>>> On Wed, Jun 1, 2011 at 1:51 AM, Richard Guenther
>>>>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>>>>> On Wed, Jun 1, 2011 at 1:34 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>>>>>> The following patch implements the a new option that dumps gcc PASS
>>>>>>>>>>>> configuration. The sample output is attached. There is one
>>>>>>>>>>>> limitation: some placeholder passes that are named with '*xxx' are
>>>>>>>>>>>> note registered thus they are not listed. They are not important as
>>>>>>>>>>>> they can not be turned on/off anyway.
>>>>>>>>>>>>
>>>>>>>>>>>> The patch also enhanced -fenable-xxx and -fdisable-xx to allow a list
>>>>>>>>>>>> of function assembler names to be specified.
>>>>>>>>>>>>
>>>>>>>>>>>> Ok for trunk?
>>>>>>>>>>>
>>>>>>>>>>> Please split the patch.
>>>>>>>>>>>
>>>>>>>>>>> I'm not too happy how you dump the pass configuration. Why not simply,
>>>>>>>>>>> at a _single_ place, walk the pass tree? Instead of doing pieces of it
>>>>>>>>>>> at pass execution time when it's not already dumped - that really looks
>>>>>>>>>>> gross.
>>>>>>>>>>
>>>>>>>>>> Yes, that was the original plan -- but it has problems
>>>>>>>>>> 1) the dumper needs to know the root pass lists -- which can change
>>>>>>>>>> frequently -- it can be a long term maintanance burden;
>>>>>>>>>> 2) the centralized dumper needs to be done after option processing
>>>>>>>>>> 3) not sure if gate functions have any side effects or have dependencies on cfun
>>>>>>>>>>
>>>>>>>>>> The proposed solutions IMHO is not that intrusive -- just three hooks
>>>>>>>>>> to do the dumping and tracking indentation.
>>>>>>>>>
>>>>>>>>> Well, if you have a CU that is empty or optimized to nothing at some point
>>>>>>>>> you will not get a complete pass list. I suppose optimize attributes might
>>>>>>>>> also confuse output. Your solution might not be that intrusive
>>>>>>>>> but it is still ugly. I don't see 1) as an issue, for 2) you can just call the
>>>>>>>>> dumping from toplev_main before calling do_compile (), 3) gate functions
>>>>>>>>> shouldn't have side-effects, but as they could gate on optimize_for_speed ()
>>>>>>>>> your option summary output will be bogus anyway.
>>>>>>>>>
>>>>>>>>> So - what is the output intended for if it isn't reliable?
>>>>>>>>
>>>>>>>> This needs to be cleaned up at some point -- the gate function should
>>>>>>>> behave the same for all functions and per-function decisions need to
>>>>>>>> be pushed down to the executor body. I will try to rework the patch
>>>>>>>> as you suggested to see if there are problems.
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Richard.
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> The documentation should also link this option to the -fenable/disable
>>>>>>>>>>> options as obviously the pass names in that dump are those to be
>>>>>>>>>>> used for those flags (and not readily available anywhere else).
>>>>>>>>>>
>>>>>>>>>> Ok.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> I also think that it would be way more useful to note in the individual
>>>>>>>>>>> dump files the functions (at the place they would usually appear) that
>>>>>>>>>>> have the pass explicitly enabled/disabled.
>>>>>>>>>>
>>>>>>>>>> Ok -- for ipa passes or tree/rtl passes where all functions are
>>>>>>>>>> explicitly disabled.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>>
>>>>>>>>>> David
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Richard.
>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> David
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-07 19:09 ` Xinliang David Li
2011-06-07 20:39 ` Xinliang David Li
2011-06-08 8:54 ` Richard Guenther
@ 2011-06-09 22:16 ` H.J. Lu
2011-06-09 22:24 ` Carrot Wei
` (2 more replies)
2 siblings, 3 replies; 30+ messages in thread
From: H.J. Lu @ 2011-06-09 22:16 UTC (permalink / raw)
To: Xinliang David Li; +Cc: Richard Guenther, GCC Patches
On Tue, Jun 7, 2011 at 11:54 AM, Xinliang David Li <davidxl@google.com> wrote:
> Please review the attached two patches.
>
> In the first patch, gate functions are cleaned up. All the per
> function legality checks are moved into the executor and the
> optimization heuristic checks (optimize for size) remain in the
> gators. These allow the the following overriding order:
>
> common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
> heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
> options <--- legality check
>
> Testing under going. Ok for trunk?
>
This caused:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49350
--
H.J.
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-09 22:16 ` H.J. Lu
@ 2011-06-09 22:24 ` Carrot Wei
2011-06-09 22:32 ` Xinliang David Li
2011-06-09 22:51 ` Xinliang David Li
2 siblings, 0 replies; 30+ messages in thread
From: Carrot Wei @ 2011-06-09 22:24 UTC (permalink / raw)
To: H.J. Lu; +Cc: Xinliang David Li, Richard Guenther, GCC Patches
It also breaks arm backend.
../trunk/configure '--build=x86_64-build_pc-linux-gnu'
'--host=x86_64-build_pc-linux-gnu'
'--target=arm-unknown-linux-gnueabi'
'--with-sysroot=/home/carrot/x-tools/arm-unknown-linux-gnueabi/arm-unknown-linux-gnueabi/sys-root'
'--disable-multilib' '--with-float=soft' '--disable-sjlj-exceptions'
'--enable-__cxa_atexit' '--disable-nls' '--enable-threads=posix'
'--enable-symvers=gnu' '--enable-c99' '--enable-long-long'
'--enable-target-optspace' '--disable-bootstrap'
'build_alias=x86_64-build_pc-linux-gnu'
'host_alias=x86_64-build_pc-linux-gnu'
'target_alias=arm-unknown-linux-gnueabi'
'--enable-languages=c,c++,lto'
make
...
/bin/sh ../libtool --tag CXX --mode=compile
/usr/local/google/home/carrot/armobj1/./gcc/xgcc -shared-libgcc
-B/usr/local/google/home/carrot/armobj1/./gcc -nostdinc++
-L/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/src
-L/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/src/.libs
-B/usr/local/arm-unknown-linux-gnueabi/bin/
-B/usr/local/arm-unknown-linux-gnueabi/lib/ -isystem
/usr/local/arm-unknown-linux-gnueabi/include -isystem
/usr/local/arm-unknown-linux-gnueabi/sys-include
-I/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/include/arm-unknown-linux-gnueabi
-I/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/include
-I/usr/local/google/home/carrot/trunk/libstdc++-v3/libsupc++
-fno-implicit-templates -Wall -Wextra -Wwrite-strings -Wcast-qual
-fdiagnostics-show-location=once -ffunction-sections -fdata-sections
-g -Os -I/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/include/backward
-Wno-deprecated -c ../../../../trunk/libstdc++-v3/src/strstream.cc
libtool: compile: /usr/local/google/home/carrot/armobj1/./gcc/xgcc
-shared-libgcc -B/usr/local/google/home/carrot/armobj1/./gcc
-nostdinc++ -L/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/src
-L/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/src/.libs
-B/usr/local/arm-unknown-linux-gnueabi/bin/
-B/usr/local/arm-unknown-linux-gnueabi/lib/ -isystem
/usr/local/arm-unknown-linux-gnueabi/include -isystem
/usr/local/arm-unknown-linux-gnueabi/sys-include
-I/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/include/arm-unknown-linux-gnueabi
-I/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/include
-I/usr/local/google/home/carrot/trunk/libstdc++-v3/libsupc++
-fno-implicit-templates -Wall -Wextra -Wwrite-strings -Wcast-qual
-fdiagnostics-show-location=once -ffunction-sections -fdata-sections
-g -Os -I/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/include/backward
-Wno-deprecated -c ../../../../trunk/libstdc++-v3/src/strstream.cc
-fPIC -DPIC -o .libs/strstream.o
In file included from ../../../../trunk/libstdc++-v3/src/strstream.cc:45:0:
/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/include/backward/strstream:
In member function 'void*
std::strstream::_ZTv0_n12_NSt9strstreamD1Ev()':
/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/include/backward/strstream:171:13:
internal compiler error: in verify_curr_properties, at passes.c:1660
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.
make[4]: *** [strstream.lo] Error 1
make[4]: Leaving directory
`/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3/src'
make[3]: *** [all-recursive] Error 1
make[3]: Leaving directory
`/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3'
make[2]: *** [all] Error 2
make[2]: Leaving directory
`/usr/local/google/home/carrot/armobj1/arm-unknown-linux-gnueabi/libstdc++-v3'
make[1]: *** [all-target-libstdc++-v3] Error 2
make[1]: Leaving directory `/usr/local/google/home/carrot/armobj1'
make: *** [all] Error 2
On Fri, Jun 10, 2011 at 6:05 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jun 7, 2011 at 11:54 AM, Xinliang David Li <davidxl@google.com> wrote:
>> Please review the attached two patches.
>>
>> In the first patch, gate functions are cleaned up. All the per
>> function legality checks are moved into the executor and the
>> optimization heuristic checks (optimize for size) remain in the
>> gators. These allow the the following overriding order:
>>
>> common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
>> heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
>> options <--- legality check
>>
>> Testing under going. Ok for trunk?
>>
>
> This caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49350
>
> --
> H.J.
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-09 22:16 ` H.J. Lu
2011-06-09 22:24 ` Carrot Wei
@ 2011-06-09 22:32 ` Xinliang David Li
2011-06-09 22:51 ` Xinliang David Li
2 siblings, 0 replies; 30+ messages in thread
From: Xinliang David Li @ 2011-06-09 22:32 UTC (permalink / raw)
To: H.J. Lu; +Cc: Richard Guenther, GCC Patches
Can you send me a trace? I can not reproduce the problem.
David
On Thu, Jun 9, 2011 at 3:05 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jun 7, 2011 at 11:54 AM, Xinliang David Li <davidxl@google.com> wrote:
>> Please review the attached two patches.
>>
>> In the first patch, gate functions are cleaned up. All the per
>> function legality checks are moved into the executor and the
>> optimization heuristic checks (optimize for size) remain in the
>> gators. These allow the the following overriding order:
>>
>> common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
>> heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
>> options <--- legality check
>>
>> Testing under going. Ok for trunk?
>>
>
> This caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49350
>
> --
> H.J.
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-09 22:16 ` H.J. Lu
2011-06-09 22:24 ` Carrot Wei
2011-06-09 22:32 ` Xinliang David Li
@ 2011-06-09 22:51 ` Xinliang David Li
2011-06-09 23:28 ` Xinliang David Li
2 siblings, 1 reply; 30+ messages in thread
From: Xinliang David Li @ 2011-06-09 22:51 UTC (permalink / raw)
To: H.J. Lu; +Cc: Richard Guenther, GCC Patches
Though I can not reproduce it, it might be related to what Richard
mentioned in the review -- The TODO's are executed though the pass is
not. This opened up a can of worm -- I will revert the patches for
now.
David
On Thu, Jun 9, 2011 at 3:05 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jun 7, 2011 at 11:54 AM, Xinliang David Li <davidxl@google.com> wrote:
>> Please review the attached two patches.
>>
>> In the first patch, gate functions are cleaned up. All the per
>> function legality checks are moved into the executor and the
>> optimization heuristic checks (optimize for size) remain in the
>> gators. These allow the the following overriding order:
>>
>> common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
>> heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
>> options <--- legality check
>>
>> Testing under going. Ok for trunk?
>>
>
> This caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49350
>
> --
> H.J.
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-09 22:51 ` Xinliang David Li
@ 2011-06-09 23:28 ` Xinliang David Li
2011-06-10 9:10 ` Richard Guenther
0 siblings, 1 reply; 30+ messages in thread
From: Xinliang David Li @ 2011-06-09 23:28 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches, H.J. Lu
Patch is temporally rolled back.
Richard, looks like deeper pass manager cleanup is needed -- I would
like to delay that. For now, this leaves us two choices -- 1) do cfunc
push/pop, or 2) do pass dump while executing. None of them is ideal,
but safe enough.
Thanks,
David
On Thu, Jun 9, 2011 at 3:32 PM, Xinliang David Li <davidxl@google.com> wrote:
> Though I can not reproduce it, it might be related to what Richard
> mentioned in the review -- The TODO's are executed though the pass is
> not. This opened up a can of worm -- I will revert the patches for
> now.
>
> David
>
>
> On Thu, Jun 9, 2011 at 3:05 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Tue, Jun 7, 2011 at 11:54 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> Please review the attached two patches.
>>>
>>> In the first patch, gate functions are cleaned up. All the per
>>> function legality checks are moved into the executor and the
>>> optimization heuristic checks (optimize for size) remain in the
>>> gators. These allow the the following overriding order:
>>>
>>> common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
>>> heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
>>> options <--- legality check
>>>
>>> Testing under going. Ok for trunk?
>>>
>>
>> This caused:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49350
>>
>> --
>> H.J.
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-09 23:28 ` Xinliang David Li
@ 2011-06-10 9:10 ` Richard Guenther
2011-06-10 16:37 ` Xinliang David Li
0 siblings, 1 reply; 30+ messages in thread
From: Richard Guenther @ 2011-06-10 9:10 UTC (permalink / raw)
To: Xinliang David Li; +Cc: GCC Patches, H.J. Lu
On Fri, Jun 10, 2011 at 12:51 AM, Xinliang David Li <davidxl@google.com> wrote:
> Patch is temporally rolled back.
>
> Richard, looks like deeper pass manager cleanup is needed -- I would
> like to delay that. For now, this leaves us two choices -- 1) do cfunc
> push/pop, or 2) do pass dump while executing. None of them is ideal,
> but safe enough.
Well. It seems we should take a step back and look at the whole picture
and try to figure out how it should look like in the end (maybe do that
in London).
For now I prefer 1) over 2).
Thanks,
Richard.
> Thanks,
>
> David
>
> On Thu, Jun 9, 2011 at 3:32 PM, Xinliang David Li <davidxl@google.com> wrote:
>> Though I can not reproduce it, it might be related to what Richard
>> mentioned in the review -- The TODO's are executed though the pass is
>> not. This opened up a can of worm -- I will revert the patches for
>> now.
>>
>> David
>>
>>
>> On Thu, Jun 9, 2011 at 3:05 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Tue, Jun 7, 2011 at 11:54 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> Please review the attached two patches.
>>>>
>>>> In the first patch, gate functions are cleaned up. All the per
>>>> function legality checks are moved into the executor and the
>>>> optimization heuristic checks (optimize for size) remain in the
>>>> gators. These allow the the following overriding order:
>>>>
>>>> common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
>>>> heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
>>>> options <--- legality check
>>>>
>>>> Testing under going. Ok for trunk?
>>>>
>>>
>>> This caused:
>>>
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49350
>>>
>>> --
>>> H.J.
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: -fdump-passes -fenable-xxx=func_name_list
2011-06-10 9:10 ` Richard Guenther
@ 2011-06-10 16:37 ` Xinliang David Li
0 siblings, 0 replies; 30+ messages in thread
From: Xinliang David Li @ 2011-06-10 16:37 UTC (permalink / raw)
To: Richard Guenther; +Cc: GCC Patches, H.J. Lu
On Fri, Jun 10, 2011 at 2:03 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Fri, Jun 10, 2011 at 12:51 AM, Xinliang David Li <davidxl@google.com> wrote:
>> Patch is temporally rolled back.
>>
>> Richard, looks like deeper pass manager cleanup is needed -- I would
>> like to delay that. For now, this leaves us two choices -- 1) do cfunc
>> push/pop, or 2) do pass dump while executing. None of them is ideal,
>> but safe enough.
>
> Well. It seems we should take a step back and look at the whole picture
> and try to figure out how it should look like in the end (maybe do that
> in London).
>
> For now I prefer 1) over 2).
That sounds good. For now I will check in 1) (as it has no impact on
default behavior) and do pass clean up later. I won't be in London for
discussion, but you let me know how the discussion goes.
Thanks,
David
>
> Thanks,
> Richard.
>
>> Thanks,
>>
>> David
>>
>> On Thu, Jun 9, 2011 at 3:32 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> Though I can not reproduce it, it might be related to what Richard
>>> mentioned in the review -- The TODO's are executed though the pass is
>>> not. This opened up a can of worm -- I will revert the patches for
>>> now.
>>>
>>> David
>>>
>>>
>>> On Thu, Jun 9, 2011 at 3:05 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Tue, Jun 7, 2011 at 11:54 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> Please review the attached two patches.
>>>>>
>>>>> In the first patch, gate functions are cleaned up. All the per
>>>>> function legality checks are moved into the executor and the
>>>>> optimization heuristic checks (optimize for size) remain in the
>>>>> gators. These allow the the following overriding order:
>>>>>
>>>>> common flags (O2, -ftree-vrp, -fgcse etc) <--- compiler
>>>>> heuristic (optimize for size/speed) <--- -fdisable/enable forcing pass
>>>>> options <--- legality check
>>>>>
>>>>> Testing under going. Ok for trunk?
>>>>>
>>>>
>>>> This caused:
>>>>
>>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49350
>>>>
>>>> --
>>>> H.J.
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2011-06-10 16:15 UTC | newest]
Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <BANLkTikXRUTmZZokg4OtJA5fBrWUG+7yZux3=CLDBox1Q+Qhtw@mail.gmail.com>
2011-06-01 8:51 ` -fdump-passes -fenable-xxx=func_name_list Richard Guenther
2011-06-01 16:17 ` Xinliang David Li
2011-06-01 17:24 ` Xinliang David Li
2011-06-05 17:25 ` Xinliang David Li
2011-06-06 11:22 ` Richard Guenther
2011-06-06 15:54 ` Xinliang David Li
2011-06-06 15:59 ` Richard Guenther
2011-06-06 19:21 ` Xinliang David Li
2011-06-07 10:11 ` Richard Guenther
2011-06-01 19:29 ` Xinliang David Li
2011-06-01 19:29 ` Richard Guenther
2011-06-01 19:46 ` Xinliang David Li
2011-06-02 7:13 ` Xinliang David Li
2011-06-05 17:25 ` Xinliang David Li
2011-06-06 11:38 ` Richard Guenther
2011-06-06 16:00 ` Xinliang David Li
2011-06-06 19:23 ` Xinliang David Li
2011-06-07 10:10 ` Richard Guenther
2011-06-07 16:24 ` Xinliang David Li
2011-06-07 19:09 ` Xinliang David Li
2011-06-07 20:39 ` Xinliang David Li
2011-06-08 9:06 ` Richard Guenther
2011-06-08 8:54 ` Richard Guenther
2011-06-09 22:16 ` H.J. Lu
2011-06-09 22:24 ` Carrot Wei
2011-06-09 22:32 ` Xinliang David Li
2011-06-09 22:51 ` Xinliang David Li
2011-06-09 23:28 ` Xinliang David Li
2011-06-10 9:10 ` Richard Guenther
2011-06-10 16:37 ` Xinliang David Li
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).