New options to disable/enable any pass for any functions (issue4550056)

On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Wed, 25 May 2011, Xinliang David Li wrote:
>
>> Ping. The link to the message:
>>
>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>
> I don't consider this an option handling patch.  Patches adding whole new
> features involving new options should be reviewed by maintainers for the
> part of the compiler relevant to those features (since there isn't a pass
> manager maintainer, I guess that means middle-end).

Hmm, I suppose then you reviewed the option handling parts and they
are ok?  Those globbing options always cause headache to me.

+-fenable-ipa-@var{pass} @gol
+-fenable-rtl-@var{pass} @gol
+-fenable-rtl-@var{pass}=@var{range-list} @gol
+-fenable-tree-@var{pass} @gol

so, no -fenable-tree-@var{pass}=@var{range-list}?

Does the pass name match 1:1 with the dump file name?  In which
case

+Disable ipa 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.

isn't true as passes that are invoked only a single time lack the number
suffix (yes, I'd really like that to be changed ...)

Please break likes also in .texi files and stick to 80 columns.  Please
document that these options are debugging options and regular
options for enabling/disabling passes should be used.  I would suggest
to group documentation differently and document -fenable-* and
-fdisable-*, thus,

+ -fdisable-@var{kind}-@var{pass}
+ -fenable-@var{kind}-@var{pass}

Even in .texi files please two spaces after a full-stop.

+extern void enable_disable_pass (const char *, bool);

I'd rather have both enable_pass and disable_pass ;)

+struct function;
+extern void pass_dump_function_header (FILE *, tree, struct function *);

that's odd and probably should be split out, the function should
maybe reside in tree-pretty-print.c.

wrappers.

+void
+register_pass_name (struct opt_pass *pass, const char *name)

doesn't seem to need exporting, so don't and make it static.

+  if (!pass_name_tab)
+    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);

see above, the initial size should be larger - we have 223 passes at the
moment, so use 256.

+  else
+    return; /* Ignore plugin passes.  */

?  You mean they might clash?

+struct opt_pass *
+get_pass_by_name (const char *name)

doesn't need exporting either.

+      if (is_enable)
+        error ("unrecognized option -fenable");
+      else
+        error ("unrecognized option -fdisable");

I think that should be fatal_error - Joseph?

+      if (is_enable)
+        error ("unknown pass %s specified in -fenable", phase_name);
+      else
+        error ("unknown pass %s specified in -fdisble", phase_name);

likewise.

+      if (!enabled_pass_uid_range_tab)
+       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);

instead of using a hashtable here please use a VEC indexed by
the static_pass_number which shoud speed up

+static bool
+is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
+                                       tree func, htab_t tab)
+{
+  struct uid_range **slot, *range, key;
+  int cgraph_uid;
+
+  if (!tab)
+    return false;
+
+  key.pass = pass;
+  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
+  if (!slot || !*slot)
+    return false;

and simplify the code quite a bit.

+  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;

note that cgraph uids are recycled, so it might not be the best idea
to use them as discriminator (though I don't have a good idea how
to represent ranges without them).

+  explicitly_enabled = is_pass_explicitly_enabled (pass,
current_function_decl);
+  explicitly_disabled = is_pass_explicitly_disabled (pass,
current_function_decl);
+
   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);

so explicitly disabling wins over explicit enabling ;)  I think this
implementation detail and the fact that you always query both
hints at that the interface should be like

gate_status = override_gate_status (pass, current_function_decl, gate_status);

instead.

Thus, please split out the function header dumping changes and rework
the rest of the patch as suggested.

Thanks,
Richard.

> --
> Joseph S. Myers
> joseph@codesourcery.com
>

On Thu, 26 May 2011, Richard Guenther wrote:

> +      if (is_enable)
> +        error ("unrecognized option -fenable");
> +      else
> +        error ("unrecognized option -fdisable");
> 
> I think that should be fatal_error - Joseph?

No, all existing errors for unknown options are ordinary errors rather 
than fatal errors (though if the driver detects a problem it won't then 
run cc1).

On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
> <joseph@codesourcery.com> wrote:
>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>
>>> Ping. The link to the message:
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>
>> I don't consider this an option handling patch.  Patches adding whole new
>> features involving new options should be reviewed by maintainers for the
>> part of the compiler relevant to those features (since there isn't a pass
>> manager maintainer, I guess that means middle-end).
>
> Hmm, I suppose then you reviewed the option handling parts and they
> are ok?  Those globbing options always cause headache to me.
>
> +-fenable-ipa-@var{pass} @gol
> +-fenable-rtl-@var{pass} @gol
> +-fenable-rtl-@var{pass}=@var{range-list} @gol
> +-fenable-tree-@var{pass} @gol
>
> so, no -fenable-tree-@var{pass}=@var{range-list}?
>

Missed. Will add.


> Does the pass name match 1:1 with the dump file name?  In which
> case

Yes.

>
> +Disable ipa 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.
>
> isn't true as passes that are invoked only a single time lack the number
> suffix (yes, I'd really like that to be changed ...)

Yes, pass with single static instance does not need number suffix.

>
> Please break likes also in .texi files and stick to 80 columns.

Done.

>  Please
> document that these options are debugging options and regular
> options for enabling/disabling passes should be used.  I would suggest
> to group documentation differently and document -fenable-* and
> -fdisable-*, thus,
>
> + -fdisable-@var{kind}-@var{pass}
> + -fenable-@var{kind}-@var{pass}
>
> Even in .texi files please two spaces after a full-stop.

Done

>
> +extern void enable_disable_pass (const char *, bool);
>
> I'd rather have both enable_pass and disable_pass ;)

Ok.

>
> +struct function;
> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>
> that's odd and probably should be split out, the function should
> maybe reside in tree-pretty-print.c.

Ok.

>
> Index: tree-ssa-loop-ivopts.c
> ===================================================================
> --- tree-ssa-loop-ivopts.c      (revision 173837)
> +++ tree-ssa-loop-ivopts.c      (working copy)
> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>
> well - doesn't belong here ;)

Sorry -- many things in the same client -- not needed with your latest
change anyway.

>
> +static hashval_t
> +passr_hash (const void *p)
> +{
> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
> +  return htab_hash_string (s->unique_name);
> +}
> +
> +/* Hash equal function  */
> +
> +static int
> +passr_eq (const void *p1, const void *p2)
> +{
> +  const struct pass_registry *const s1 = (const struct pass_registry
> *const) p1;
> +  const struct pass_registry *const s2 = (const struct pass_registry
> *const) p2;
> +
> +  return !strcmp (s1->unique_name, s2->unique_name);
> +}
>
> you can use htab_hash_string and strcmp directly, no need for these
> wrappers.

The hashtable entry is not string in this case. It is pass_registry,
thus the wrapper.

>
> +void
> +register_pass_name (struct opt_pass *pass, const char *name)
>
> doesn't seem to need exporting, so don't and make it static.

Done.

>
> +  if (!pass_name_tab)
> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>
> see above, the initial size should be larger - we have 223 passes at the
> moment, so use 256.

Done.

>
> +  else
> +    return; /* Ignore plugin passes.  */
>
> ?  You mean they might clash?

Yes, name clash.

>
> +struct opt_pass *
> +get_pass_by_name (const char *name)
>
> doesn't need exporting either.

Done.

>
> +      if (is_enable)
> +        error ("unrecognized option -fenable");
> +      else
> +        error ("unrecognized option -fdisable");
>
> I think that should be fatal_error - Joseph?
>
> +      if (is_enable)
> +        error ("unknown pass %s specified in -fenable", phase_name);
> +      else
> +        error ("unknown pass %s specified in -fdisble", phase_name);
>
> likewise.
>
> +      if (!enabled_pass_uid_range_tab)
> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>
> instead of using a hashtable here please use a VEC indexed by
> the static_pass_number which shoud speed up

Ok.  The reason I did not use it is because in most of the cases, the
htab will be very small -- it is determined by the number of passes
specified in the command line, while the VEC requires allocating const
size array. Not an issue as long as by default the array is not
allocated.

>
> +static bool
> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
> +                                       tree func, htab_t tab)
> +{
> +  struct uid_range **slot, *range, key;
> +  int cgraph_uid;
> +
> +  if (!tab)
> +    return false;
> +
> +  key.pass = pass;
> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
> +  if (!slot || !*slot)
> +    return false;
>
> and simplify the code quite a bit.
>
> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>
> note that cgraph uids are recycled, so it might not be the best idea
> to use them as discriminator (though I don't have a good idea how
> to represent ranges without them).

Yes. It is not a big problem as the blind search does not need to know
the id->name mapping. Once the id s found, it can be easily discovered
via dump.

>
> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
> current_function_decl);
> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
> current_function_decl);
> +
>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>
> so explicitly disabling wins over explicit enabling ;)  I think this
> implementation detail and the fact that you always query both
> hints at that the interface should be like
>
> gate_status = override_gate_status (pass, current_function_decl, gate_status);

Done.

>
> instead.
>
> Thus, please split out the function header dumping changes and rework
> the rest of the patch as suggested.

Split out. The new patch is attached.

Ok after testing is done?

Thanks,

David

>
> Thanks,
> Richard.
>
>> --
>> Joseph S. Myers
>> joseph@codesourcery.com
>>
>

The latest version that only exports 2 functions from passes.c.

David

On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>> <joseph@codesourcery.com> wrote:
>>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>>
>>>> Ping. The link to the message:
>>>>
>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>>
>>> I don't consider this an option handling patch.  Patches adding whole new
>>> features involving new options should be reviewed by maintainers for the
>>> part of the compiler relevant to those features (since there isn't a pass
>>> manager maintainer, I guess that means middle-end).
>>
>> Hmm, I suppose then you reviewed the option handling parts and they
>> are ok?  Those globbing options always cause headache to me.
>>
>> +-fenable-ipa-@var{pass} @gol
>> +-fenable-rtl-@var{pass} @gol
>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>> +-fenable-tree-@var{pass} @gol
>>
>> so, no -fenable-tree-@var{pass}=@var{range-list}?
>>
>
> Missed. Will add.
>
>
>> Does the pass name match 1:1 with the dump file name?  In which
>> case
>
> Yes.
>
>>
>> +Disable ipa 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.
>>
>> isn't true as passes that are invoked only a single time lack the number
>> suffix (yes, I'd really like that to be changed ...)
>
> Yes, pass with single static instance does not need number suffix.
>
>>
>> Please break likes also in .texi files and stick to 80 columns.
>
> Done.
>
>>  Please
>> document that these options are debugging options and regular
>> options for enabling/disabling passes should be used.  I would suggest
>> to group documentation differently and document -fenable-* and
>> -fdisable-*, thus,
>>
>> + -fdisable-@var{kind}-@var{pass}
>> + -fenable-@var{kind}-@var{pass}
>>
>> Even in .texi files please two spaces after a full-stop.
>
> Done
>
>>
>> +extern void enable_disable_pass (const char *, bool);
>>
>> I'd rather have both enable_pass and disable_pass ;)
>
> Ok.
>
>>
>> +struct function;
>> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>>
>> that's odd and probably should be split out, the function should
>> maybe reside in tree-pretty-print.c.
>
> Ok.
>
>>
>> Index: tree-ssa-loop-ivopts.c
>> ===================================================================
>> --- tree-ssa-loop-ivopts.c      (revision 173837)
>> +++ tree-ssa-loop-ivopts.c      (working copy)
>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>>
>> well - doesn't belong here ;)
>
> Sorry -- many things in the same client -- not needed with your latest
> change anyway.
>
>>
>> +static hashval_t
>> +passr_hash (const void *p)
>> +{
>> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
>> +  return htab_hash_string (s->unique_name);
>> +}
>> +
>> +/* Hash equal function  */
>> +
>> +static int
>> +passr_eq (const void *p1, const void *p2)
>> +{
>> +  const struct pass_registry *const s1 = (const struct pass_registry
>> *const) p1;
>> +  const struct pass_registry *const s2 = (const struct pass_registry
>> *const) p2;
>> +
>> +  return !strcmp (s1->unique_name, s2->unique_name);
>> +}
>>
>> you can use htab_hash_string and strcmp directly, no need for these
>> wrappers.
>
> The hashtable entry is not string in this case. It is pass_registry,
> thus the wrapper.
>
>>
>> +void
>> +register_pass_name (struct opt_pass *pass, const char *name)
>>
>> doesn't seem to need exporting, so don't and make it static.
>
> Done.
>
>>
>> +  if (!pass_name_tab)
>> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>>
>> see above, the initial size should be larger - we have 223 passes at the
>> moment, so use 256.
>
> Done.
>
>>
>> +  else
>> +    return; /* Ignore plugin passes.  */
>>
>> ?  You mean they might clash?
>
> Yes, name clash.
>
>>
>> +struct opt_pass *
>> +get_pass_by_name (const char *name)
>>
>> doesn't need exporting either.
>
> Done.
>
>>
>> +      if (is_enable)
>> +        error ("unrecognized option -fenable");
>> +      else
>> +        error ("unrecognized option -fdisable");
>>
>> I think that should be fatal_error - Joseph?
>>
>> +      if (is_enable)
>> +        error ("unknown pass %s specified in -fenable", phase_name);
>> +      else
>> +        error ("unknown pass %s specified in -fdisble", phase_name);
>>
>> likewise.
>>
>> +      if (!enabled_pass_uid_range_tab)
>> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>>
>> instead of using a hashtable here please use a VEC indexed by
>> the static_pass_number which shoud speed up
>
> Ok.  The reason I did not use it is because in most of the cases, the
> htab will be very small -- it is determined by the number of passes
> specified in the command line, while the VEC requires allocating const
> size array. Not an issue as long as by default the array is not
> allocated.
>
>>
>> +static bool
>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>> +                                       tree func, htab_t tab)
>> +{
>> +  struct uid_range **slot, *range, key;
>> +  int cgraph_uid;
>> +
>> +  if (!tab)
>> +    return false;
>> +
>> +  key.pass = pass;
>> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
>> +  if (!slot || !*slot)
>> +    return false;
>>
>> and simplify the code quite a bit.
>>
>> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>>
>> note that cgraph uids are recycled, so it might not be the best idea
>> to use them as discriminator (though I don't have a good idea how
>> to represent ranges without them).
>
> Yes. It is not a big problem as the blind search does not need to know
> the id->name mapping. Once the id s found, it can be easily discovered
> via dump.
>
>>
>> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
>> current_function_decl);
>> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
>> current_function_decl);
>> +
>>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>>
>> so explicitly disabling wins over explicit enabling ;)  I think this
>> implementation detail and the fact that you always query both
>> hints at that the interface should be like
>>
>> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>
> Done.
>
>>
>> instead.
>>
>> Thus, please split out the function header dumping changes and rework
>> the rest of the patch as suggested.
>
> Split out. The new patch is attached.
>
> Ok after testing is done?
>
> Thanks,
>
> David
>
>>
>> Thanks,
>> Richard.
>>
>>> --
>>> Joseph S. Myers
>>> joseph@codesourcery.com
>>>
>>
>

On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davidxl@google.com> wrote:
> The latest version that only exports 2 functions from passes.c.

Ok with ...

@@ -637,4 +637,8 @@ extern bool first_pass_instance;
 /* Declare for plugins.  */
 extern void do_per_function_toporder (void (*) (void *), void *);

+extern void disable_pass (const char *);
+extern void enable_pass (const char *);
+struct function;
+

struct function forward decl removed.

+  explicitly_enabled = is_pass_explicitly_enabled (pass, func);
+  explicitly_disabled = is_pass_explicitly_disabled (pass, func);

both functions inlined here and removed.

+#define MAX_PASS_ID 512

this removed and instead a VEC_safe_grow_cleared () or VEC_length ()
before the accesses.

+-fenable-ipa-@var{pass} @gol
+-fenable-rtl-@var{pass} @gol
+-fenable-rtl-@var{pass}=@var{range-list} @gol
+-fenable-tree-@var{pass} @gol
+-fenable-tree-@var{pass}=@var{range-list} @gol

-fenable-@var{kind}-@var{pass}, etc.

