Re: [tree-profiling-branch PATCH] IPA constant propagation

[Steven Bosscher <stevenb at suse dot de>]

On Friday 30 July 2004 10:33, Razya Ladelsky wrote:
> Hello,
>
> This patch implements IPA CP (InterProcedural constant propagation).
> The flag fipa-cp enables the IPA CP optimization.
>  At this stage IPA CP needs to analyze all of the application's files, as
> cloning of methods is not yet supported. This functionality will be
> supported
> shortly.
> Therefore, for the moment the option -fipa-no-cloning  should be specified
>
> in order to perform IPA CP.
>
> To apply the optimization on an application containing
> file1.c, file2.c, file3.c, we should use the command:
>
> gcc -fipa-cp -fipa-no-cloning -combine file1.c file2.c file3.c
>
> combine option is currently supported only in mainline
> (it will be supported in the tree-profiling-branch soon)
> but the code was tested also on mainline.
>
> Bootstrap and regression tests successfully passed on x86.
>
> Comments are welcome.
>
> Thanks,
> Razya
>
> Changelog Entry:
>
> 2004-07-30   Razya Ladelsky  <razya@il.ibm.com>
>
>         * ipa_prop.c: New File: IPA constant propagation.
>         * ipa_prop.h: New File: Same.
>         * Makefile.in (ipa_prop.c, ipa_prop.h): Add new files.
>         * common.opt (fipa-cp, fipa-no-cloning): New flags for IPA constant
>         propagation.
>         * opts.c (fipa-cp, fipa-no-cloning): Same.
>         * flags.h (fipa-cp, fipa-no-cloning): Same.
>         * toplev.c (fipa-cp, fipa-no-cloning): Same.
>         * cgraph.h: Include ipa_prop.h.
>         (cgraph_node, cgraph_edge): Added new fields for IPA constant
>	  propagation.
>         * cgraph.c (cgraph-create_node, cgraph_create_edge):
>         Initialization for IPA constant propagation.
>         * cgraphunit.c (cgraph_optimize, record_call_1): Support for IPA
>         constant propagation.


Hi Razya,

First of all, I think it's great you're working on this.  But, I do
have a few comments on things you probably should address before you
can put this pass on the branch.

First of all, you say you've tested it but you don't tell how you've
exercised the new code.  You don't have it enabled during bootstrap
so I wonder how you evaluated the effects of IPCP, where it helps in
the code we generate.

The following hunk makes no sense, you know that decl is nonzero here,
or you would have already segfaulted:

diff -c -3 -p -r1.1.4.35.2.11 cgraphunit.c
*** cgraphunit.c	4 Jun 2004 20:44:50 -0000	1.1.4.35.2.11
--- cgraphunit.c	30 Jul 2004 05:53:44 -0000
*************** record_call_1 (tree *tp, int *walk_subtr
*** 410,415 ****
--- 410,417 ----
  	  tree decl = TREE_OPERAND (*tp, 0);
  	  if (TREE_CODE (decl) == FUNCTION_DECL)
  	    cgraph_mark_needed_node (cgraph_node (decl));
+           if (flag_ipa_cp && decl == NULL_TREE)
+             flag_ipa_cp = 0;
  	}
        break;


You add all the data for IPCP from the cgraph_nodes and cgraph_edges:

*************** struct cgraph_node GTY((chain_next ("%h.
*** 109,114 ****
--- 109,122 ----
    bool analyzed;
    /* Set when function is scheduled to be assembled.  */
    bool output;
+   /* Fields needed by IPA Constant Propagation algorithm.  */
+   /* Array of cvals.  */
+   struct ipcp_formal* GTY ((skip (""))) ipcp_cval;
+   int ipcp_arg_num;
+   /* Mapping each parameter to its PARM_DECL tree.  */
+   struct ipcp_tree_map* GTY ((skip (""))) ipcp_param_tree;
+   /* Indicating which parameter is modified in its method.  */
+   struct ipcp_modify* GTY ((skip (""))) ipcp_mod;
  };

  struct cgraph_edge GTY((chain_next ("%h.next_caller")))
*************** struct cgraph_edge GTY((chain_next ("%h.
*** 122,127 ****
--- 130,139 ----
    /* When NULL, inline this call.  When non-NULL, points to the explanation
       why function was not inlined.  */
    const char *inline_failed;
+   /* Fields needed by IPA Constant Propagation algorithm.  */
+   /* Jump functions' array */
+   struct ipcp_jump_func* GTY ((skip (""))) ipcp_param_map;
+   int ipcp_param_num;
  };

  /* The cgraph_varpool data structure.

When we have more IPA algorithms implemented and everyone adds their
pass specific fields to these cgraph_* structs, it's not going to be
pretty.  Why can't you put those fields in a separate struct and hang
them from cgraph_{node,edge}->aux when IPCP is running?


Nit: As I've told you before, you might want to push ipcp_driver() down
a few lines, to below "Performing interprocedural optimizations" ;-)

*************** cgraph_optimize (void)
*** 1728,1733 ****
--- 1732,1739 ----
  #endif
    if (!flag_unit_at_a_time)
      return;
+   if (flag_ipa_cp && flag_ipa_no_cloning)
+     ipcp_driver ();
    timevar_push (TV_CGRAPHOPT);
    if (!quiet_flag)
      fprintf (stderr, "Performing intraprocedural optimizations\n");



The file ipa_prop.h seems redundant.  You can put those enums and
structs in ipa_prop.c and the ipcp_driver in tree-flow.h or cgraph.h.
You now make a lot of files depend in ipa_prop.h, please don't do that.
Make dependencies are a big mess as it is, and you're making it worse.


I haven't looked much at ipa_prop.c, the code may be OK but there's a
lot to be complained about comments.
1) there is no copyright header
2) there is no overview/introduction to the algorithm that
   the file implements
3) there are almost no comments before and inside functions.


There really should also be documentation in doc/invoke.texi and in
doc/passes.texi, but that can be addressed at a later point (before
this goes on mainline).
For the tree-profiling branch, I'd very much appreciate if you could
fix the issues I mentioned, especially the comment issue is important
because we might need to seriously overhaul your code when we start
working on a more complete IPA infrastructure on the branch along the
lines I previously mailed you about.

Hope this helps,

Gr.
Steven