Re: [PATCH] Limit VDEF walking in IPA-SRA when looking for potential modifications

[Richard Guenther <rguenther at suse dot de>]

On Mon, 2 Nov 2009, Martin Jambor wrote:

> Hi,
> 
> the patch below limits walking of VDEFs when searching for potential
> modifications of what a parameter points to by providing a common
> bitmap to various invocations of walk_aliased_vdefs which look for
> alias vdefs of the same thing.
> 
> This is not a regression fix but may avoid some unnecessary costly
> walks and the scope of the patch is small.  Therefore I'm still
> proposing to commit this to trunk.  I have bootstrapped and tested the
> patch on x86_64-linux with "yes" and "release" checking (both with
> Ada).
> 
> What do you think?

I wonder if it is correct.  How do the parm representatives
relate to the access expressions?  Thus, I would have expected
that ao_ref_init be moved outside of the inner loop and initialized
from repr->base.

If written as your patch then for example in

struct X { int i; float f; } *q;
double *x;
float foo (struct X *p)
{
  q->f = 1;
  *x = 0;
  return p->i + p->f;
}

if you start walking from the access p->i you visit both stores
and do not find an alias but mark both stmts as visited.  Then
you start walking from p->f and stop at *x because you have already
visited it.  And you do not see that q->f may possibly an alias
and grp_maybe_modified stays not set.

Thus, using the bitmap is only valid if you re-use it with the
same ao_ref or if you really really know what you are doing.

Richard.

> Martin
> 
> 
> 2009-11-02  Martin Jambor  <mjambor@suse.cz>
> 
> 	* tree-sra.c (analyze_modified_params): Pass a common bitmap to
> 	walk_aliased_vdefs.
> 
> 
> Index: mine/gcc/tree-sra.c
> ===================================================================
> --- mine.orig/gcc/tree-sra.c
> +++ mine/gcc/tree-sra.c
> @@ -2825,6 +2825,7 @@ analyze_modified_params (VEC (access_p,
>  	   repr = repr->next_grp)
>  	{
>  	  VEC (access_p, heap) *access_vec;
> +	  bitmap visited;
>  	  int j, access_count;
>  	  tree parm;
>  
> @@ -2835,6 +2836,7 @@ analyze_modified_params (VEC (access_p,
>  	      || repr->grp_maybe_modified)
>  	    continue;
>  
> +	  visited = BITMAP_ALLOC (NULL);
>  	  access_vec = get_base_access_vector (parm);
>  	  access_count = VEC_length (access_p, access_vec);
>  	  for (j = 0; j < access_count; j++)
> @@ -2847,10 +2849,11 @@ analyze_modified_params (VEC (access_p,
>  	      access = VEC_index (access_p, access_vec, j);
>  	      ao_ref_init (&ar, access->expr);
>  	      walk_aliased_vdefs (&ar, gimple_vuse (access->stmt),
> -				  mark_maybe_modified, repr, NULL);
> +				  mark_maybe_modified, repr, &visited);
>  	      if (repr->grp_maybe_modified)
>  		break;
>  	    }
> +	  BITMAP_FREE (visited);
>  	}
>      }
>  }
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex