This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH]: Interprocedural detection of readonly and non-addressable static variables


On Wed, Jun 08, 2005 at 04:35:59PM -0400, Kenneth Zadeck wrote:
> Here is one big unified diff rather than the 6 patches. 
> 
Sorry it took so long to review.  The patch needs a few changes,
many of them are mechanical.  But there are a couple that I'd
like to get addressed before accepting the patch:

1. There is a bit of code duplication in the IPA analyses.  It
   seems that we could share good chunks in ipa-utils.c or
   something.

2. The code predicated with AGRESSIVE_ALIASING has to disappear if
   we can't still enable it.

3. The register promotion code has to be re-implemented to use the
   operand infrastructure.  There is no reason why it should be
   manipulating trees that way.  It also doesn't need to manually
   walk through expressions when the operand scanner can give it
   all the information it needs.

I'm willing to let you work on #1 as a follow-up patch.  The rest
of the changes are mechanical and pre-approved.

> +   Say there exists (in c) 
> +
> +   struct X {
> +     struct Y y1;
> +     struct Z z2;
> +   } x1, *px1,  *px2;
> +
> +   struct Y y2, *py;
> +   struct Z z2, *pz;
> +
> +
> +   py = &px1.y1;
> +   px2 = &x1;
> +
> +   Consider the four questions:
> +
> +   Can a store to x1 interfere with px2->y2?
>
s/y2/y1/

> +   (*px2).z2
>
Hmm?

> +   Can a store to x1 change the value pointed to by with py?
> +   Can a store to x1 change the value pointed to by with pz?
> +
> +   The answer to these questions can be yes, yes, yes, and no.
> +
>
Why 'no' on the last one?  'pz' is never assigned anywhere?  Are
these globals or locals?

> +   The first two questions can be answered with a simple examination
> +   of the type system.  If structure X contains a field of type Y then
> +   a store thru a pointer to an X can overwrite any field that is
> +   contained (recursively) in an X (unless we know that px1 != px2).
> +
> +   The last two of the questions can be solved in the same way as the
> +   first two questions but this is too conservative.  The observation
> +   is that if the address of a field is not explicitly taken and the
> +   type is completely local to the compilation unit, then it is
> +   impossible that a pointer to an instance of the field type overlaps
> +   with an enclosing structure.
> +
Make the example more self-contained.  You are making assumptions
about the code that are not clear from the snippet.

> @@ -1889,6 +1932,7 @@ aliases_everything_p (rtx mem)
>    return 0;
>  }
>  
> +/*#define AGRESSIVE_ALIASING 1*/
>
No.  If the common type bug still exists and it's not fixable for
now, just add a FIXME comment.  File a PR for it, if you want.

> +/* The code in this module is called by the ipa pass manager. It
> +   should be one of the later passes since it's information is used by
>
s/it's/its/

> +/* Return true if the variable T is the right kind of static variable to
> +   perform compilation unit scope escape analysis.  */
> +
> +static inline void 
> +check_decl (funct_state local, 
> +	    tree t, bool checking_write)
>
The function doesn't return anything.  You also need to document
arguments.

> +
> +  /* Since we have dispatched the locals and params, if we are writing, this
> +     cannot be a pure or constant function.  */
>
Hmm?  What do you mean by this?

> +
> +  if (SSA_VAR_P (t) || (TREE_CODE (t) == FUNCTION_DECL))
> +    check_operand (local, t, checking_write);
>
check_operand only works on VAR_DECLs.  This may be
related to my confusion in check_decl.  We don't seem to be
handling any kind of decl other than VAR_DECLs.