+@item -fdisable-@var{ipa|tree|rtl}-@var{pass}
+@itemx -fenable-@var{ipa|tree|rtl}-@var{pass}
+@itemx -fdisable-@var{tree|rtl}-@var{pass}=@var{range-list}
+@itemx -fenable-@var{tree|rtl}-@var{pass}=@var{range-list}

likewise.

Thanks,
Richard.

> David
>
> On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davidxl@google.com> wrote:
>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>>> <joseph@codesourcery.com> wrote:
>>>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>>>
>>>>> Ping. The link to the message:
>>>>>
>>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>>>
>>>> I don't consider this an option handling patch.  Patches adding whole new
>>>> features involving new options should be reviewed by maintainers for the
>>>> part of the compiler relevant to those features (since there isn't a pass
>>>> manager maintainer, I guess that means middle-end).
>>>
>>> Hmm, I suppose then you reviewed the option handling parts and they
>>> are ok?  Those globbing options always cause headache to me.
>>>
>>> +-fenable-ipa-@var{pass} @gol
>>> +-fenable-rtl-@var{pass} @gol
>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>> +-fenable-tree-@var{pass} @gol
>>>
>>> so, no -fenable-tree-@var{pass}=@var{range-list}?
>>>
>>
>> Missed. Will add.
>>
>>
>>> Does the pass name match 1:1 with the dump file name?  In which
>>> case
>>
>> Yes.
>>
>>>
>>> +Disable ipa 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.
>>>
>>> isn't true as passes that are invoked only a single time lack the number
>>> suffix (yes, I'd really like that to be changed ...)
>>
>> Yes, pass with single static instance does not need number suffix.
>>
>>>
>>> Please break likes also in .texi files and stick to 80 columns.
>>
>> Done.
>>
>>>  Please
>>> document that these options are debugging options and regular
>>> options for enabling/disabling passes should be used.  I would suggest
>>> to group documentation differently and document -fenable-* and
>>> -fdisable-*, thus,
>>>
>>> + -fdisable-@var{kind}-@var{pass}
>>> + -fenable-@var{kind}-@var{pass}
>>>
>>> Even in .texi files please two spaces after a full-stop.
>>
>> Done
>>
>>>
>>> +extern void enable_disable_pass (const char *, bool);
>>>
>>> I'd rather have both enable_pass and disable_pass ;)
>>
>> Ok.
>>
>>>
>>> +struct function;
>>> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>>>
>>> that's odd and probably should be split out, the function should
>>> maybe reside in tree-pretty-print.c.
>>
>> Ok.
>>
>>>
>>> Index: tree-ssa-loop-ivopts.c
>>> ===================================================================
>>> --- tree-ssa-loop-ivopts.c      (revision 173837)
>>> +++ tree-ssa-loop-ivopts.c      (working copy)
>>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>>>
>>> well - doesn't belong here ;)
>>
>> Sorry -- many things in the same client -- not needed with your latest
>> change anyway.
>>
>>>
>>> +static hashval_t
>>> +passr_hash (const void *p)
>>> +{
>>> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
>>> +  return htab_hash_string (s->unique_name);
>>> +}
>>> +
>>> +/* Hash equal function  */
>>> +
>>> +static int
>>> +passr_eq (const void *p1, const void *p2)
>>> +{
>>> +  const struct pass_registry *const s1 = (const struct pass_registry
>>> *const) p1;
>>> +  const struct pass_registry *const s2 = (const struct pass_registry
>>> *const) p2;
>>> +
>>> +  return !strcmp (s1->unique_name, s2->unique_name);
>>> +}
>>>
>>> you can use htab_hash_string and strcmp directly, no need for these
>>> wrappers.
>>
>> The hashtable entry is not string in this case. It is pass_registry,
>> thus the wrapper.
>>
>>>
>>> +void
>>> +register_pass_name (struct opt_pass *pass, const char *name)
>>>
>>> doesn't seem to need exporting, so don't and make it static.
>>
>> Done.
>>
>>>
>>> +  if (!pass_name_tab)
>>> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>>>
>>> see above, the initial size should be larger - we have 223 passes at the
>>> moment, so use 256.
>>
>> Done.
>>
>>>
>>> +  else
>>> +    return; /* Ignore plugin passes.  */
>>>
>>> ?  You mean they might clash?
>>
>> Yes, name clash.
>>
>>>
>>> +struct opt_pass *
>>> +get_pass_by_name (const char *name)
>>>
>>> doesn't need exporting either.
>>
>> Done.
>>
>>>
>>> +      if (is_enable)
>>> +        error ("unrecognized option -fenable");
>>> +      else
>>> +        error ("unrecognized option -fdisable");
>>>
>>> I think that should be fatal_error - Joseph?
>>>
>>> +      if (is_enable)
>>> +        error ("unknown pass %s specified in -fenable", phase_name);
>>> +      else
>>> +        error ("unknown pass %s specified in -fdisble", phase_name);
>>>
>>> likewise.
>>>
>>> +      if (!enabled_pass_uid_range_tab)
>>> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>>>
>>> instead of using a hashtable here please use a VEC indexed by
>>> the static_pass_number which shoud speed up
>>
>> Ok.  The reason I did not use it is because in most of the cases, the
>> htab will be very small -- it is determined by the number of passes
>> specified in the command line, while the VEC requires allocating const
>> size array. Not an issue as long as by default the array is not
>> allocated.
>>
>>>
>>> +static bool
>>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>>> +                                       tree func, htab_t tab)
>>> +{
>>> +  struct uid_range **slot, *range, key;
>>> +  int cgraph_uid;
>>> +
>>> +  if (!tab)
>>> +    return false;
>>> +
>>> +  key.pass = pass;
>>> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
>>> +  if (!slot || !*slot)
>>> +    return false;
>>>
>>> and simplify the code quite a bit.
>>>
>>> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>>>
>>> note that cgraph uids are recycled, so it might not be the best idea
>>> to use them as discriminator (though I don't have a good idea how
>>> to represent ranges without them).
>>
>> Yes. It is not a big problem as the blind search does not need to know
>> the id->name mapping. Once the id s found, it can be easily discovered
>> via dump.
>>
>>>
>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
>>> current_function_decl);
>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
>>> current_function_decl);
>>> +
>>>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>>>
>>> so explicitly disabling wins over explicit enabling ;)  I think this
>>> implementation detail and the fact that you always query both
>>> hints at that the interface should be like
>>>
>>> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>>
>> Done.
>>
>>>
>>> instead.
>>>
>>> Thus, please split out the function header dumping changes and rework
>>> the rest of the patch as suggested.
>>
>> Split out. The new patch is attached.
>>
>> Ok after testing is done?
>>
>> Thanks,
>>
>> David
>>
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> --
>>>> Joseph S. Myers
>>>> joseph@codesourcery.com
>>>>
>>>
>>
>

The attached are two simple follow up patches

1) the first patch does some refactorization on function header
dumping (with more information printed)

2) the second patch cleans up some pass names. Part of the cleanup
results from a previous discussion with Honza -- a) rename
'tree_profile_ipa' into 'profile', and make 'ipa_profile' and
'profile' into 'profile_estimate'. The rest of cleanups are needed to
make sure pass names are unique.

Ok for trunk?

Thanks,

David

On Fri, May 27, 2011 at 2:58 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davidxl@google.com> wrote:
>> The latest version that only exports 2 functions from passes.c.
>
> Ok with ...
>
> @@ -637,4 +637,8 @@ extern bool first_pass_instance;
>  /* Declare for plugins.  */
>  extern void do_per_function_toporder (void (*) (void *), void *);
>
> +extern void disable_pass (const char *);
> +extern void enable_pass (const char *);
> +struct function;
> +
>
> struct function forward decl removed.
>
> +  explicitly_enabled = is_pass_explicitly_enabled (pass, func);
> +  explicitly_disabled = is_pass_explicitly_disabled (pass, func);
>
> both functions inlined here and removed.
>
> +#define MAX_PASS_ID 512
>
> this removed and instead a VEC_safe_grow_cleared () or VEC_length ()
> before the accesses.
>
> +-fenable-ipa-@var{pass} @gol
> +-fenable-rtl-@var{pass} @gol
> +-fenable-rtl-@var{pass}=@var{range-list} @gol
> +-fenable-tree-@var{pass} @gol
> +-fenable-tree-@var{pass}=@var{range-list} @gol
>
> -fenable-@var{kind}-@var{pass}, etc.
>
> +@item -fdisable-@var{ipa|tree|rtl}-@var{pass}
> +@itemx -fenable-@var{ipa|tree|rtl}-@var{pass}
> +@itemx -fdisable-@var{tree|rtl}-@var{pass}=@var{range-list}
> +@itemx -fenable-@var{tree|rtl}-@var{pass}=@var{range-list}
>
> likewise.
>
> Thanks,
> Richard.
>
>> David
>>
>> On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>>>> <joseph@codesourcery.com> wrote:
>>>>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>>>>
>>>>>> Ping. The link to the message:
>>>>>>
>>>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>>>>
>>>>> I don't consider this an option handling patch.  Patches adding whole new
>>>>> features involving new options should be reviewed by maintainers for the
>>>>> part of the compiler relevant to those features (since there isn't a pass
>>>>> manager maintainer, I guess that means middle-end).
>>>>
>>>> Hmm, I suppose then you reviewed the option handling parts and they
>>>> are ok?  Those globbing options always cause headache to me.
>>>>
>>>> +-fenable-ipa-@var{pass} @gol
>>>> +-fenable-rtl-@var{pass} @gol
>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>> +-fenable-tree-@var{pass} @gol
>>>>
>>>> so, no -fenable-tree-@var{pass}=@var{range-list}?
>>>>
>>>
>>> Missed. Will add.
>>>
>>>
>>>> Does the pass name match 1:1 with the dump file name?  In which
>>>> case
>>>
>>> Yes.
>>>
>>>>
>>>> +Disable ipa 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.
>>>>
>>>> isn't true as passes that are invoked only a single time lack the number
>>>> suffix (yes, I'd really like that to be changed ...)
>>>
>>> Yes, pass with single static instance does not need number suffix.
>>>
>>>>
>>>> Please break likes also in .texi files and stick to 80 columns.
>>>
>>> Done.
>>>
>>>>  Please
>>>> document that these options are debugging options and regular
>>>> options for enabling/disabling passes should be used.  I would suggest
>>>> to group documentation differently and document -fenable-* and
>>>> -fdisable-*, thus,
>>>>
>>>> + -fdisable-@var{kind}-@var{pass}
>>>> + -fenable-@var{kind}-@var{pass}
>>>>
>>>> Even in .texi files please two spaces after a full-stop.
>>>
>>> Done
>>>
>>>>
>>>> +extern void enable_disable_pass (const char *, bool);
>>>>
>>>> I'd rather have both enable_pass and disable_pass ;)
>>>
>>> Ok.
>>>
>>>>
>>>> +struct function;
>>>> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>>>>
>>>> that's odd and probably should be split out, the function should
>>>> maybe reside in tree-pretty-print.c.
>>>
>>> Ok.
>>>
>>>>
>>>> Index: tree-ssa-loop-ivopts.c
>>>> ===================================================================
>>>> --- tree-ssa-loop-ivopts.c      (revision 173837)
>>>> +++ tree-ssa-loop-ivopts.c      (working copy)
>>>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>>>>
>>>> well - doesn't belong here ;)
>>>
>>> Sorry -- many things in the same client -- not needed with your latest
>>> change anyway.
>>>
>>>>
>>>> +static hashval_t
>>>> +passr_hash (const void *p)
>>>> +{
>>>> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
>>>> +  return htab_hash_string (s->unique_name);
>>>> +}
>>>> +
>>>> +/* Hash equal function  */
>>>> +
>>>> +static int
>>>> +passr_eq (const void *p1, const void *p2)
>>>> +{
>>>> +  const struct pass_registry *const s1 = (const struct pass_registry
>>>> *const) p1;
>>>> +  const struct pass_registry *const s2 = (const struct pass_registry
>>>> *const) p2;
>>>> +
>>>> +  return !strcmp (s1->unique_name, s2->unique_name);
>>>> +}
>>>>
>>>> you can use htab_hash_string and strcmp directly, no need for these
>>>> wrappers.
>>>
>>> The hashtable entry is not string in this case. It is pass_registry,
>>> thus the wrapper.
>>>
>>>>
>>>> +void
>>>> +register_pass_name (struct opt_pass *pass, const char *name)
>>>>
>>>> doesn't seem to need exporting, so don't and make it static.
>>>
>>> Done.
>>>
>>>>
>>>> +  if (!pass_name_tab)
>>>> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>>>>
>>>> see above, the initial size should be larger - we have 223 passes at the
>>>> moment, so use 256.
>>>
>>> Done.
>>>
>>>>
>>>> +  else
>>>> +    return; /* Ignore plugin passes.  */
>>>>
>>>> ?  You mean they might clash?
>>>
>>> Yes, name clash.
>>>
>>>>
>>>> +struct opt_pass *
>>>> +get_pass_by_name (const char *name)
>>>>
>>>> doesn't need exporting either.
>>>
>>> Done.
>>>
>>>>
>>>> +      if (is_enable)
>>>> +        error ("unrecognized option -fenable");
>>>> +      else
>>>> +        error ("unrecognized option -fdisable");
>>>>
>>>> I think that should be fatal_error - Joseph?
>>>>
>>>> +      if (is_enable)
>>>> +        error ("unknown pass %s specified in -fenable", phase_name);
>>>> +      else
>>>> +        error ("unknown pass %s specified in -fdisble", phase_name);
>>>>
>>>> likewise.
>>>>
>>>> +      if (!enabled_pass_uid_range_tab)
>>>> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>>>>
>>>> instead of using a hashtable here please use a VEC indexed by
>>>> the static_pass_number which shoud speed up
>>>
>>> Ok.  The reason I did not use it is because in most of the cases, the
>>> htab will be very small -- it is determined by the number of passes
>>> specified in the command line, while the VEC requires allocating const
>>> size array. Not an issue as long as by default the array is not
>>> allocated.
>>>
>>>>
>>>> +static bool
>>>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>>>> +                                       tree func, htab_t tab)
>>>> +{
>>>> +  struct uid_range **slot, *range, key;
>>>> +  int cgraph_uid;
>>>> +
>>>> +  if (!tab)
>>>> +    return false;
>>>> +
>>>> +  key.pass = pass;
>>>> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
>>>> +  if (!slot || !*slot)
>>>> +    return false;
>>>>
>>>> and simplify the code quite a bit.
>>>>
>>>> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>>>>
>>>> note that cgraph uids are recycled, so it might not be the best idea
>>>> to use them as discriminator (though I don't have a good idea how
>>>> to represent ranges without them).
>>>
>>> Yes. It is not a big problem as the blind search does not need to know
>>> the id->name mapping. Once the id s found, it can be easily discovered
>>> via dump.
>>>
>>>>
>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
>>>> current_function_decl);
>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
>>>> current_function_decl);
>>>> +
>>>>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>>>>
>>>> so explicitly disabling wins over explicit enabling ;)  I think this
>>>> implementation detail and the fact that you always query both
>>>> hints at that the interface should be like
>>>>
>>>> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>>>
>>> Done.
>>>
>>>>
>>>> instead.
>>>>
>>>> Thus, please split out the function header dumping changes and rework
>>>> the rest of the patch as suggested.
>>>
>>> Split out. The new patch is attached.
>>>
>>> Ok after testing is done?
>>>
>>> Thanks,
>>>
>>> David
>>>
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> --
>>>>> Joseph S. Myers
>>>>> joseph@codesourcery.com
>>>>>
>>>>
>>>
>>
>

This is the complete patch for pass name fixes (with test case changes).

David


On Mon, May 30, 2011 at 1:16 PM, Xinliang David Li <davidxl@google.com> wrote:
> The attached are two simple follow up patches
>
> 1) the first patch does some refactorization on function header
> dumping (with more information printed)
>
> 2) the second patch cleans up some pass names. Part of the cleanup
> results from a previous discussion with Honza -- a) rename
> 'tree_profile_ipa' into 'profile', and make 'ipa_profile' and
> 'profile' into 'profile_estimate'. The rest of cleanups are needed to
> make sure pass names are unique.
>
> Ok for trunk?
>
> Thanks,
>
> David
>
> On Fri, May 27, 2011 at 2:58 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> The latest version that only exports 2 functions from passes.c.
>>
>> Ok with ...
>>
>> @@ -637,4 +637,8 @@ extern bool first_pass_instance;
>>  /* Declare for plugins.  */
>>  extern void do_per_function_toporder (void (*) (void *), void *);
>>
>> +extern void disable_pass (const char *);
>> +extern void enable_pass (const char *);
>> +struct function;
>> +
>>
>> struct function forward decl removed.
>>
>> +  explicitly_enabled = is_pass_explicitly_enabled (pass, func);
>> +  explicitly_disabled = is_pass_explicitly_disabled (pass, func);
>>
>> both functions inlined here and removed.
>>
>> +#define MAX_PASS_ID 512
>>
>> this removed and instead a VEC_safe_grow_cleared () or VEC_length ()
>> before the accesses.
>>
>> +-fenable-ipa-@var{pass} @gol
>> +-fenable-rtl-@var{pass} @gol
>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>> +-fenable-tree-@var{pass} @gol
>> +-fenable-tree-@var{pass}=@var{range-list} @gol
>>
>> -fenable-@var{kind}-@var{pass}, etc.
>>
>> +@item -fdisable-@var{ipa|tree|rtl}-@var{pass}
>> +@itemx -fenable-@var{ipa|tree|rtl}-@var{pass}
>> +@itemx -fdisable-@var{tree|rtl}-@var{pass}=@var{range-list}
>> +@itemx -fenable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>
>> likewise.
>>
>> Thanks,
>> Richard.
>>
>>> David
>>>
>>> On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>>>>> <joseph@codesourcery.com> wrote:
>>>>>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>>>>>
>>>>>>> Ping. The link to the message:
>>>>>>>
>>>>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>>>>>
>>>>>> I don't consider this an option handling patch.  Patches adding whole new
>>>>>> features involving new options should be reviewed by maintainers for the
>>>>>> part of the compiler relevant to those features (since there isn't a pass
>>>>>> manager maintainer, I guess that means middle-end).
>>>>>
>>>>> Hmm, I suppose then you reviewed the option handling parts and they
>>>>> are ok?  Those globbing options always cause headache to me.
>>>>>
>>>>> +-fenable-ipa-@var{pass} @gol
>>>>> +-fenable-rtl-@var{pass} @gol
>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>>> +-fenable-tree-@var{pass} @gol
>>>>>
>>>>> so, no -fenable-tree-@var{pass}=@var{range-list}?
>>>>>
>>>>
>>>> Missed. Will add.
>>>>
>>>>
>>>>> Does the pass name match 1:1 with the dump file name?  In which
>>>>> case
>>>>
>>>> Yes.
>>>>
>>>>>
>>>>> +Disable ipa 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.
>>>>>
>>>>> isn't true as passes that are invoked only a single time lack the number
>>>>> suffix (yes, I'd really like that to be changed ...)
>>>>
>>>> Yes, pass with single static instance does not need number suffix.
>>>>
>>>>>
>>>>> Please break likes also in .texi files and stick to 80 columns.
>>>>
>>>> Done.
>>>>
>>>>>  Please
>>>>> document that these options are debugging options and regular
>>>>> options for enabling/disabling passes should be used.  I would suggest
>>>>> to group documentation differently and document -fenable-* and
>>>>> -fdisable-*, thus,
>>>>>
>>>>> + -fdisable-@var{kind}-@var{pass}
>>>>> + -fenable-@var{kind}-@var{pass}
>>>>>
>>>>> Even in .texi files please two spaces after a full-stop.
>>>>
>>>> Done
>>>>
>>>>>
>>>>> +extern void enable_disable_pass (const char *, bool);
>>>>>
>>>>> I'd rather have both enable_pass and disable_pass ;)
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>> +struct function;
>>>>> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>>>>>
>>>>> that's odd and probably should be split out, the function should
>>>>> maybe reside in tree-pretty-print.c.
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>> Index: tree-ssa-loop-ivopts.c
>>>>> ===================================================================
>>>>> --- tree-ssa-loop-ivopts.c      (revision 173837)
>>>>> +++ tree-ssa-loop-ivopts.c      (working copy)
>>>>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>>>>>
>>>>> well - doesn't belong here ;)
>>>>
>>>> Sorry -- many things in the same client -- not needed with your latest
>>>> change anyway.
>>>>
>>>>>
>>>>> +static hashval_t
>>>>> +passr_hash (const void *p)
>>>>> +{
>>>>> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
>>>>> +  return htab_hash_string (s->unique_name);
>>>>> +}
>>>>> +
>>>>> +/* Hash equal function  */
>>>>> +
>>>>> +static int
>>>>> +passr_eq (const void *p1, const void *p2)
>>>>> +{
>>>>> +  const struct pass_registry *const s1 = (const struct pass_registry
>>>>> *const) p1;
>>>>> +  const struct pass_registry *const s2 = (const struct pass_registry
>>>>> *const) p2;
>>>>> +
>>>>> +  return !strcmp (s1->unique_name, s2->unique_name);
>>>>> +}
>>>>>
>>>>> you can use htab_hash_string and strcmp directly, no need for these
>>>>> wrappers.
>>>>
>>>> The hashtable entry is not string in this case. It is pass_registry,
>>>> thus the wrapper.
>>>>
>>>>>
>>>>> +void
>>>>> +register_pass_name (struct opt_pass *pass, const char *name)
>>>>>
>>>>> doesn't seem to need exporting, so don't and make it static.
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> +  if (!pass_name_tab)
>>>>> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>>>>>
>>>>> see above, the initial size should be larger - we have 223 passes at the
>>>>> moment, so use 256.
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> +  else
>>>>> +    return; /* Ignore plugin passes.  */
>>>>>
>>>>> ?  You mean they might clash?
>>>>
>>>> Yes, name clash.
>>>>
>>>>>
>>>>> +struct opt_pass *
>>>>> +get_pass_by_name (const char *name)
>>>>>
>>>>> doesn't need exporting either.
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> +      if (is_enable)
>>>>> +        error ("unrecognized option -fenable");
>>>>> +      else
>>>>> +        error ("unrecognized option -fdisable");
>>>>>
>>>>> I think that should be fatal_error - Joseph?
>>>>>
>>>>> +      if (is_enable)
>>>>> +        error ("unknown pass %s specified in -fenable", phase_name);
>>>>> +      else
>>>>> +        error ("unknown pass %s specified in -fdisble", phase_name);
>>>>>
>>>>> likewise.
>>>>>
>>>>> +      if (!enabled_pass_uid_range_tab)
>>>>> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>>>>>
>>>>> instead of using a hashtable here please use a VEC indexed by
>>>>> the static_pass_number which shoud speed up
>>>>
>>>> Ok.  The reason I did not use it is because in most of the cases, the
>>>> htab will be very small -- it is determined by the number of passes
>>>> specified in the command line, while the VEC requires allocating const
>>>> size array. Not an issue as long as by default the array is not
>>>> allocated.
>>>>
>>>>>
>>>>> +static bool
>>>>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>>>>> +                                       tree func, htab_t tab)
>>>>> +{
>>>>> +  struct uid_range **slot, *range, key;
>>>>> +  int cgraph_uid;
>>>>> +
>>>>> +  if (!tab)
>>>>> +    return false;
>>>>> +
>>>>> +  key.pass = pass;
>>>>> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
>>>>> +  if (!slot || !*slot)
>>>>> +    return false;
>>>>>
>>>>> and simplify the code quite a bit.
>>>>>
>>>>> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>>>>>
>>>>> note that cgraph uids are recycled, so it might not be the best idea
>>>>> to use them as discriminator (though I don't have a good idea how
>>>>> to represent ranges without them).
>>>>
>>>> Yes. It is not a big problem as the blind search does not need to know
>>>> the id->name mapping. Once the id s found, it can be easily discovered
>>>> via dump.
>>>>
>>>>>
>>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
>>>>> current_function_decl);
>>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
>>>>> current_function_decl);
>>>>> +
>>>>>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>>>>>
>>>>> so explicitly disabling wins over explicit enabling ;)  I think this
>>>>> implementation detail and the fact that you always query both
>>>>> hints at that the interface should be like
>>>>>
>>>>> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> instead.
>>>>>
>>>>> Thus, please split out the function header dumping changes and rework
>>>>> the rest of the patch as suggested.
>>>>
>>>> Split out. The new patch is attached.
>>>>
>>>> Ok after testing is done?
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>>>
>>>>> Thanks,
>>>>> Richard.
>>>>>
>>>>>> --
>>>>>> Joseph S. Myers
>>>>>> joseph@codesourcery.com
>>>>>>
>>>>>
>>>>
>>>
>>
>

On Mon, May 30, 2011 at 10:16 PM, Xinliang David Li <davidxl@google.com> wrote:
> The attached are two simple follow up patches
>
> 1) the first patch does some refactorization on function header
> dumping (with more information printed)
>
> 2) the second patch cleans up some pass names. Part of the cleanup
> results from a previous discussion with Honza -- a) rename
> 'tree_profile_ipa' into 'profile', and make 'ipa_profile' and
> 'profile' into 'profile_estimate'. The rest of cleanups are needed to
> make sure pass names are unique.
>
> Ok for trunk?

+
+void
+pass_dump_function_header (FILE *dump_file, tree fdecl, struct function *fun)

This function needs documentation, the ChangeLog entry misses
the tree-pretty-print.h change.

+struct function;

instead of this please include coretypes.h from tree-pretty-print.h
and add the struct function forward declaration there if it isn't already
present.

You change the output of the header, so please make sure you
have bootstrapped and tested with _all_ languages included
(and also watch for bugreports for target specific bugs).

+  fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d, uid=%d)",
+           dname, aname, fun->funcdef_no, node->uid);

I see no point in dumping funcdef_no - it wasn't dumped before in
any place.  Instead I miss dumping of the DECL_UID and thus
a more specific 'uid', like 'cgraph-uid'.

+  aname = (IDENTIFIER_POINTER
+          (DECL_ASSEMBLER_NAME (fdecl)));

using DECL_ASSEMBLER_NAME is bad - it might trigger computation
of DECL_ASSEMBLER_NAME which certainly shouldn't be done
only for dumping purposes.  Instead do sth like

   if (DECL_ASSEMBLER_NAME_SET_P (fdecl))
     aname = DECL_ASSEMBLER_NAME (fdecl);
   else
     aname = '<unset-asm-name>';

and please also watch for cgraph_get_node returning NULL.

Also please call the function dump_function_header instead of
pass_dump_function_header.

Please re-post with appropriate changes.

Thanks,
Richard.

> Thanks,
>
> David
>
> On Fri, May 27, 2011 at 2:58 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> The latest version that only exports 2 functions from passes.c.
>>
>> Ok with ...
>>
>> @@ -637,4 +637,8 @@ extern bool first_pass_instance;
>>  /* Declare for plugins.  */
>>  extern void do_per_function_toporder (void (*) (void *), void *);
>>
>> +extern void disable_pass (const char *);
>> +extern void enable_pass (const char *);
>> +struct function;
>> +
>>
>> struct function forward decl removed.
>>
>> +  explicitly_enabled = is_pass_explicitly_enabled (pass, func);
>> +  explicitly_disabled = is_pass_explicitly_disabled (pass, func);
>>
>> both functions inlined here and removed.
>>
>> +#define MAX_PASS_ID 512
>>
>> this removed and instead a VEC_safe_grow_cleared () or VEC_length ()
>> before the accesses.
>>
>> +-fenable-ipa-@var{pass} @gol
>> +-fenable-rtl-@var{pass} @gol
>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>> +-fenable-tree-@var{pass} @gol
>> +-fenable-tree-@var{pass}=@var{range-list} @gol
>>
>> -fenable-@var{kind}-@var{pass}, etc.
>>
>> +@item -fdisable-@var{ipa|tree|rtl}-@var{pass}
>> +@itemx -fenable-@var{ipa|tree|rtl}-@var{pass}
>> +@itemx -fdisable-@var{tree|rtl}-@var{pass}=@var{range-list}
>> +@itemx -fenable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>
>> likewise.
>>
>> Thanks,
>> Richard.
>>
>>> David
>>>
>>> On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>>>>> <joseph@codesourcery.com> wrote:
>>>>>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>>>>>
>>>>>>> Ping. The link to the message:
>>>>>>>
>>>>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>>>>>
>>>>>> I don't consider this an option handling patch.  Patches adding whole new
>>>>>> features involving new options should be reviewed by maintainers for the
>>>>>> part of the compiler relevant to those features (since there isn't a pass
>>>>>> manager maintainer, I guess that means middle-end).
>>>>>
>>>>> Hmm, I suppose then you reviewed the option handling parts and they
>>>>> are ok?  Those globbing options always cause headache to me.
>>>>>
>>>>> +-fenable-ipa-@var{pass} @gol
>>>>> +-fenable-rtl-@var{pass} @gol
>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>>> +-fenable-tree-@var{pass} @gol
>>>>>
>>>>> so, no -fenable-tree-@var{pass}=@var{range-list}?
>>>>>
>>>>
>>>> Missed. Will add.
>>>>
>>>>
>>>>> Does the pass name match 1:1 with the dump file name?  In which
>>>>> case
>>>>
>>>> Yes.
>>>>
>>>>>
>>>>> +Disable ipa 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.
>>>>>
>>>>> isn't true as passes that are invoked only a single time lack the number
>>>>> suffix (yes, I'd really like that to be changed ...)
>>>>
>>>> Yes, pass with single static instance does not need number suffix.
>>>>
>>>>>
>>>>> Please break likes also in .texi files and stick to 80 columns.
>>>>
>>>> Done.
>>>>
>>>>>  Please
>>>>> document that these options are debugging options and regular
>>>>> options for enabling/disabling passes should be used.  I would suggest
>>>>> to group documentation differently and document -fenable-* and
>>>>> -fdisable-*, thus,
>>>>>
>>>>> + -fdisable-@var{kind}-@var{pass}
>>>>> + -fenable-@var{kind}-@var{pass}
>>>>>
>>>>> Even in .texi files please two spaces after a full-stop.
>>>>
>>>> Done
>>>>
>>>>>
>>>>> +extern void enable_disable_pass (const char *, bool);
>>>>>
>>>>> I'd rather have both enable_pass and disable_pass ;)
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>> +struct function;
>>>>> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>>>>>
>>>>> that's odd and probably should be split out, the function should
>>>>> maybe reside in tree-pretty-print.c.
>>>>
>>>> Ok.
>>>>
>>>>>
>>>>> Index: tree-ssa-loop-ivopts.c
>>>>> ===================================================================
>>>>> --- tree-ssa-loop-ivopts.c      (revision 173837)
>>>>> +++ tree-ssa-loop-ivopts.c      (working copy)
>>>>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>>>>>
>>>>> well - doesn't belong here ;)
>>>>
>>>> Sorry -- many things in the same client -- not needed with your latest
>>>> change anyway.
>>>>
>>>>>
>>>>> +static hashval_t
>>>>> +passr_hash (const void *p)
>>>>> +{
>>>>> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
>>>>> +  return htab_hash_string (s->unique_name);
>>>>> +}
>>>>> +
>>>>> +/* Hash equal function  */
>>>>> +
>>>>> +static int
>>>>> +passr_eq (const void *p1, const void *p2)
>>>>> +{
>>>>> +  const struct pass_registry *const s1 = (const struct pass_registry
>>>>> *const) p1;
>>>>> +  const struct pass_registry *const s2 = (const struct pass_registry
>>>>> *const) p2;
>>>>> +
>>>>> +  return !strcmp (s1->unique_name, s2->unique_name);
>>>>> +}
>>>>>
>>>>> you can use htab_hash_string and strcmp directly, no need for these
>>>>> wrappers.
>>>>
>>>> The hashtable entry is not string in this case. It is pass_registry,
>>>> thus the wrapper.
>>>>
>>>>>
>>>>> +void
>>>>> +register_pass_name (struct opt_pass *pass, const char *name)
>>>>>
>>>>> doesn't seem to need exporting, so don't and make it static.
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> +  if (!pass_name_tab)
>>>>> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>>>>>
>>>>> see above, the initial size should be larger - we have 223 passes at the
>>>>> moment, so use 256.
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> +  else
>>>>> +    return; /* Ignore plugin passes.  */
>>>>>
>>>>> ?  You mean they might clash?
>>>>
>>>> Yes, name clash.
>>>>
>>>>>
>>>>> +struct opt_pass *
>>>>> +get_pass_by_name (const char *name)
>>>>>
>>>>> doesn't need exporting either.
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> +      if (is_enable)
>>>>> +        error ("unrecognized option -fenable");
>>>>> +      else
>>>>> +        error ("unrecognized option -fdisable");
>>>>>
>>>>> I think that should be fatal_error - Joseph?
>>>>>
>>>>> +      if (is_enable)
>>>>> +        error ("unknown pass %s specified in -fenable", phase_name);
>>>>> +      else
>>>>> +        error ("unknown pass %s specified in -fdisble", phase_name);
>>>>>
>>>>> likewise.
>>>>>
>>>>> +      if (!enabled_pass_uid_range_tab)
>>>>> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>>>>>
>>>>> instead of using a hashtable here please use a VEC indexed by
>>>>> the static_pass_number which shoud speed up
>>>>
>>>> Ok.  The reason I did not use it is because in most of the cases, the
>>>> htab will be very small -- it is determined by the number of passes
>>>> specified in the command line, while the VEC requires allocating const
>>>> size array. Not an issue as long as by default the array is not
>>>> allocated.
>>>>
>>>>>
>>>>> +static bool
>>>>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>>>>> +                                       tree func, htab_t tab)
>>>>> +{
>>>>> +  struct uid_range **slot, *range, key;
>>>>> +  int cgraph_uid;
>>>>> +
>>>>> +  if (!tab)
>>>>> +    return false;
>>>>> +
>>>>> +  key.pass = pass;
>>>>> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
>>>>> +  if (!slot || !*slot)
>>>>> +    return false;
>>>>>
>>>>> and simplify the code quite a bit.
>>>>>
>>>>> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>>>>>
>>>>> note that cgraph uids are recycled, so it might not be the best idea
>>>>> to use them as discriminator (though I don't have a good idea how
>>>>> to represent ranges without them).
>>>>
>>>> Yes. It is not a big problem as the blind search does not need to know
>>>> the id->name mapping. Once the id s found, it can be easily discovered
>>>> via dump.
>>>>
>>>>>
>>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
>>>>> current_function_decl);
>>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
>>>>> current_function_decl);
>>>>> +
>>>>>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>>>>>
>>>>> so explicitly disabling wins over explicit enabling ;)  I think this
>>>>> implementation detail and the fact that you always query both
>>>>> hints at that the interface should be like
>>>>>
>>>>> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>>>>
>>>> Done.
>>>>
>>>>>
>>>>> instead.
>>>>>
>>>>> Thus, please split out the function header dumping changes and rework
>>>>> the rest of the patch as suggested.
>>>>
>>>> Split out. The new patch is attached.
>>>>
>>>> Ok after testing is done?
>>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>>>
>>>>> Thanks,
>>>>> Richard.
>>>>>
>>>>>> --
>>>>>> Joseph S. Myers
>>>>>> joseph@codesourcery.com
>>>>>>
>>>>>
>>>>
>>>
>>
>

On Mon, May 30, 2011 at 11:44 PM, Xinliang David Li <davidxl@google.com> wrote:
> This is the complete patch for pass name fixes (with test case changes).

This is ok if Honza thinks the profile pass names make more sense this
way.

Thanks,
Richard.

> David
>
>
> On Mon, May 30, 2011 at 1:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>> The attached are two simple follow up patches
>>
>> 1) the first patch does some refactorization on function header
>> dumping (with more information printed)
>>
>> 2) the second patch cleans up some pass names. Part of the cleanup
>> results from a previous discussion with Honza -- a) rename
>> 'tree_profile_ipa' into 'profile', and make 'ipa_profile' and
>> 'profile' into 'profile_estimate'. The rest of cleanups are needed to
>> make sure pass names are unique.
>>
>> Ok for trunk?
>>
>> Thanks,
>>
>> David
>>
>> On Fri, May 27, 2011 at 2:58 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> The latest version that only exports 2 functions from passes.c.
>>>
>>> Ok with ...
>>>
>>> @@ -637,4 +637,8 @@ extern bool first_pass_instance;
>>>  /* Declare for plugins.  */
>>>  extern void do_per_function_toporder (void (*) (void *), void *);
>>>
>>> +extern void disable_pass (const char *);
>>> +extern void enable_pass (const char *);
>>> +struct function;
>>> +
>>>
>>> struct function forward decl removed.
>>>
>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass, func);
>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass, func);
>>>
>>> both functions inlined here and removed.
>>>
>>> +#define MAX_PASS_ID 512
>>>
>>> this removed and instead a VEC_safe_grow_cleared () or VEC_length ()
>>> before the accesses.
>>>
>>> +-fenable-ipa-@var{pass} @gol
>>> +-fenable-rtl-@var{pass} @gol
>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>> +-fenable-tree-@var{pass} @gol
>>> +-fenable-tree-@var{pass}=@var{range-list} @gol
>>>
>>> -fenable-@var{kind}-@var{pass}, etc.
>>>
>>> +@item -fdisable-@var{ipa|tree|rtl}-@var{pass}
>>> +@itemx -fenable-@var{ipa|tree|rtl}-@var{pass}
>>> +@itemx -fdisable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>> +@itemx -fenable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>>
>>> likewise.
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> David
>>>>
>>>> On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>>>>>> <joseph@codesourcery.com> wrote:
>>>>>>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>>>>>>
>>>>>>>> Ping. The link to the message:
>>>>>>>>
>>>>>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>>>>>>
>>>>>>> I don't consider this an option handling patch.  Patches adding whole new
>>>>>>> features involving new options should be reviewed by maintainers for the
>>>>>>> part of the compiler relevant to those features (since there isn't a pass
>>>>>>> manager maintainer, I guess that means middle-end).
>>>>>>
>>>>>> Hmm, I suppose then you reviewed the option handling parts and they
>>>>>> are ok?  Those globbing options always cause headache to me.
>>>>>>
>>>>>> +-fenable-ipa-@var{pass} @gol
>>>>>> +-fenable-rtl-@var{pass} @gol
>>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>>>> +-fenable-tree-@var{pass} @gol
>>>>>>
>>>>>> so, no -fenable-tree-@var{pass}=@var{range-list}?
>>>>>>
>>>>>
>>>>> Missed. Will add.
>>>>>
>>>>>
>>>>>> Does the pass name match 1:1 with the dump file name?  In which
>>>>>> case
>>>>>
>>>>> Yes.
>>>>>
>>>>>>
>>>>>> +Disable ipa 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.
>>>>>>
>>>>>> isn't true as passes that are invoked only a single time lack the number
>>>>>> suffix (yes, I'd really like that to be changed ...)
>>>>>
>>>>> Yes, pass with single static instance does not need number suffix.
>>>>>
>>>>>>
>>>>>> Please break likes also in .texi files and stick to 80 columns.
>>>>>
>>>>> Done.
>>>>>
>>>>>>  Please
>>>>>> document that these options are debugging options and regular
>>>>>> options for enabling/disabling passes should be used.  I would suggest
>>>>>> to group documentation differently and document -fenable-* and
>>>>>> -fdisable-*, thus,
>>>>>>
>>>>>> + -fdisable-@var{kind}-@var{pass}
>>>>>> + -fenable-@var{kind}-@var{pass}
>>>>>>
>>>>>> Even in .texi files please two spaces after a full-stop.
>>>>>
>>>>> Done
>>>>>
>>>>>>
>>>>>> +extern void enable_disable_pass (const char *, bool);
>>>>>>
>>>>>> I'd rather have both enable_pass and disable_pass ;)
>>>>>
>>>>> Ok.
>>>>>
>>>>>>
>>>>>> +struct function;
>>>>>> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>>>>>>
>>>>>> that's odd and probably should be split out, the function should
>>>>>> maybe reside in tree-pretty-print.c.
>>>>>
>>>>> Ok.
>>>>>
>>>>>>
>>>>>> Index: tree-ssa-loop-ivopts.c
>>>>>> ===================================================================
>>>>>> --- tree-ssa-loop-ivopts.c      (revision 173837)
>>>>>> +++ tree-ssa-loop-ivopts.c      (working copy)
>>>>>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>>>>>>
>>>>>> well - doesn't belong here ;)
>>>>>
>>>>> Sorry -- many things in the same client -- not needed with your latest
>>>>> change anyway.
>>>>>
>>>>>>
>>>>>> +static hashval_t
>>>>>> +passr_hash (const void *p)
>>>>>> +{
>>>>>> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
>>>>>> +  return htab_hash_string (s->unique_name);
>>>>>> +}
>>>>>> +
>>>>>> +/* Hash equal function  */
>>>>>> +
>>>>>> +static int
>>>>>> +passr_eq (const void *p1, const void *p2)
>>>>>> +{
>>>>>> +  const struct pass_registry *const s1 = (const struct pass_registry
>>>>>> *const) p1;
>>>>>> +  const struct pass_registry *const s2 = (const struct pass_registry
>>>>>> *const) p2;
>>>>>> +
>>>>>> +  return !strcmp (s1->unique_name, s2->unique_name);
>>>>>> +}
>>>>>>
>>>>>> you can use htab_hash_string and strcmp directly, no need for these
>>>>>> wrappers.
>>>>>
>>>>> The hashtable entry is not string in this case. It is pass_registry,
>>>>> thus the wrapper.
>>>>>
>>>>>>
>>>>>> +void
>>>>>> +register_pass_name (struct opt_pass *pass, const char *name)
>>>>>>
>>>>>> doesn't seem to need exporting, so don't and make it static.
>>>>>
>>>>> Done.
>>>>>
>>>>>>
>>>>>> +  if (!pass_name_tab)
>>>>>> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>>>>>>
>>>>>> see above, the initial size should be larger - we have 223 passes at the
>>>>>> moment, so use 256.
>>>>>
>>>>> Done.
>>>>>
>>>>>>
>>>>>> +  else
>>>>>> +    return; /* Ignore plugin passes.  */
>>>>>>
>>>>>> ?  You mean they might clash?
>>>>>
>>>>> Yes, name clash.
>>>>>
>>>>>>
>>>>>> +struct opt_pass *
>>>>>> +get_pass_by_name (const char *name)
>>>>>>
>>>>>> doesn't need exporting either.
>>>>>
>>>>> Done.
>>>>>
>>>>>>
>>>>>> +      if (is_enable)
>>>>>> +        error ("unrecognized option -fenable");
>>>>>> +      else
>>>>>> +        error ("unrecognized option -fdisable");
>>>>>>
>>>>>> I think that should be fatal_error - Joseph?
>>>>>>
>>>>>> +      if (is_enable)
>>>>>> +        error ("unknown pass %s specified in -fenable", phase_name);
>>>>>> +      else
>>>>>> +        error ("unknown pass %s specified in -fdisble", phase_name);
>>>>>>
>>>>>> likewise.
>>>>>>
>>>>>> +      if (!enabled_pass_uid_range_tab)
>>>>>> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>>>>>>
>>>>>> instead of using a hashtable here please use a VEC indexed by
>>>>>> the static_pass_number which shoud speed up
>>>>>
>>>>> Ok.  The reason I did not use it is because in most of the cases, the
>>>>> htab will be very small -- it is determined by the number of passes
>>>>> specified in the command line, while the VEC requires allocating const
>>>>> size array. Not an issue as long as by default the array is not
>>>>> allocated.
>>>>>
>>>>>>
>>>>>> +static bool
>>>>>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>>>>>> +                                       tree func, htab_t tab)
>>>>>> +{
>>>>>> +  struct uid_range **slot, *range, key;
>>>>>> +  int cgraph_uid;
>>>>>> +
>>>>>> +  if (!tab)
>>>>>> +    return false;
>>>>>> +
>>>>>> +  key.pass = pass;
>>>>>> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
>>>>>> +  if (!slot || !*slot)
>>>>>> +    return false;
>>>>>>
>>>>>> and simplify the code quite a bit.
>>>>>>
>>>>>> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>>>>>>
>>>>>> note that cgraph uids are recycled, so it might not be the best idea
>>>>>> to use them as discriminator (though I don't have a good idea how
>>>>>> to represent ranges without them).
>>>>>
>>>>> Yes. It is not a big problem as the blind search does not need to know
>>>>> the id->name mapping. Once the id s found, it can be easily discovered
>>>>> via dump.
>>>>>
>>>>>>
>>>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
>>>>>> current_function_decl);
>>>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
>>>>>> current_function_decl);
>>>>>> +
>>>>>>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>>>>>>
>>>>>> so explicitly disabling wins over explicit enabling ;)  I think this
>>>>>> implementation detail and the fact that you always query both
>>>>>> hints at that the interface should be like
>>>>>>
>>>>>> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>>>>>
>>>>> Done.
>>>>>
>>>>>>
>>>>>> instead.
>>>>>>
>>>>>> Thus, please split out the function header dumping changes and rework
>>>>>> the rest of the patch as suggested.
>>>>>
>>>>> Split out. The new patch is attached.
>>>>>
>>>>> Ok after testing is done?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> David
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Richard.
>>>>>>
>>>>>>> --
>>>>>>> Joseph S. Myers
>>>>>>> joseph@codesourcery.com
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

On Tue, May 31, 2011 at 2:05 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, May 30, 2011 at 10:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>> The attached are two simple follow up patches
>>
>> 1) the first patch does some refactorization on function header
>> dumping (with more information printed)
>>
>> 2) the second patch cleans up some pass names. Part of the cleanup
>> results from a previous discussion with Honza -- a) rename
>> 'tree_profile_ipa' into 'profile', and make 'ipa_profile' and
>> 'profile' into 'profile_estimate'. The rest of cleanups are needed to
>> make sure pass names are unique.
>>
>> Ok for trunk?
>
> +
> +void
> +pass_dump_function_header (FILE *dump_file, tree fdecl, struct function *fun)
>
> This function needs documentation, the ChangeLog entry misses
> the tree-pretty-print.h change.
>
> +struct function;
>
> instead of this please include coretypes.h from tree-pretty-print.h
> and add the struct function forward declaration there if it isn't already
> present.
>
> You change the output of the header, so please make sure you
> have bootstrapped and tested with _all_ languages included
> (and also watch for bugreports for target specific bugs).
>

Ok.

> +  fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d, uid=%d)",
> +           dname, aname, fun->funcdef_no, node->uid);
>
> I see no point in dumping funcdef_no - it wasn't dumped before in
> any place.  Instead I miss dumping of the DECL_UID and thus
> a more specific 'uid', like 'cgraph-uid'.

Ok will add decl_uid. Funcdef_no is very useful for debugging FDO
coverage mismatch related problems as it is the id used in profile
hashing.

>
> +  aname = (IDENTIFIER_POINTER
> +          (DECL_ASSEMBLER_NAME (fdecl)));
>
> using DECL_ASSEMBLER_NAME is bad - it might trigger computation
> of DECL_ASSEMBLER_NAME which certainly shouldn't be done
> only for dumping purposes.  Instead do sth like
>
>   if (DECL_ASSEMBLER_NAME_SET_P (fdecl))
>     aname = DECL_ASSEMBLER_NAME (fdecl);
>   else
>     aname = '<unset-asm-name>';

Ok.

>
> and please also watch for cgraph_get_node returning NULL.
>
> Also please call the function dump_function_header instead of
> pass_dump_function_header.
>

Ok.

Thanks,

