This is the mail archive of the gcc@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: [GSoC: DDG export][RFC] Current status


"Alexander Monakov" <amonakov@gmail.com> writes:

> The implementation follows the outline presented in my initial message
> on the project, here: http://gcc.gnu.org/ml/gcc/2007-03/msg00900.html
> .  Data references and data dependency relations obtained via
> compute_dependencies_for_loop are copied into containing struct (which
> is a member of struct function).  This containing struct is marked
> GTY((skip)), because its lifetime and lifetime of its components is
> known (from the moment of export until destruction late in the RTL
> pipeline).  However, I need to preserve trees that are referenced in
> the exported datarefs (to be able to bind MEMs to datarefs later), so
> I need to put them into a GC-visible array.  For now that array is
> global.  Can it be done better?

It seems to me that this array could also be in the function struct.
I don't see why not.  Given the way it is used, it also seems to me
that this array should be a pointer_map.

> MEMs are bound to datarefs by means of saving original trees for MEMs
> during expand.  This is a large part of the attached patch with
> changes to emit-rtl.[ch], which adds new field to struct mem_attrs
> (mem_orig_expr), initializes it and propagates through various RTL
> transformations.  Notice that it has stricter hashing  (as compared to
> orig_expr part of the same struct), so it will reduce sharing of
> mem_attrs (and also increase size of the struct, obviously).  Can it
> be a problem?  Please also note that this patch is a part of aliasing
> information exporting patch by Dmitry Melnik, which available now in
> alias-export branch.

Adding a new field to mem_attrs could be a problem.  You'll have to
measure to see how much the memory usage changes.

> After original tree is found, a corresponding dataref is looked up in
> exported info.  I guess I will need here hashtabs for fast lookup
> (also for searching ddrs for pairs of datarefs), am I right?

Yes, hash tables or pointer maps.

> The verifier also frequently breaks on passes that create unreachable
> basic blocks (because dominator analysis called from flow_loops_find
> asserts there are none).  For now I just place
> delete_unreachable_blocks in such passes, is that ok?

Not for the final patch, but it's OK for debugging.


> +/* If EXPR contains conversions at the root of the tree, all of them 
> +   will be removed.  */
> +
> +static tree
> +skip_conversions (tree expr)
> +{
> +  tree inner = expr;
> +  /* Remove any conversions: they don't change what the underlying
> +     object is.  Likewise for SAVE_EXPR.  */
> +  while (TREE_CODE (inner) == NOP_EXPR || TREE_CODE (inner) == CONVERT_EXPR
> +        || TREE_CODE (inner) == NON_LVALUE_EXPR
> +        || TREE_CODE (inner) == VIEW_CONVERT_EXPR
> +        || TREE_CODE (inner) == SAVE_EXPR)
> +    inner = TREE_OPERAND (inner, 0);
> +  return inner;
> +}

I don't understand why it's OK to throw all these away.  Some of them
change the meaning of the expression.  I also don't understand why you
want to throw them away.


> @@ -1401,27 +1456,36 @@ component_ref_for_mem_expr (tree ref)
>  {
>    tree inner = TREE_OPERAND (ref, 0);
>  
> -  if (TREE_CODE (inner) == COMPONENT_REF)
> +  /* Remove any conversions: they don't change what the underlying
> +     object is.  Likewise for SAVE_EXPR.  */
> +  while (TREE_CODE (inner) == NOP_EXPR || TREE_CODE (inner) == CONVERT_EXPR
> +	 || TREE_CODE (inner) == NON_LVALUE_EXPR
> +	 || TREE_CODE (inner) == VIEW_CONVERT_EXPR
> +	 || TREE_CODE (inner) == SAVE_EXPR)
> +    inner = TREE_OPERAND (inner, 0);
> +
> +  if (TREE_CODE (inner) == COMPONENT_REF || TREE_CODE (inner) == INDIRECT_REF)
>      inner = component_ref_for_mem_expr (inner);
>    else
>      {
> -      /* Now remove any conversions: they don't change what the underlying
> -	 object is.  Likewise for SAVE_EXPR.  */
> -      while (TREE_CODE (inner) == NOP_EXPR || TREE_CODE (inner) == CONVERT_EXPR
> -	     || TREE_CODE (inner) == NON_LVALUE_EXPR
> -	     || TREE_CODE (inner) == VIEW_CONVERT_EXPR
> -	     || TREE_CODE (inner) == SAVE_EXPR)
> -	inner = TREE_OPERAND (inner, 0);
> -
>        if (! DECL_P (inner))
>  	inner = NULL_TREE;
>      }
>  
>    if (inner == TREE_OPERAND (ref, 0))
>      return ref;
> -  else
> -    return build3 (COMPONENT_REF, TREE_TYPE (ref), inner,
> -		   TREE_OPERAND (ref, 1), NULL_TREE);
> +  else if (TREE_CODE (ref) == COMPONENT_REF)
> +    {
> +      return build3 (COMPONENT_REF, TREE_TYPE (ref), inner,
> +		     TREE_OPERAND (ref, 1), NULL_TREE);
> +    }
> +  else  /* INDIRECT_REF.  */
> +    {
> +      if (inner == NULL_TREE)
> +	return NULL_TREE;
> +      else
> +	return build1 (INDIRECT_REF, TREE_TYPE (ref), inner);
> +    }
>  }

The handling of INDIRECT_REF here seems clearly wrong.  You definitely
can't discard conversions for an INDIRECT_REF.


Hope this helps a bit.  I will have more luck understanding what is
going on when I see how you use the data.

Ian


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