> +}
> +
> +/* Given a memory reference T, will return the variable at the bottom
> +   of the access.  Unlike get_base_address, this will recurse thru
> +   INDIRECT_REFS.  */
> +
> +static tree
> +get_base_var (tree t)
> +{
>
There is a local copy of this in every ipa-*.c file.  Better to
make it extern and put in tree-dfa.c.

> +static void
> +check_rhs_var (funct_state local, tree t)
> +{
> +  look_for_address_of (local, t);
> +
> +  /* Memcmp and strlen can both trap and they are declared pure.  */
>
Hmm?  The comment seems disconnected to the code.

> +  if (tree_could_trap_p (t)
> +      && local->pure_const_state == IPA_CONST)
> +    local->pure_const_state = IPA_PURE;
> +
> +  check_tree(local, t, false);
>
Spacing after check_tree.

> +}
> +
> +/* Check to see if T is an assignment to a static var we are
> +   interrested in analyzing.  LOCAL is passed in to get access to its bit
>
s/interrested/interested/

> +   vectors.
> +*/
> +
Close comment in line above with ".  */"

> +static void
> +check_lhs_var (funct_state local, tree t)
> +{
> +  /* Memcmp and strlen can both trap and they are declared pure.  */
>
Again.  What's the comment trying to explain?

> +/* This is a scaled down version of get_asm_expr_operands from
> +   tree_ssa_operands.c.  The version there runs much later and assumes
> +   that aliasing information is already available. Here we are just
> +   trying to find if the set of inputs and outputs contain references
> +   or address of operations to local static variables.  FN is the
> +   function being analyzed and STMT is the actual asm statement.  */
> +
Update argument documentation.  There is no FN argument.

> +/* Check the parameters of a function call from CALLER to CALL_EXPR to
> +   see if any of them are static vars.  Also check to see if this is
> +   either an indirect call, a call outside the compilation unit, or
> +   has special attributes that effect the clobbers.  The caller
> +   parameter is the tree node for the caller and the second operand is
> +   the tree node for the entire call expression.  */
> +
Same here.

> +  /* The const and pure flags are set by a variety of places in the
> +     compiler (including here).  If someone has already set the flags
> +     for the callee, (such as for some of the builtins) we will use
> +     them, otherwise we will compute our own information.  */
> +  
> +  /* Const and pure functions have less clobber effects than other
> +     functions so we process these first.  Otherwise if it is a call
> +     outside the compilation unit or an indirect call we punt.  This
> +     leaves local calls which will be processed by following the call
> +     graph.  */  
>
Join the two comments.

> +/* Walk tree and record all calls.  Called via walk_tree.  The data is
> +   the function that is being scanned.  */
> +/* TP is the part of the tree currently under the
> +   microscope. WALK_SUBTREES is part of the walk_tree api but is
> +   unused here.  DATA is cgraph_node of the function being walked.  */
> +
Same here.  In fact, the first comment is unnecessary.

> +static tree
> +scan_for_static_refs (tree *tp, 
> +		      int *walk_subtrees, 
> +		      void *data)
> +{
>
This needs to be re-implemented to use the operand infastructure
once we get all the bodies in SSA form.  Could you add a FIXME note?

This is also duplicated in ipa-pure-const.c.  Any reason why the
two cannot be shared?


> +	  /* This is an utter HACK FIXEME PLEASE!!!!!!!  The C++
> +	     front end, for reasons that are only apparent to it's
> +	     designers, decides that it was only kidding when it
> +	     generated some of the functions.  As a further joke it
> +	     then decides, after compilation is underway, to null
> +	     out the DECL_STRUCT_FUNCTION (node->decl) field of
> +	     these function so that no one can use them.  What a
> +	     funny joke!!!!  */
>
No point getting too excited.  But I guess it gave you a headache
or two.  Is there a PR for this problem?

> +	  gcc_assert (DECL_STRUCT_FUNCTION (fn->decl));
>
I don't understand where the hack is.  We are simply asserting
that we do have a DECL_STRUCT_FUNCTION.  The code doesn't seem to
be working around the problem.

> +  /* FIXME - When Jan gets the local statics promoted to the global
> +     variable list, the next two loops go away.  */
>
Better not to name names, say "When we get local statics
promoted...".  10 years from now this comment may not
make sense.

> +  /* There are some shared nodes, in particular the initializers on
> +     static declarations.  We do not need to scan them more than once
> +     since all we would be interrested in are the addressof
>
s/interrested/interested/

> +  /* Process all of the functions. 
> +
> +     We do not want to process any of the clones so we check that this
> +     is a master clone.  However, we do NOT process any
> +     AVAIL_OVERWRITABLE functions (these are never clones) we cannot
> +     guarantee that what we learn about the one we see will be true
> +     for the one that overriders it.
> +  */
Join comment closing.  There's a few others floating around that
need the same change.

> +/* FIXME -- PROFILE-RESTRUCTURE: There are a large number of places in
> +   this module that will need to be changed when the final engineering
> +   is done to support ssa form in more than a single function at a
> +   time.
> +
Pre-approved once we get var_ann()->uid out.

> +/* This is a scaled down version of get_asm_expr_operands from
> +   tree_ssa_operands.c.  The version there runs much later and assumes
> +   that aliasing information is already available. Here we are just
> +   trying to find if the set of inputs and outputs contain references
> +   or address of operations to local static variables.  FN is the
> +   function being analyzed and STMT is the actual asm statement.  */
> +
> +static void
> +get_asm_expr_operands (ipa_reference_local_vars_info_t local, tree stmt)
>
Share code with the other version of get_asm_expr_operands.
There's quite a bit of duplication in the two analysis (the IL
scanning parts).

> +
> +  /* Prune out the variables that were found to behave badly
> +     (i.e. have there address taken).  */
>
s/there/their/

> +  /* Propagate the local information thru the call graph to produce
> +     the global information.  All the nodes within a cycle will have
> +     the same info so we collapse cycles first.  Then we can do the
> +     propagation in one pass from the leaves to the roots.  */
> +  order_pos = ipa_utils_reduced_inorder (order, true, true);
> +  if (dump_file)
> +    ipa_utils_print_order(dump_file, "reduced", order, order_pos);
> +
Some of this propagation code could be made generic so that you
can call it from all the analyses.  Perhaps they can be pushed
into ipa-utils.c

Also, could you factor out the debugging dumps and cleanup code?

> +   FULL_ESCAPE - when bad things happen to good types. One of the
> +   following things happens to the type: (a) either an instance of the
> +   variable has it's address passed to an externally visible function,
>
s/it's/its/

> +
> +/* Map the several instances of a type into a single instance.  These
> +   can arise in several ways, nono of which can be justified except by
> +   laziness and stupidity.  */
>
Hmm?  I can't parse this comment.

> +static bitmap_obstack ipa_obstack;
> +
> +/* All of the "unique_type" code is a hack to get around the sleezy
>
s/sleezy/sleazy/

> +/* Find the unique representative for a type with UID.  */  
> +static int
> +unique_type_id_for (int uid, bool allow_missing ATTRIBUTE_UNUSED)
>
No ATTRIBUTE_UNUSED is necessary.

> +    {
> +      /* ICE when compiling libstdc++.  */
> +      if (!allow_missing)
> +	abort();
>
gcc_unreachable ().

> +/* Check the parameters of a function call from CALLER to CALL_EXPR to
> +   see if any of them are static vars.  Also check to see if this is
> +   either an indirect call, a call outside the compilation unit, or
> +   has special attributes that effect the clobbers.  The caller
> +   parameter is the tree node for the caller and the second operand is
> +   the tree node for the entire call expression.  */
> +
>
Comment doesn't match code.

> +/* OP0 is the one we *know* is a pointer type.
> +   OP1 may be a pointer type.  */
>
What is CODE?  What does the function do?  What's the meaning of
its return value?

> +/* Use a completely lame algorithm for removing duplicate types.  This
> +   code should not be here except for a bad implementation of whole
> +   program compilation.  */
>
This comment makes no sense to me.  Care to elaborate?  What's
missing?  What's lame about it?

> +/* It is not necessary to carry around the addressof map for any
> +   escaping TYPE.  */
> + 
Better document what the function does.

> +/* Transitively close the addressof bitmap for the type with UID.
> +   This means that if we had a.b and b.c, a would have both b an c in
> +   it's maps.  */ 
> +
s/it's/its/

> +/* The main entry point for type escape analysis.  */
> +
> +static void
> +type_escape_execute (void)
> +{
>
Quite a bit of code could be factored out here.  I've noticed
several similarities in all the analyses.  Could you factor out
common parts and have all the analyses use them?

> +  /* Map the duplicate types to a single unique type.  This is a hack
> +     it is not a general algorithm.  */
>
In what sense is it a hack?  What would be the real solution?

> +/* Return a INPUT_BITMAP for the asm inputs and OUTPUT_BITMAP for the
> +   asm outputs of static variables written by the asm STMT.  */
> +
This is not what this function is doing.  It's adding *any* decl
it finds to the bitmaps.  Why not just check for statics instead
of relying on set intersection later on?

However, I don't like most of the code in this file.  It's
directly manipulating trees instead of using the operand
interface.  It doesn't need to because at this point we *do* have
all the operand stuff built.  Please re-implement.


Diego.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]