David
> Please re-post with appropriate changes.
>
> Thanks,
> Richard.
>
>> Thanks,
>>
>> David
>>
>> On Fri, May 27, 2011 at 2:58 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>> The latest version that only exports 2 functions from passes.c.
>>>
>>> Ok with ...
>>>
>>> @@ -637,4 +637,8 @@ extern bool first_pass_instance;
>>>  /* Declare for plugins.  */
>>>  extern void do_per_function_toporder (void (*) (void *), void *);
>>>
>>> +extern void disable_pass (const char *);
>>> +extern void enable_pass (const char *);
>>> +struct function;
>>> +
>>>
>>> struct function forward decl removed.
>>>
>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass, func);
>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass, func);
>>>
>>> both functions inlined here and removed.
>>>
>>> +#define MAX_PASS_ID 512
>>>
>>> this removed and instead a VEC_safe_grow_cleared () or VEC_length ()
>>> before the accesses.
>>>
>>> +-fenable-ipa-@var{pass} @gol
>>> +-fenable-rtl-@var{pass} @gol
>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>> +-fenable-tree-@var{pass} @gol
>>> +-fenable-tree-@var{pass}=@var{range-list} @gol
>>>
>>> -fenable-@var{kind}-@var{pass}, etc.
>>>
>>> +@item -fdisable-@var{ipa|tree|rtl}-@var{pass}
>>> +@itemx -fenable-@var{ipa|tree|rtl}-@var{pass}
>>> +@itemx -fdisable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>> +@itemx -fenable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>>
>>> likewise.
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> David
>>>>
>>>> On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>>>>>> <joseph@codesourcery.com> wrote:
>>>>>>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>>>>>>
>>>>>>>> Ping. The link to the message:
>>>>>>>>
>>>>>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>>>>>>
>>>>>>> I don't consider this an option handling patch.  Patches adding whole new
>>>>>>> features involving new options should be reviewed by maintainers for the
>>>>>>> part of the compiler relevant to those features (since there isn't a pass
>>>>>>> manager maintainer, I guess that means middle-end).
>>>>>>
>>>>>> Hmm, I suppose then you reviewed the option handling parts and they
>>>>>> are ok?  Those globbing options always cause headache to me.
>>>>>>
>>>>>> +-fenable-ipa-@var{pass} @gol
>>>>>> +-fenable-rtl-@var{pass} @gol
>>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>>>> +-fenable-tree-@var{pass} @gol
>>>>>>
>>>>>> so, no -fenable-tree-@var{pass}=@var{range-list}?
>>>>>>
>>>>>
>>>>> Missed. Will add.
>>>>>
>>>>>
>>>>>> Does the pass name match 1:1 with the dump file name?  In which
>>>>>> case
>>>>>
>>>>> Yes.
>>>>>
>>>>>>
>>>>>> +Disable ipa 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.
>>>>>>
>>>>>> isn't true as passes that are invoked only a single time lack the number
>>>>>> suffix (yes, I'd really like that to be changed ...)
>>>>>
>>>>> Yes, pass with single static instance does not need number suffix.
>>>>>
>>>>>>
>>>>>> Please break likes also in .texi files and stick to 80 columns.
>>>>>
>>>>> Done.
>>>>>
>>>>>>  Please
>>>>>> document that these options are debugging options and regular
>>>>>> options for enabling/disabling passes should be used.  I would suggest
>>>>>> to group documentation differently and document -fenable-* and
>>>>>> -fdisable-*, thus,
>>>>>>
>>>>>> + -fdisable-@var{kind}-@var{pass}
>>>>>> + -fenable-@var{kind}-@var{pass}
>>>>>>
>>>>>> Even in .texi files please two spaces after a full-stop.
>>>>>
>>>>> Done
>>>>>
>>>>>>
>>>>>> +extern void enable_disable_pass (const char *, bool);
>>>>>>
>>>>>> I'd rather have both enable_pass and disable_pass ;)
>>>>>
>>>>> Ok.
>>>>>
>>>>>>
>>>>>> +struct function;
>>>>>> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>>>>>>
>>>>>> that's odd and probably should be split out, the function should
>>>>>> maybe reside in tree-pretty-print.c.
>>>>>
>>>>> Ok.
>>>>>
>>>>>>
>>>>>> Index: tree-ssa-loop-ivopts.c
>>>>>> ===================================================================
>>>>>> --- tree-ssa-loop-ivopts.c      (revision 173837)
>>>>>> +++ tree-ssa-loop-ivopts.c      (working copy)
>>>>>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>>>>>>
>>>>>> well - doesn't belong here ;)
>>>>>
>>>>> Sorry -- many things in the same client -- not needed with your latest
>>>>> change anyway.
>>>>>
>>>>>>
>>>>>> +static hashval_t
>>>>>> +passr_hash (const void *p)
>>>>>> +{
>>>>>> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
>>>>>> +  return htab_hash_string (s->unique_name);
>>>>>> +}
>>>>>> +
>>>>>> +/* Hash equal function  */
>>>>>> +
>>>>>> +static int
>>>>>> +passr_eq (const void *p1, const void *p2)
>>>>>> +{
>>>>>> +  const struct pass_registry *const s1 = (const struct pass_registry
>>>>>> *const) p1;
>>>>>> +  const struct pass_registry *const s2 = (const struct pass_registry
>>>>>> *const) p2;
>>>>>> +
>>>>>> +  return !strcmp (s1->unique_name, s2->unique_name);
>>>>>> +}
>>>>>>
>>>>>> you can use htab_hash_string and strcmp directly, no need for these
>>>>>> wrappers.
>>>>>
>>>>> The hashtable entry is not string in this case. It is pass_registry,
>>>>> thus the wrapper.
>>>>>
>>>>>>
>>>>>> +void
>>>>>> +register_pass_name (struct opt_pass *pass, const char *name)
>>>>>>
>>>>>> doesn't seem to need exporting, so don't and make it static.
>>>>>
>>>>> Done.
>>>>>
>>>>>>
>>>>>> +  if (!pass_name_tab)
>>>>>> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>>>>>>
>>>>>> see above, the initial size should be larger - we have 223 passes at the
>>>>>> moment, so use 256.
>>>>>
>>>>> Done.
>>>>>
>>>>>>
>>>>>> +  else
>>>>>> +    return; /* Ignore plugin passes.  */
>>>>>>
>>>>>> ?  You mean they might clash?
>>>>>
>>>>> Yes, name clash.
>>>>>
>>>>>>
>>>>>> +struct opt_pass *
>>>>>> +get_pass_by_name (const char *name)
>>>>>>
>>>>>> doesn't need exporting either.
>>>>>
>>>>> Done.
>>>>>
>>>>>>
>>>>>> +      if (is_enable)
>>>>>> +        error ("unrecognized option -fenable");
>>>>>> +      else
>>>>>> +        error ("unrecognized option -fdisable");
>>>>>>
>>>>>> I think that should be fatal_error - Joseph?
>>>>>>
>>>>>> +      if (is_enable)
>>>>>> +        error ("unknown pass %s specified in -fenable", phase_name);
>>>>>> +      else
>>>>>> +        error ("unknown pass %s specified in -fdisble", phase_name);
>>>>>>
>>>>>> likewise.
>>>>>>
>>>>>> +      if (!enabled_pass_uid_range_tab)
>>>>>> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>>>>>>
>>>>>> instead of using a hashtable here please use a VEC indexed by
>>>>>> the static_pass_number which shoud speed up
>>>>>
>>>>> Ok.  The reason I did not use it is because in most of the cases, the
>>>>> htab will be very small -- it is determined by the number of passes
>>>>> specified in the command line, while the VEC requires allocating const
>>>>> size array. Not an issue as long as by default the array is not
>>>>> allocated.
>>>>>
>>>>>>
>>>>>> +static bool
>>>>>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>>>>>> +                                       tree func, htab_t tab)
>>>>>> +{
>>>>>> +  struct uid_range **slot, *range, key;
>>>>>> +  int cgraph_uid;
>>>>>> +
>>>>>> +  if (!tab)
>>>>>> +    return false;
>>>>>> +
>>>>>> +  key.pass = pass;
>>>>>> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
>>>>>> +  if (!slot || !*slot)
>>>>>> +    return false;
>>>>>>
>>>>>> and simplify the code quite a bit.
>>>>>>
>>>>>> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>>>>>>
>>>>>> note that cgraph uids are recycled, so it might not be the best idea
>>>>>> to use them as discriminator (though I don't have a good idea how
>>>>>> to represent ranges without them).
>>>>>
>>>>> Yes. It is not a big problem as the blind search does not need to know
>>>>> the id->name mapping. Once the id s found, it can be easily discovered
>>>>> via dump.
>>>>>
>>>>>>
>>>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
>>>>>> current_function_decl);
>>>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
>>>>>> current_function_decl);
>>>>>> +
>>>>>>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>>>>>>
>>>>>> so explicitly disabling wins over explicit enabling ;)  I think this
>>>>>> implementation detail and the fact that you always query both
>>>>>> hints at that the interface should be like
>>>>>>
>>>>>> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>>>>>
>>>>> Done.
>>>>>
>>>>>>
>>>>>> instead.
>>>>>>
>>>>>> Thus, please split out the function header dumping changes and rework
>>>>>> the rest of the patch as suggested.
>>>>>
>>>>> Split out. The new patch is attached.
>>>>>
>>>>> Ok after testing is done?
>>>>>
>>>>> Thanks,
>>>>>
>>>>> David
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> Richard.
>>>>>>
>>>>>>> --
>>>>>>> Joseph S. Myers
>>>>>>> joseph@codesourcery.com
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

The new patch is attached. The test (c,c++,fortran, java, ada) is on going.

Thanks,

David

On Tue, May 31, 2011 at 9:06 AM, Xinliang David Li <davidxl@google.com> wrote:
> On Tue, May 31, 2011 at 2:05 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Mon, May 30, 2011 at 10:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> The attached are two simple follow up patches
>>>
>>> 1) the first patch does some refactorization on function header
>>> dumping (with more information printed)
>>>
>>> 2) the second patch cleans up some pass names. Part of the cleanup
>>> results from a previous discussion with Honza -- a) rename
>>> 'tree_profile_ipa' into 'profile', and make 'ipa_profile' and
>>> 'profile' into 'profile_estimate'. The rest of cleanups are needed to
>>> make sure pass names are unique.
>>>
>>> Ok for trunk?
>>
>> +
>> +void
>> +pass_dump_function_header (FILE *dump_file, tree fdecl, struct function *fun)
>>
>> This function needs documentation, the ChangeLog entry misses
>> the tree-pretty-print.h change.
>>
>> +struct function;
>>
>> instead of this please include coretypes.h from tree-pretty-print.h
>> and add the struct function forward declaration there if it isn't already
>> present.
>>
>> You change the output of the header, so please make sure you
>> have bootstrapped and tested with _all_ languages included
>> (and also watch for bugreports for target specific bugs).
>>
>
> Ok.
>
>> +  fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d, uid=%d)",
>> +           dname, aname, fun->funcdef_no, node->uid);
>>
>> I see no point in dumping funcdef_no - it wasn't dumped before in
>> any place.  Instead I miss dumping of the DECL_UID and thus
>> a more specific 'uid', like 'cgraph-uid'.
>
> Ok will add decl_uid. Funcdef_no is very useful for debugging FDO
> coverage mismatch related problems as it is the id used in profile
> hashing.
>
>>
>> +  aname = (IDENTIFIER_POINTER
>> +          (DECL_ASSEMBLER_NAME (fdecl)));
>>
>> using DECL_ASSEMBLER_NAME is bad - it might trigger computation
>> of DECL_ASSEMBLER_NAME which certainly shouldn't be done
>> only for dumping purposes.  Instead do sth like
>>
>>   if (DECL_ASSEMBLER_NAME_SET_P (fdecl))
>>     aname = DECL_ASSEMBLER_NAME (fdecl);
>>   else
>>     aname = '<unset-asm-name>';
>
> Ok.
>
>>
>> and please also watch for cgraph_get_node returning NULL.
>>
>> Also please call the function dump_function_header instead of
>> pass_dump_function_header.
>>
>
> Ok.
>
> Thanks,
>
> David
>> Please re-post with appropriate changes.
>>
>> Thanks,
>> Richard.
>>
>>> Thanks,
>>>
>>> David
>>>
>>> On Fri, May 27, 2011 at 2:58 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> The latest version that only exports 2 functions from passes.c.
>>>>
>>>> Ok with ...
>>>>
>>>> @@ -637,4 +637,8 @@ extern bool first_pass_instance;
>>>>  /* Declare for plugins.  */
>>>>  extern void do_per_function_toporder (void (*) (void *), void *);
>>>>
>>>> +extern void disable_pass (const char *);
>>>> +extern void enable_pass (const char *);
>>>> +struct function;
>>>> +
>>>>
>>>> struct function forward decl removed.
>>>>
>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass, func);
>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass, func);
>>>>
>>>> both functions inlined here and removed.
>>>>
>>>> +#define MAX_PASS_ID 512
>>>>
>>>> this removed and instead a VEC_safe_grow_cleared () or VEC_length ()
>>>> before the accesses.
>>>>
>>>> +-fenable-ipa-@var{pass} @gol
>>>> +-fenable-rtl-@var{pass} @gol
>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>> +-fenable-tree-@var{pass} @gol
>>>> +-fenable-tree-@var{pass}=@var{range-list} @gol
>>>>
>>>> -fenable-@var{kind}-@var{pass}, etc.
>>>>
>>>> +@item -fdisable-@var{ipa|tree|rtl}-@var{pass}
>>>> +@itemx -fenable-@var{ipa|tree|rtl}-@var{pass}
>>>> +@itemx -fdisable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>>> +@itemx -fenable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>>>
>>>> likewise.
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> David
>>>>>
>>>>> On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>>>>>>> <joseph@codesourcery.com> wrote:
>>>>>>>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>>>>>>>
>>>>>>>>> Ping. The link to the message:
>>>>>>>>>
>>>>>>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>>>>>>>
>>>>>>>> I don't consider this an option handling patch.  Patches adding whole new
>>>>>>>> features involving new options should be reviewed by maintainers for the
>>>>>>>> part of the compiler relevant to those features (since there isn't a pass
>>>>>>>> manager maintainer, I guess that means middle-end).
>>>>>>>
>>>>>>> Hmm, I suppose then you reviewed the option handling parts and they
>>>>>>> are ok?  Those globbing options always cause headache to me.
>>>>>>>
>>>>>>> +-fenable-ipa-@var{pass} @gol
>>>>>>> +-fenable-rtl-@var{pass} @gol
>>>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>>>>> +-fenable-tree-@var{pass} @gol
>>>>>>>
>>>>>>> so, no -fenable-tree-@var{pass}=@var{range-list}?
>>>>>>>
>>>>>>
>>>>>> Missed. Will add.
>>>>>>
>>>>>>
>>>>>>> Does the pass name match 1:1 with the dump file name?  In which
>>>>>>> case
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>>
>>>>>>> +Disable ipa 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.
>>>>>>>
>>>>>>> isn't true as passes that are invoked only a single time lack the number
>>>>>>> suffix (yes, I'd really like that to be changed ...)
>>>>>>
>>>>>> Yes, pass with single static instance does not need number suffix.
>>>>>>
>>>>>>>
>>>>>>> Please break likes also in .texi files and stick to 80 columns.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>  Please
>>>>>>> document that these options are debugging options and regular
>>>>>>> options for enabling/disabling passes should be used.  I would suggest
>>>>>>> to group documentation differently and document -fenable-* and
>>>>>>> -fdisable-*, thus,
>>>>>>>
>>>>>>> + -fdisable-@var{kind}-@var{pass}
>>>>>>> + -fenable-@var{kind}-@var{pass}
>>>>>>>
>>>>>>> Even in .texi files please two spaces after a full-stop.
>>>>>>
>>>>>> Done
>>>>>>
>>>>>>>
>>>>>>> +extern void enable_disable_pass (const char *, bool);
>>>>>>>
>>>>>>> I'd rather have both enable_pass and disable_pass ;)
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>>>
>>>>>>> +struct function;
>>>>>>> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>>>>>>>
>>>>>>> that's odd and probably should be split out, the function should
>>>>>>> maybe reside in tree-pretty-print.c.
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>>>
>>>>>>> Index: tree-ssa-loop-ivopts.c
>>>>>>> ===================================================================
>>>>>>> --- tree-ssa-loop-ivopts.c      (revision 173837)
>>>>>>> +++ tree-ssa-loop-ivopts.c      (working copy)
>>>>>>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>>>>>>>
>>>>>>> well - doesn't belong here ;)
>>>>>>
>>>>>> Sorry -- many things in the same client -- not needed with your latest
>>>>>> change anyway.
>>>>>>
>>>>>>>
>>>>>>> +static hashval_t
>>>>>>> +passr_hash (const void *p)
>>>>>>> +{
>>>>>>> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
>>>>>>> +  return htab_hash_string (s->unique_name);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/* Hash equal function  */
>>>>>>> +
>>>>>>> +static int
>>>>>>> +passr_eq (const void *p1, const void *p2)
>>>>>>> +{
>>>>>>> +  const struct pass_registry *const s1 = (const struct pass_registry
>>>>>>> *const) p1;
>>>>>>> +  const struct pass_registry *const s2 = (const struct pass_registry
>>>>>>> *const) p2;
>>>>>>> +
>>>>>>> +  return !strcmp (s1->unique_name, s2->unique_name);
>>>>>>> +}
>>>>>>>
>>>>>>> you can use htab_hash_string and strcmp directly, no need for these
>>>>>>> wrappers.
>>>>>>
>>>>>> The hashtable entry is not string in this case. It is pass_registry,
>>>>>> thus the wrapper.
>>>>>>
>>>>>>>
>>>>>>> +void
>>>>>>> +register_pass_name (struct opt_pass *pass, const char *name)
>>>>>>>
>>>>>>> doesn't seem to need exporting, so don't and make it static.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>
>>>>>>> +  if (!pass_name_tab)
>>>>>>> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>>>>>>>
>>>>>>> see above, the initial size should be larger - we have 223 passes at the
>>>>>>> moment, so use 256.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>
>>>>>>> +  else
>>>>>>> +    return; /* Ignore plugin passes.  */
>>>>>>>
>>>>>>> ?  You mean they might clash?
>>>>>>
>>>>>> Yes, name clash.
>>>>>>
>>>>>>>
>>>>>>> +struct opt_pass *
>>>>>>> +get_pass_by_name (const char *name)
>>>>>>>
>>>>>>> doesn't need exporting either.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>
>>>>>>> +      if (is_enable)
>>>>>>> +        error ("unrecognized option -fenable");
>>>>>>> +      else
>>>>>>> +        error ("unrecognized option -fdisable");
>>>>>>>
>>>>>>> I think that should be fatal_error - Joseph?
>>>>>>>
>>>>>>> +      if (is_enable)
>>>>>>> +        error ("unknown pass %s specified in -fenable", phase_name);
>>>>>>> +      else
>>>>>>> +        error ("unknown pass %s specified in -fdisble", phase_name);
>>>>>>>
>>>>>>> likewise.
>>>>>>>
>>>>>>> +      if (!enabled_pass_uid_range_tab)
>>>>>>> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>>>>>>>
>>>>>>> instead of using a hashtable here please use a VEC indexed by
>>>>>>> the static_pass_number which shoud speed up
>>>>>>
>>>>>> Ok.  The reason I did not use it is because in most of the cases, the
>>>>>> htab will be very small -- it is determined by the number of passes
>>>>>> specified in the command line, while the VEC requires allocating const
>>>>>> size array. Not an issue as long as by default the array is not
>>>>>> allocated.
>>>>>>
>>>>>>>
>>>>>>> +static bool
>>>>>>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>>>>>>> +                                       tree func, htab_t tab)
>>>>>>> +{
>>>>>>> +  struct uid_range **slot, *range, key;
>>>>>>> +  int cgraph_uid;
>>>>>>> +
>>>>>>> +  if (!tab)
>>>>>>> +    return false;
>>>>>>> +
>>>>>>> +  key.pass = pass;
>>>>>>> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
>>>>>>> +  if (!slot || !*slot)
>>>>>>> +    return false;
>>>>>>>
>>>>>>> and simplify the code quite a bit.
>>>>>>>
>>>>>>> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>>>>>>>
>>>>>>> note that cgraph uids are recycled, so it might not be the best idea
>>>>>>> to use them as discriminator (though I don't have a good idea how
>>>>>>> to represent ranges without them).
>>>>>>
>>>>>> Yes. It is not a big problem as the blind search does not need to know
>>>>>> the id->name mapping. Once the id s found, it can be easily discovered
>>>>>> via dump.
>>>>>>
>>>>>>>
>>>>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
>>>>>>> current_function_decl);
>>>>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
>>>>>>> current_function_decl);
>>>>>>> +
>>>>>>>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>>>>>>>
>>>>>>> so explicitly disabling wins over explicit enabling ;)  I think this
>>>>>>> implementation detail and the fact that you always query both
>>>>>>> hints at that the interface should be like
>>>>>>>
>>>>>>> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>
>>>>>>> instead.
>>>>>>>
>>>>>>> Thus, please split out the function header dumping changes and rework
>>>>>>> the rest of the patch as suggested.
>>>>>>
>>>>>> Split out. The new patch is attached.
>>>>>>
>>>>>> Ok after testing is done?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Richard.
>>>>>>>
>>>>>>>> --
>>>>>>>> Joseph S. Myers
>>>>>>>> joseph@codesourcery.com
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Honza, are you ok with the pass name change?

David

On Tue, May 31, 2011 at 2:07 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, May 30, 2011 at 11:44 PM, Xinliang David Li <davidxl@google.com> wrote:
>> This is the complete patch for pass name fixes (with test case changes).
>
> This is ok if Honza thinks the profile pass names make more sense this
> way.
>
> Thanks,
> Richard.
>
>> David
>>
>>
>> On Mon, May 30, 2011 at 1:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>> The attached are two simple follow up patches
>>>
>>> 1) the first patch does some refactorization on function header
>>> dumping (with more information printed)
>>>
>>> 2) the second patch cleans up some pass names. Part of the cleanup
>>> results from a previous discussion with Honza -- a) rename
>>> 'tree_profile_ipa' into 'profile', and make 'ipa_profile' and
>>> 'profile' into 'profile_estimate'. The rest of cleanups are needed to
>>> make sure pass names are unique.
>>>
>>> Ok for trunk?
>>>
>>> Thanks,
>>>
>>> David
>>>
>>> On Fri, May 27, 2011 at 2:58 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> The latest version that only exports 2 functions from passes.c.
>>>>
>>>> Ok with ...
>>>>
>>>> @@ -637,4 +637,8 @@ extern bool first_pass_instance;
>>>>  /* Declare for plugins.  */
>>>>  extern void do_per_function_toporder (void (*) (void *), void *);
>>>>
>>>> +extern void disable_pass (const char *);
>>>> +extern void enable_pass (const char *);
>>>> +struct function;
>>>> +
>>>>
>>>> struct function forward decl removed.
>>>>
>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass, func);
>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass, func);
>>>>
>>>> both functions inlined here and removed.
>>>>
>>>> +#define MAX_PASS_ID 512
>>>>
>>>> this removed and instead a VEC_safe_grow_cleared () or VEC_length ()
>>>> before the accesses.
>>>>
>>>> +-fenable-ipa-@var{pass} @gol
>>>> +-fenable-rtl-@var{pass} @gol
>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>> +-fenable-tree-@var{pass} @gol
>>>> +-fenable-tree-@var{pass}=@var{range-list} @gol
>>>>
>>>> -fenable-@var{kind}-@var{pass}, etc.
>>>>
>>>> +@item -fdisable-@var{ipa|tree|rtl}-@var{pass}
>>>> +@itemx -fenable-@var{ipa|tree|rtl}-@var{pass}
>>>> +@itemx -fdisable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>>> +@itemx -fenable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>>>
>>>> likewise.
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> David
>>>>>
>>>>> On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>>>>>>> <joseph@codesourcery.com> wrote:
>>>>>>>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>>>>>>>
>>>>>>>>> Ping. The link to the message:
>>>>>>>>>
>>>>>>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>>>>>>>
>>>>>>>> I don't consider this an option handling patch.  Patches adding whole new
>>>>>>>> features involving new options should be reviewed by maintainers for the
>>>>>>>> part of the compiler relevant to those features (since there isn't a pass
>>>>>>>> manager maintainer, I guess that means middle-end).
>>>>>>>
>>>>>>> Hmm, I suppose then you reviewed the option handling parts and they
>>>>>>> are ok?  Those globbing options always cause headache to me.
>>>>>>>
>>>>>>> +-fenable-ipa-@var{pass} @gol
>>>>>>> +-fenable-rtl-@var{pass} @gol
>>>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>>>>> +-fenable-tree-@var{pass} @gol
>>>>>>>
>>>>>>> so, no -fenable-tree-@var{pass}=@var{range-list}?
>>>>>>>
>>>>>>
>>>>>> Missed. Will add.
>>>>>>
>>>>>>
>>>>>>> Does the pass name match 1:1 with the dump file name?  In which
>>>>>>> case
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>>>
>>>>>>> +Disable ipa 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.
>>>>>>>
>>>>>>> isn't true as passes that are invoked only a single time lack the number
>>>>>>> suffix (yes, I'd really like that to be changed ...)
>>>>>>
>>>>>> Yes, pass with single static instance does not need number suffix.
>>>>>>
>>>>>>>
>>>>>>> Please break likes also in .texi files and stick to 80 columns.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>  Please
>>>>>>> document that these options are debugging options and regular
>>>>>>> options for enabling/disabling passes should be used.  I would suggest
>>>>>>> to group documentation differently and document -fenable-* and
>>>>>>> -fdisable-*, thus,
>>>>>>>
>>>>>>> + -fdisable-@var{kind}-@var{pass}
>>>>>>> + -fenable-@var{kind}-@var{pass}
>>>>>>>
>>>>>>> Even in .texi files please two spaces after a full-stop.
>>>>>>
>>>>>> Done
>>>>>>
>>>>>>>
>>>>>>> +extern void enable_disable_pass (const char *, bool);
>>>>>>>
>>>>>>> I'd rather have both enable_pass and disable_pass ;)
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>>>
>>>>>>> +struct function;
>>>>>>> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>>>>>>>
>>>>>>> that's odd and probably should be split out, the function should
>>>>>>> maybe reside in tree-pretty-print.c.
>>>>>>
>>>>>> Ok.
>>>>>>
>>>>>>>
>>>>>>> Index: tree-ssa-loop-ivopts.c
>>>>>>> ===================================================================
>>>>>>> --- tree-ssa-loop-ivopts.c      (revision 173837)
>>>>>>> +++ tree-ssa-loop-ivopts.c      (working copy)
>>>>>>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>>>>>>>
>>>>>>> well - doesn't belong here ;)
>>>>>>
>>>>>> Sorry -- many things in the same client -- not needed with your latest
>>>>>> change anyway.
>>>>>>
>>>>>>>
>>>>>>> +static hashval_t
>>>>>>> +passr_hash (const void *p)
>>>>>>> +{
>>>>>>> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
>>>>>>> +  return htab_hash_string (s->unique_name);
>>>>>>> +}
>>>>>>> +
>>>>>>> +/* Hash equal function  */
>>>>>>> +
>>>>>>> +static int
>>>>>>> +passr_eq (const void *p1, const void *p2)
>>>>>>> +{
>>>>>>> +  const struct pass_registry *const s1 = (const struct pass_registry
>>>>>>> *const) p1;
>>>>>>> +  const struct pass_registry *const s2 = (const struct pass_registry
>>>>>>> *const) p2;
>>>>>>> +
>>>>>>> +  return !strcmp (s1->unique_name, s2->unique_name);
>>>>>>> +}
>>>>>>>
>>>>>>> you can use htab_hash_string and strcmp directly, no need for these
>>>>>>> wrappers.
>>>>>>
>>>>>> The hashtable entry is not string in this case. It is pass_registry,
>>>>>> thus the wrapper.
>>>>>>
>>>>>>>
>>>>>>> +void
>>>>>>> +register_pass_name (struct opt_pass *pass, const char *name)
>>>>>>>
>>>>>>> doesn't seem to need exporting, so don't and make it static.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>
>>>>>>> +  if (!pass_name_tab)
>>>>>>> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>>>>>>>
>>>>>>> see above, the initial size should be larger - we have 223 passes at the
>>>>>>> moment, so use 256.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>
>>>>>>> +  else
>>>>>>> +    return; /* Ignore plugin passes.  */
>>>>>>>
>>>>>>> ?  You mean they might clash?
>>>>>>
>>>>>> Yes, name clash.
>>>>>>
>>>>>>>
>>>>>>> +struct opt_pass *
>>>>>>> +get_pass_by_name (const char *name)
>>>>>>>
>>>>>>> doesn't need exporting either.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>
>>>>>>> +      if (is_enable)
>>>>>>> +        error ("unrecognized option -fenable");
>>>>>>> +      else
>>>>>>> +        error ("unrecognized option -fdisable");
>>>>>>>
>>>>>>> I think that should be fatal_error - Joseph?
>>>>>>>
>>>>>>> +      if (is_enable)
>>>>>>> +        error ("unknown pass %s specified in -fenable", phase_name);
>>>>>>> +      else
>>>>>>> +        error ("unknown pass %s specified in -fdisble", phase_name);
>>>>>>>
>>>>>>> likewise.
>>>>>>>
>>>>>>> +      if (!enabled_pass_uid_range_tab)
>>>>>>> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>>>>>>>
>>>>>>> instead of using a hashtable here please use a VEC indexed by
>>>>>>> the static_pass_number which shoud speed up
>>>>>>
>>>>>> Ok.  The reason I did not use it is because in most of the cases, the
>>>>>> htab will be very small -- it is determined by the number of passes
>>>>>> specified in the command line, while the VEC requires allocating const
>>>>>> size array. Not an issue as long as by default the array is not
>>>>>> allocated.
>>>>>>
>>>>>>>
>>>>>>> +static bool
>>>>>>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>>>>>>> +                                       tree func, htab_t tab)
>>>>>>> +{
>>>>>>> +  struct uid_range **slot, *range, key;
>>>>>>> +  int cgraph_uid;
>>>>>>> +
>>>>>>> +  if (!tab)
>>>>>>> +    return false;
>>>>>>> +
>>>>>>> +  key.pass = pass;
>>>>>>> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
>>>>>>> +  if (!slot || !*slot)
>>>>>>> +    return false;
>>>>>>>
>>>>>>> and simplify the code quite a bit.
>>>>>>>
>>>>>>> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>>>>>>>
>>>>>>> note that cgraph uids are recycled, so it might not be the best idea
>>>>>>> to use them as discriminator (though I don't have a good idea how
>>>>>>> to represent ranges without them).
>>>>>>
>>>>>> Yes. It is not a big problem as the blind search does not need to know
>>>>>> the id->name mapping. Once the id s found, it can be easily discovered
>>>>>> via dump.
>>>>>>
>>>>>>>
>>>>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
>>>>>>> current_function_decl);
>>>>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
>>>>>>> current_function_decl);
>>>>>>> +
>>>>>>>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>>>>>>>
>>>>>>> so explicitly disabling wins over explicit enabling ;)  I think this
>>>>>>> implementation detail and the fact that you always query both
>>>>>>> hints at that the interface should be like
>>>>>>>
>>>>>>> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>
>>>>>>> instead.
>>>>>>>
>>>>>>> Thus, please split out the function header dumping changes and rework
>>>>>>> the rest of the patch as suggested.
>>>>>>
>>>>>> Split out. The new patch is attached.
>>>>>>
>>>>>> Ok after testing is done?
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> David
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Richard.
>>>>>>>
>>>>>>>> --
>>>>>>>> Joseph S. Myers
>>>>>>>> joseph@codesourcery.com
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

Please discard the previous one. This is the right one:

David

On Tue, May 31, 2011 at 5:01 PM, Xinliang David Li <davidxl@google.com> wrote:
> The new patch is attached. The test (c,c++,fortran, java, ada) is on going.
>
> Thanks,
>
> David
>
> On Tue, May 31, 2011 at 9:06 AM, Xinliang David Li <davidxl@google.com> wrote:
>> On Tue, May 31, 2011 at 2:05 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Mon, May 30, 2011 at 10:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>> The attached are two simple follow up patches
>>>>
>>>> 1) the first patch does some refactorization on function header
>>>> dumping (with more information printed)
>>>>
>>>> 2) the second patch cleans up some pass names. Part of the cleanup
>>>> results from a previous discussion with Honza -- a) rename
>>>> 'tree_profile_ipa' into 'profile', and make 'ipa_profile' and
>>>> 'profile' into 'profile_estimate'. The rest of cleanups are needed to
>>>> make sure pass names are unique.
>>>>
>>>> Ok for trunk?
>>>
>>> +
>>> +void
>>> +pass_dump_function_header (FILE *dump_file, tree fdecl, struct function *fun)
>>>
>>> This function needs documentation, the ChangeLog entry misses
>>> the tree-pretty-print.h change.
>>>
>>> +struct function;
>>>
>>> instead of this please include coretypes.h from tree-pretty-print.h
>>> and add the struct function forward declaration there if it isn't already
>>> present.
>>>
>>> You change the output of the header, so please make sure you
>>> have bootstrapped and tested with _all_ languages included
>>> (and also watch for bugreports for target specific bugs).
>>>
>>
>> Ok.
>>
>>> +  fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d, uid=%d)",
>>> +           dname, aname, fun->funcdef_no, node->uid);
>>>
>>> I see no point in dumping funcdef_no - it wasn't dumped before in
>>> any place.  Instead I miss dumping of the DECL_UID and thus
>>> a more specific 'uid', like 'cgraph-uid'.
>>
>> Ok will add decl_uid. Funcdef_no is very useful for debugging FDO
>> coverage mismatch related problems as it is the id used in profile
>> hashing.
>>
>>>
>>> +  aname = (IDENTIFIER_POINTER
>>> +          (DECL_ASSEMBLER_NAME (fdecl)));
>>>
>>> using DECL_ASSEMBLER_NAME is bad - it might trigger computation
>>> of DECL_ASSEMBLER_NAME which certainly shouldn't be done
>>> only for dumping purposes.  Instead do sth like
>>>
>>>   if (DECL_ASSEMBLER_NAME_SET_P (fdecl))
>>>     aname = DECL_ASSEMBLER_NAME (fdecl);
>>>   else
>>>     aname = '<unset-asm-name>';
>>
>> Ok.
>>
>>>
>>> and please also watch for cgraph_get_node returning NULL.
>>>
>>> Also please call the function dump_function_header instead of
>>> pass_dump_function_header.
>>>
>>
>> Ok.
>>
>> Thanks,
>>
>> David
>>> Please re-post with appropriate changes.
>>>
>>> Thanks,
>>> Richard.
>>>
>>>> Thanks,
>>>>
>>>> David
>>>>
>>>> On Fri, May 27, 2011 at 2:58 AM, Richard Guenther
>>>> <richard.guenther@gmail.com> wrote:
>>>>> On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>> The latest version that only exports 2 functions from passes.c.
>>>>>
>>>>> Ok with ...
>>>>>
>>>>> @@ -637,4 +637,8 @@ extern bool first_pass_instance;
>>>>>  /* Declare for plugins.  */
>>>>>  extern void do_per_function_toporder (void (*) (void *), void *);
>>>>>
>>>>> +extern void disable_pass (const char *);
>>>>> +extern void enable_pass (const char *);
>>>>> +struct function;
>>>>> +
>>>>>
>>>>> struct function forward decl removed.
>>>>>
>>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass, func);
>>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass, func);
>>>>>
>>>>> both functions inlined here and removed.
>>>>>
>>>>> +#define MAX_PASS_ID 512
>>>>>
>>>>> this removed and instead a VEC_safe_grow_cleared () or VEC_length ()
>>>>> before the accesses.
>>>>>
>>>>> +-fenable-ipa-@var{pass} @gol
>>>>> +-fenable-rtl-@var{pass} @gol
>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>>> +-fenable-tree-@var{pass} @gol
>>>>> +-fenable-tree-@var{pass}=@var{range-list} @gol
>>>>>
>>>>> -fenable-@var{kind}-@var{pass}, etc.
>>>>>
>>>>> +@item -fdisable-@var{ipa|tree|rtl}-@var{pass}
>>>>> +@itemx -fenable-@var{ipa|tree|rtl}-@var{pass}
>>>>> +@itemx -fdisable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>>>> +@itemx -fenable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>>>>
>>>>> likewise.
>>>>>
>>>>> Thanks,
>>>>> Richard.
>>>>>
>>>>>> David
>>>>>>
>>>>>> On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>>>>>>>> <joseph@codesourcery.com> wrote:
>>>>>>>>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>>>>>>>>
>>>>>>>>>> Ping. The link to the message:
>>>>>>>>>>
>>>>>>>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>>>>>>>>
>>>>>>>>> I don't consider this an option handling patch.  Patches adding whole new
>>>>>>>>> features involving new options should be reviewed by maintainers for the
>>>>>>>>> part of the compiler relevant to those features (since there isn't a pass
>>>>>>>>> manager maintainer, I guess that means middle-end).
>>>>>>>>
>>>>>>>> Hmm, I suppose then you reviewed the option handling parts and they
>>>>>>>> are ok?  Those globbing options always cause headache to me.
>>>>>>>>
>>>>>>>> +-fenable-ipa-@var{pass} @gol
>>>>>>>> +-fenable-rtl-@var{pass} @gol
>>>>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>>>>>> +-fenable-tree-@var{pass} @gol
>>>>>>>>
>>>>>>>> so, no -fenable-tree-@var{pass}=@var{range-list}?
>>>>>>>>
>>>>>>>
>>>>>>> Missed. Will add.
>>>>>>>
>>>>>>>
>>>>>>>> Does the pass name match 1:1 with the dump file name?  In which
>>>>>>>> case
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>>>
>>>>>>>> +Disable ipa 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.
>>>>>>>>
>>>>>>>> isn't true as passes that are invoked only a single time lack the number
>>>>>>>> suffix (yes, I'd really like that to be changed ...)
>>>>>>>
>>>>>>> Yes, pass with single static instance does not need number suffix.
>>>>>>>
>>>>>>>>
>>>>>>>> Please break likes also in .texi files and stick to 80 columns.
>>>>>>>
>>>>>>> Done.
>>>>>>>
>>>>>>>>  Please
>>>>>>>> document that these options are debugging options and regular
>>>>>>>> options for enabling/disabling passes should be used.  I would suggest
>>>>>>>> to group documentation differently and document -fenable-* and
>>>>>>>> -fdisable-*, thus,
>>>>>>>>
>>>>>>>> + -fdisable-@var{kind}-@var{pass}
>>>>>>>> + -fenable-@var{kind}-@var{pass}
>>>>>>>>
>>>>>>>> Even in .texi files please two spaces after a full-stop.
>>>>>>>
>>>>>>> Done
>>>>>>>
>>>>>>>>
>>>>>>>> +extern void enable_disable_pass (const char *, bool);
>>>>>>>>
>>>>>>>> I'd rather have both enable_pass and disable_pass ;)
>>>>>>>
>>>>>>> Ok.
>>>>>>>
>>>>>>>>
>>>>>>>> +struct function;
>>>>>>>> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>>>>>>>>
>>>>>>>> that's odd and probably should be split out, the function should
>>>>>>>> maybe reside in tree-pretty-print.c.
>>>>>>>
>>>>>>> Ok.
>>>>>>>
>>>>>>>>
>>>>>>>> Index: tree-ssa-loop-ivopts.c
>>>>>>>> ===================================================================
>>>>>>>> --- tree-ssa-loop-ivopts.c      (revision 173837)
>>>>>>>> +++ tree-ssa-loop-ivopts.c      (working copy)
>>>>>>>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>>>>>>>>
>>>>>>>> well - doesn't belong here ;)
>>>>>>>
>>>>>>> Sorry -- many things in the same client -- not needed with your latest
>>>>>>> change anyway.
>>>>>>>
>>>>>>>>
>>>>>>>> +static hashval_t
>>>>>>>> +passr_hash (const void *p)
>>>>>>>> +{
>>>>>>>> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
>>>>>>>> +  return htab_hash_string (s->unique_name);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +/* Hash equal function  */
>>>>>>>> +
>>>>>>>> +static int
>>>>>>>> +passr_eq (const void *p1, const void *p2)
>>>>>>>> +{
>>>>>>>> +  const struct pass_registry *const s1 = (const struct pass_registry
>>>>>>>> *const) p1;
>>>>>>>> +  const struct pass_registry *const s2 = (const struct pass_registry
>>>>>>>> *const) p2;
>>>>>>>> +
>>>>>>>> +  return !strcmp (s1->unique_name, s2->unique_name);
>>>>>>>> +}
>>>>>>>>
>>>>>>>> you can use htab_hash_string and strcmp directly, no need for these
>>>>>>>> wrappers.
>>>>>>>
>>>>>>> The hashtable entry is not string in this case. It is pass_registry,
>>>>>>> thus the wrapper.
>>>>>>>
>>>>>>>>
>>>>>>>> +void
>>>>>>>> +register_pass_name (struct opt_pass *pass, const char *name)
>>>>>>>>
>>>>>>>> doesn't seem to need exporting, so don't and make it static.
>>>>>>>
>>>>>>> Done.
>>>>>>>
>>>>>>>>
>>>>>>>> +  if (!pass_name_tab)
>>>>>>>> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>>>>>>>>
>>>>>>>> see above, the initial size should be larger - we have 223 passes at the
>>>>>>>> moment, so use 256.
>>>>>>>
>>>>>>> Done.
>>>>>>>
>>>>>>>>
>>>>>>>> +  else
>>>>>>>> +    return; /* Ignore plugin passes.  */
>>>>>>>>
>>>>>>>> ?  You mean they might clash?
>>>>>>>
>>>>>>> Yes, name clash.
>>>>>>>
>>>>>>>>
>>>>>>>> +struct opt_pass *
>>>>>>>> +get_pass_by_name (const char *name)
>>>>>>>>
>>>>>>>> doesn't need exporting either.
>>>>>>>
>>>>>>> Done.
>>>>>>>
>>>>>>>>
>>>>>>>> +      if (is_enable)
>>>>>>>> +        error ("unrecognized option -fenable");
>>>>>>>> +      else
>>>>>>>> +        error ("unrecognized option -fdisable");
>>>>>>>>
>>>>>>>> I think that should be fatal_error - Joseph?
>>>>>>>>
>>>>>>>> +      if (is_enable)
>>>>>>>> +        error ("unknown pass %s specified in -fenable", phase_name);
>>>>>>>> +      else
>>>>>>>> +        error ("unknown pass %s specified in -fdisble", phase_name);
>>>>>>>>
>>>>>>>> likewise.
>>>>>>>>
>>>>>>>> +      if (!enabled_pass_uid_range_tab)
>>>>>>>> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>>>>>>>>
>>>>>>>> instead of using a hashtable here please use a VEC indexed by
>>>>>>>> the static_pass_number which shoud speed up
>>>>>>>
>>>>>>> Ok.  The reason I did not use it is because in most of the cases, the
>>>>>>> htab will be very small -- it is determined by the number of passes
>>>>>>> specified in the command line, while the VEC requires allocating const
>>>>>>> size array. Not an issue as long as by default the array is not
>>>>>>> allocated.
>>>>>>>
>>>>>>>>
>>>>>>>> +static bool
>>>>>>>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>>>>>>>> +                                       tree func, htab_t tab)
>>>>>>>> +{
>>>>>>>> +  struct uid_range **slot, *range, key;
>>>>>>>> +  int cgraph_uid;
>>>>>>>> +
>>>>>>>> +  if (!tab)
>>>>>>>> +    return false;
>>>>>>>> +
>>>>>>>> +  key.pass = pass;
>>>>>>>> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
>>>>>>>> +  if (!slot || !*slot)
>>>>>>>> +    return false;
>>>>>>>>
>>>>>>>> and simplify the code quite a bit.
>>>>>>>>
>>>>>>>> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>>>>>>>>
>>>>>>>> note that cgraph uids are recycled, so it might not be the best idea
>>>>>>>> to use them as discriminator (though I don't have a good idea how
>>>>>>>> to represent ranges without them).
>>>>>>>
>>>>>>> Yes. It is not a big problem as the blind search does not need to know
>>>>>>> the id->name mapping. Once the id s found, it can be easily discovered
>>>>>>> via dump.
>>>>>>>
>>>>>>>>
>>>>>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
>>>>>>>> current_function_decl);
>>>>>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
>>>>>>>> current_function_decl);
>>>>>>>> +
>>>>>>>>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>>>>>>>>
>>>>>>>> so explicitly disabling wins over explicit enabling ;)  I think this
>>>>>>>> implementation detail and the fact that you always query both
>>>>>>>> hints at that the interface should be like
>>>>>>>>
>>>>>>>> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>>>>>>>
>>>>>>> Done.
>>>>>>>
>>>>>>>>
>>>>>>>> instead.
>>>>>>>>
>>>>>>>> Thus, please split out the function header dumping changes and rework
>>>>>>>> the rest of the patch as suggested.
>>>>>>>
>>>>>>> Split out. The new patch is attached.
>>>>>>>
>>>>>>> Ok after testing is done?
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Richard.
>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Joseph S. Myers
>>>>>>>>> joseph@codesourcery.com
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

> Please discard the previous one. This is the right one:
> 
> David
> Index: tree-pretty-print.c
> ===================================================================
> --- tree-pretty-print.c	(revision 174424)
> +++ tree-pretty-print.c	(working copy)
> @@ -3013,3 +3013,36 @@ pp_base_tree_identifier (pretty_printer 
>      pp_append_text (pp, IDENTIFIER_POINTER (id),
>  		    IDENTIFIER_POINTER (id) + IDENTIFIER_LENGTH (id));
>  }
> +
> +/* A helper function that is used to dump function information before the
> +   function dump.  */
> +
> +void
> +dump_function_header (FILE *dump_file, tree fdecl, struct function *fun)

You can get to FUN via DECL_STRUCT_FUNCTION (fndecl) that would save you a parameter...
> Index: tree-pretty-print.h
> ===================================================================
> --- tree-pretty-print.h	(revision 174422)
> +++ tree-pretty-print.h	(working copy)
> @@ -24,6 +24,7 @@ along with GCC; see the file COPYING3.  
>  #define GCC_TREE_PRETTY_PRINT_H
>  
>  #include "pretty-print.h"
> +#include "coretypes.h"

.. and need for this
> Index: coretypes.h
> ===================================================================
> --- coretypes.h	(revision 174422)
> +++ coretypes.h	(working copy)
> @@ -75,6 +75,7 @@ typedef struct diagnostic_context diagno
>  struct gimple_seq_d;
>  typedef struct gimple_seq_d *gimple_seq;
>  typedef const struct gimple_seq_d *const_gimple_seq;
> +struct function;

... and this.

Otherwise the patch seems OK.  You need to update Makefile.in for new dependencies.

Honza

On Wed, Jun 1, 2011 at 2:03 AM, Xinliang David Li <davidxl@google.com> wrote:
> Please discard the previous one. This is the right one:

See also Honzas comments (on the wrong patch presumably ;)).

+  if (node)
+    fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d,
decl_uid = %d, cgraph_uid=%d)",
+             dname, aname, fun->funcdef_no, DECL_UID(fdecl), node->uid);
+  else
+    fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d, decl_uid = %d)",
+             dname, aname, fun->funcdef_no, DECL_UID(fdecl));
+
+  fprintf (dump_file, "%s\n\n",
+           node->frequency == NODE_FREQUENCY_HOT

you also need to check for node == NULL here.

Ok with this (and honzas suggested change for not passing struct
function *).

Thanks,
Richard.

> David
>
> On Tue, May 31, 2011 at 5:01 PM, Xinliang David Li <davidxl@google.com> wrote:
>> The new patch is attached. The test (c,c++,fortran, java, ada) is on going.
>>
>> Thanks,
>>
>> David
>>
>> On Tue, May 31, 2011 at 9:06 AM, Xinliang David Li <davidxl@google.com> wrote:
>>> On Tue, May 31, 2011 at 2:05 AM, Richard Guenther
>>> <richard.guenther@gmail.com> wrote:
>>>> On Mon, May 30, 2011 at 10:16 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>> The attached are two simple follow up patches
>>>>>
>>>>> 1) the first patch does some refactorization on function header
>>>>> dumping (with more information printed)
>>>>>
>>>>> 2) the second patch cleans up some pass names. Part of the cleanup
>>>>> results from a previous discussion with Honza -- a) rename
>>>>> 'tree_profile_ipa' into 'profile', and make 'ipa_profile' and
>>>>> 'profile' into 'profile_estimate'. The rest of cleanups are needed to
>>>>> make sure pass names are unique.
>>>>>
>>>>> Ok for trunk?
>>>>
>>>> +
>>>> +void
>>>> +pass_dump_function_header (FILE *dump_file, tree fdecl, struct function *fun)
>>>>
>>>> This function needs documentation, the ChangeLog entry misses
>>>> the tree-pretty-print.h change.
>>>>
>>>> +struct function;
>>>>
>>>> instead of this please include coretypes.h from tree-pretty-print.h
>>>> and add the struct function forward declaration there if it isn't already
>>>> present.
>>>>
>>>> You change the output of the header, so please make sure you
>>>> have bootstrapped and tested with _all_ languages included
>>>> (and also watch for bugreports for target specific bugs).
>>>>
>>>
>>> Ok.
>>>
>>>> +  fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d, uid=%d)",
>>>> +           dname, aname, fun->funcdef_no, node->uid);
>>>>
>>>> I see no point in dumping funcdef_no - it wasn't dumped before in
>>>> any place.  Instead I miss dumping of the DECL_UID and thus
>>>> a more specific 'uid', like 'cgraph-uid'.
>>>
>>> Ok will add decl_uid. Funcdef_no is very useful for debugging FDO
>>> coverage mismatch related problems as it is the id used in profile
>>> hashing.
>>>
>>>>
>>>> +  aname = (IDENTIFIER_POINTER
>>>> +          (DECL_ASSEMBLER_NAME (fdecl)));
>>>>
>>>> using DECL_ASSEMBLER_NAME is bad - it might trigger computation
>>>> of DECL_ASSEMBLER_NAME which certainly shouldn't be done
>>>> only for dumping purposes.  Instead do sth like
>>>>
>>>>   if (DECL_ASSEMBLER_NAME_SET_P (fdecl))
>>>>     aname = DECL_ASSEMBLER_NAME (fdecl);
>>>>   else
>>>>     aname = '<unset-asm-name>';
>>>
>>> Ok.
>>>
>>>>
>>>> and please also watch for cgraph_get_node returning NULL.
>>>>
>>>> Also please call the function dump_function_header instead of
>>>> pass_dump_function_header.
>>>>
>>>
>>> Ok.
>>>
>>> Thanks,
>>>
>>> David
>>>> Please re-post with appropriate changes.
>>>>
>>>> Thanks,
>>>> Richard.
>>>>
>>>>> Thanks,
>>>>>
>>>>> David
>>>>>
>>>>> On Fri, May 27, 2011 at 2:58 AM, Richard Guenther
>>>>> <richard.guenther@gmail.com> wrote:
>>>>>> On Fri, May 27, 2011 at 12:02 AM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>> The latest version that only exports 2 functions from passes.c.
>>>>>>
>>>>>> Ok with ...
>>>>>>
>>>>>> @@ -637,4 +637,8 @@ extern bool first_pass_instance;
>>>>>>  /* Declare for plugins.  */
>>>>>>  extern void do_per_function_toporder (void (*) (void *), void *);
>>>>>>
>>>>>> +extern void disable_pass (const char *);
>>>>>> +extern void enable_pass (const char *);
>>>>>> +struct function;
>>>>>> +
>>>>>>
>>>>>> struct function forward decl removed.
>>>>>>
>>>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass, func);
>>>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass, func);
>>>>>>
>>>>>> both functions inlined here and removed.
>>>>>>
>>>>>> +#define MAX_PASS_ID 512
>>>>>>
>>>>>> this removed and instead a VEC_safe_grow_cleared () or VEC_length ()
>>>>>> before the accesses.
>>>>>>
>>>>>> +-fenable-ipa-@var{pass} @gol
>>>>>> +-fenable-rtl-@var{pass} @gol
>>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>>>> +-fenable-tree-@var{pass} @gol
>>>>>> +-fenable-tree-@var{pass}=@var{range-list} @gol
>>>>>>
>>>>>> -fenable-@var{kind}-@var{pass}, etc.
>>>>>>
>>>>>> +@item -fdisable-@var{ipa|tree|rtl}-@var{pass}
>>>>>> +@itemx -fenable-@var{ipa|tree|rtl}-@var{pass}
>>>>>> +@itemx -fdisable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>>>>> +@itemx -fenable-@var{tree|rtl}-@var{pass}=@var{range-list}
>>>>>>
>>>>>> likewise.
>>>>>>
>>>>>> Thanks,
>>>>>> Richard.
>>>>>>
>>>>>>> David
>>>>>>>
>>>>>>> On Thu, May 26, 2011 at 2:04 PM, Xinliang David Li <davidxl@google.com> wrote:
>>>>>>>> On Thu, May 26, 2011 at 2:04 AM, Richard Guenther
>>>>>>>> <richard.guenther@gmail.com> wrote:
>>>>>>>>> On Wed, May 25, 2011 at 6:53 PM, Joseph S. Myers
>>>>>>>>> <joseph@codesourcery.com> wrote:
>>>>>>>>>> On Wed, 25 May 2011, Xinliang David Li wrote:
>>>>>>>>>>
>>>>>>>>>>> Ping. The link to the message:
>>>>>>>>>>>
>>>>>>>>>>> http://gcc.gnu.org/ml/gcc-patches/2011-05/msg01303.html
>>>>>>>>>>
>>>>>>>>>> I don't consider this an option handling patch.  Patches adding whole new
>>>>>>>>>> features involving new options should be reviewed by maintainers for the
>>>>>>>>>> part of the compiler relevant to those features (since there isn't a pass
>>>>>>>>>> manager maintainer, I guess that means middle-end).
>>>>>>>>>
>>>>>>>>> Hmm, I suppose then you reviewed the option handling parts and they
>>>>>>>>> are ok?  Those globbing options always cause headache to me.
>>>>>>>>>
>>>>>>>>> +-fenable-ipa-@var{pass} @gol
>>>>>>>>> +-fenable-rtl-@var{pass} @gol
>>>>>>>>> +-fenable-rtl-@var{pass}=@var{range-list} @gol
>>>>>>>>> +-fenable-tree-@var{pass} @gol
>>>>>>>>>
>>>>>>>>> so, no -fenable-tree-@var{pass}=@var{range-list}?
>>>>>>>>>
>>>>>>>>
>>>>>>>> Missed. Will add.
>>>>>>>>
>>>>>>>>
>>>>>>>>> Does the pass name match 1:1 with the dump file name?  In which
>>>>>>>>> case
>>>>>>>>
>>>>>>>> Yes.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +Disable ipa 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.
>>>>>>>>>
>>>>>>>>> isn't true as passes that are invoked only a single time lack the number
>>>>>>>>> suffix (yes, I'd really like that to be changed ...)
>>>>>>>>
>>>>>>>> Yes, pass with single static instance does not need number suffix.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Please break likes also in .texi files and stick to 80 columns.
>>>>>>>>
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>>  Please
>>>>>>>>> document that these options are debugging options and regular
>>>>>>>>> options for enabling/disabling passes should be used.  I would suggest
>>>>>>>>> to group documentation differently and document -fenable-* and
>>>>>>>>> -fdisable-*, thus,
>>>>>>>>>
>>>>>>>>> + -fdisable-@var{kind}-@var{pass}
>>>>>>>>> + -fenable-@var{kind}-@var{pass}
>>>>>>>>>
>>>>>>>>> Even in .texi files please two spaces after a full-stop.
>>>>>>>>
>>>>>>>> Done
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +extern void enable_disable_pass (const char *, bool);
>>>>>>>>>
>>>>>>>>> I'd rather have both enable_pass and disable_pass ;)
>>>>>>>>
>>>>>>>> Ok.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +struct function;
>>>>>>>>> +extern void pass_dump_function_header (FILE *, tree, struct function *);
>>>>>>>>>
>>>>>>>>> that's odd and probably should be split out, the function should
>>>>>>>>> maybe reside in tree-pretty-print.c.
>>>>>>>>
>>>>>>>> Ok.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Index: tree-ssa-loop-ivopts.c
>>>>>>>>> ===================================================================
>>>>>>>>> --- tree-ssa-loop-ivopts.c      (revision 173837)
>>>>>>>>> +++ tree-ssa-loop-ivopts.c      (working copy)
>>>>>>>>> @@ -3968,7 +3968,7 @@ get_computation_cost_at (struct ivopts_d
>>>>>>>>>
>>>>>>>>> well - doesn't belong here ;)
>>>>>>>>
>>>>>>>> Sorry -- many things in the same client -- not needed with your latest
>>>>>>>> change anyway.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +static hashval_t
>>>>>>>>> +passr_hash (const void *p)
>>>>>>>>> +{
>>>>>>>>> +  const struct pass_registry *const s = (const struct pass_registry *const) p;
>>>>>>>>> +  return htab_hash_string (s->unique_name);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +/* Hash equal function  */
>>>>>>>>> +
>>>>>>>>> +static int
>>>>>>>>> +passr_eq (const void *p1, const void *p2)
>>>>>>>>> +{
>>>>>>>>> +  const struct pass_registry *const s1 = (const struct pass_registry
>>>>>>>>> *const) p1;
>>>>>>>>> +  const struct pass_registry *const s2 = (const struct pass_registry
>>>>>>>>> *const) p2;
>>>>>>>>> +
>>>>>>>>> +  return !strcmp (s1->unique_name, s2->unique_name);
>>>>>>>>> +}
>>>>>>>>>
>>>>>>>>> you can use htab_hash_string and strcmp directly, no need for these
>>>>>>>>> wrappers.
>>>>>>>>
>>>>>>>> The hashtable entry is not string in this case. It is pass_registry,
>>>>>>>> thus the wrapper.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +void
>>>>>>>>> +register_pass_name (struct opt_pass *pass, const char *name)
>>>>>>>>>
>>>>>>>>> doesn't seem to need exporting, so don't and make it static.
>>>>>>>>
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +  if (!pass_name_tab)
>>>>>>>>> +    pass_name_tab = htab_create (10, passr_hash, passr_eq, NULL);
>>>>>>>>>
>>>>>>>>> see above, the initial size should be larger - we have 223 passes at the
>>>>>>>>> moment, so use 256.
>>>>>>>>
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +  else
>>>>>>>>> +    return; /* Ignore plugin passes.  */
>>>>>>>>>
>>>>>>>>> ?  You mean they might clash?
>>>>>>>>
>>>>>>>> Yes, name clash.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +struct opt_pass *
>>>>>>>>> +get_pass_by_name (const char *name)
>>>>>>>>>
>>>>>>>>> doesn't need exporting either.
>>>>>>>>
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +      if (is_enable)
>>>>>>>>> +        error ("unrecognized option -fenable");
>>>>>>>>> +      else
>>>>>>>>> +        error ("unrecognized option -fdisable");
>>>>>>>>>
>>>>>>>>> I think that should be fatal_error - Joseph?
>>>>>>>>>
>>>>>>>>> +      if (is_enable)
>>>>>>>>> +        error ("unknown pass %s specified in -fenable", phase_name);
>>>>>>>>> +      else
>>>>>>>>> +        error ("unknown pass %s specified in -fdisble", phase_name);
>>>>>>>>>
>>>>>>>>> likewise.
>>>>>>>>>
>>>>>>>>> +      if (!enabled_pass_uid_range_tab)
>>>>>>>>> +       enabled_pass_uid_range_tab = htab_create (10, pass_hash, pass_eq, NULL);
>>>>>>>>>
>>>>>>>>> instead of using a hashtable here please use a VEC indexed by
>>>>>>>>> the static_pass_number which shoud speed up
>>>>>>>>
>>>>>>>> Ok.  The reason I did not use it is because in most of the cases, the
>>>>>>>> htab will be very small -- it is determined by the number of passes
>>>>>>>> specified in the command line, while the VEC requires allocating const
>>>>>>>> size array. Not an issue as long as by default the array is not
>>>>>>>> allocated.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +static bool
>>>>>>>>> +is_pass_explicitly_enabled_or_disabled (struct opt_pass *pass,
>>>>>>>>> +                                       tree func, htab_t tab)
>>>>>>>>> +{
>>>>>>>>> +  struct uid_range **slot, *range, key;
>>>>>>>>> +  int cgraph_uid;
>>>>>>>>> +
>>>>>>>>> +  if (!tab)
>>>>>>>>> +    return false;
>>>>>>>>> +
>>>>>>>>> +  key.pass = pass;
>>>>>>>>> +  slot = (struct uid_range **) htab_find_slot (tab, &key, NO_INSERT);
>>>>>>>>> +  if (!slot || !*slot)
>>>>>>>>> +    return false;
>>>>>>>>>
>>>>>>>>> and simplify the code quite a bit.
>>>>>>>>>
>>>>>>>>> +  cgraph_uid = func ? cgraph_get_node (func)->uid : 0;
>>>>>>>>>
>>>>>>>>> note that cgraph uids are recycled, so it might not be the best idea
>>>>>>>>> to use them as discriminator (though I don't have a good idea how
>>>>>>>>> to represent ranges without them).
>>>>>>>>
>>>>>>>> Yes. It is not a big problem as the blind search does not need to know
>>>>>>>> the id->name mapping. Once the id s found, it can be easily discovered
>>>>>>>> via dump.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> +  explicitly_enabled = is_pass_explicitly_enabled (pass,
>>>>>>>>> current_function_decl);
>>>>>>>>> +  explicitly_disabled = is_pass_explicitly_disabled (pass,
>>>>>>>>> current_function_decl);
>>>>>>>>> +
>>>>>>>>>   current_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 = !explicitly_disabled && (gate_status || explicitly_enabled);
>>>>>>>>>
>>>>>>>>> so explicitly disabling wins over explicit enabling ;)  I think this
>>>>>>>>> implementation detail and the fact that you always query both
>>>>>>>>> hints at that the interface should be like
>>>>>>>>>
>>>>>>>>> gate_status = override_gate_status (pass, current_function_decl, gate_status);
>>>>>>>>
>>>>>>>> Done.
>>>>>>>>
>>>>>>>>>
>>>>>>>>> instead.
>>>>>>>>>
>>>>>>>>> Thus, please split out the function header dumping changes and rework
>>>>>>>>> the rest of the patch as suggested.
>>>>>>>>
>>>>>>>> Split out. The new patch is attached.
>>>>>>>>
>>>>>>>> Ok after testing is done?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Richard.
>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Joseph S. Myers
>>>>>>>>>> joseph@codesourcery.com
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>

> On Wed, Jun 1, 2011 at 2:03 AM, Xinliang David Li <davidxl@google.com> wrote:
> > Please discard the previous one. This is the right one:
> 
> See also Honzas comments (on the wrong patch presumably ;)).
> 
> +  if (node)
> +    fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d,
> decl_uid = %d, cgraph_uid=%d)",
> +             dname, aname, fun->funcdef_no, DECL_UID(fdecl), node->uid);
> +  else
> +    fprintf (dump_file, "\n;; Function %s (%s, funcdef_no=%d, decl_uid = %d)",
> +             dname, aname, fun->funcdef_no, DECL_UID(fdecl));
> +
> +  fprintf (dump_file, "%s\n\n",
> +           node->frequency == NODE_FREQUENCY_HOT
> 
> you also need to check for node == NULL here.
> 
> Ok with this (and honzas suggested change for not passing struct
> function *).
Also I still think the function funcdef_no is quite redundant UID. For debugging profiling
I would go for cgraph_uid instead.  It has also the advantage of being available at WPA stage.
But I have nothing against printing it here.

Honza

On Mon, May 30, 2011 at 2:44 PM, Xinliang David Li <davidxl@google.com> wrote:
> This is the complete patch for pass name fixes (with test case changes).
>
> David
>
>

I think your change caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49261


H.J.