[2/2] Reimplementation of build_ref_for_offset

Hi,

thank you very much for your comments.

On Thu, Sep 09, 2010 at 11:29:29AM +0200, Richard Guenther wrote:
> On Wed, 8 Sep 2010, Martin Jambor wrote:
> 
> > 2010-09-08  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	PR tree-optimization/44972
> > 	* tree-sra.c: Include toplev.h.
> > 	(build_ref_for_offset): Entirely reimplemented.
> > 	(build_ref_for_model): New function.
> > 	(build_user_friendly_ref_for_offset): New function.
> > 	(analyze_access_subtree): Removed build_ref_for_offset check.
> > 	(propagate_subaccesses_across_link): Likewise.
> > 	(create_artificial_child_access): Use
> > 	build_user_friendly_ref_for_offset.
> > 	(propagate_subaccesses_across_link): Likewise.
> > 	(ref_expr_for_all_replacements_p): Removed.
> > 	(generate_subtree_copies): Updated comment.  Use build_ref_for_model.
> > 	(sra_modify_expr): Use build_ref_for_model.
> > 	(load_assign_lhs_subreplacements): Likewise.
> > 	(sra_modify_assign): Removed ref_expr_for_all_replacements_p checks,
> > 	checks for return values of build_ref_for_offset.
> > 	* ipa-cp.c (ipcp_lattice_from_jfunc): No need to check return value of
> > 	build_ref_for_offset.
> > 	* ipa-prop.h: Include gimple.h
> > 	* ipa-prop.c (ipa_compute_jump_functions): Update to look for MEM_REFs.
> > 	(ipa_analyze_indirect_call_uses): Update comment.
> > 	* Makefile.in (tree-sra.o): Add $(GIMPLE_H) to dependencies.
> > 	(IPA_PROP_H): Likewise.
> > 
> > 	* testsuite/gcc.dg/ipa/ipa-sra-1.c: Adjust scanning expressions.
> > 	* testsuite/gcc.dg/tree-ssa/pr45144.c: Likewise.
> > 	* testsuite/gcc.dg/tree-ssa/forwprop-5.c: Likewise and scan optimzed
> > 	dump instead.
> > 	* testsuite/g++.dg/torture/pr34850.C: Remove expected warning.
> >         * gcc/testsuite/g++.dg/torture/pr44972.C: New test.
> > 
> > Index: mine/gcc/tree-sra.c
> > ===================================================================
> > --- mine.orig/gcc/tree-sra.c
> > +++ mine/gcc/tree-sra.c
> > @@ -76,6 +76,7 @@ along with GCC; see the file COPYING3.
> >  #include "coretypes.h"
> >  #include "alloc-pool.h"
> >  #include "tm.h"
> > +#include "toplev.h"
> >  #include "tree.h"
> >  #include "gimple.h"
> >  #include "cgraph.h"
> > @@ -1320,15 +1321,114 @@ make_fancy_name (tree expr)
> >    return XOBFINISH (&name_obstack, char *);
> >  }
> >  
> > -/* Helper function for build_ref_for_offset.
> > +/* Construct a MEM_REF that would reference a part of aggregate BASE of type
> > +   EXP_TYPE at the given OFFSET.  If BASE is something for which
> > +   get_addr_base_and_unit_offset returns NULL, gsi must be non-NULL and is used
> > +   to insert new statements either before or below the current one as specified
> > +   by INSERT_AFTER.  If offset is not aligned to bytes or EXP_TYPE is a
> > +   bit_field.
> 
> .. then?  (The last sentence looks incomplete)

The whole sentence should have been removed, for some reason part of
it wasn't.

> 
>   This function is not capable of handling bitfields.  */
> > +
> > +tree
> > +build_ref_for_offset (tree base, HOST_WIDE_INT offset,
> > +		      tree exp_type, gimple_stmt_iterator *gsi,
> > +		      bool insert_after)
> > +{
> > +  tree prev_base = base;
> > +  tree off;
> > +  location_t loc = EXPR_LOCATION (base);
> > +  HOST_WIDE_INT base_offset;
> >  
> > -   FIXME: Eventually this should be rewritten to either re-use the
> > -   original access expression unshared (which is good for alias
> > -   analysis) or to build a MEM_REF expression.  */
> > +  gcc_checking_assert (offset % BITS_PER_UNIT == 0);
> > +
> > +  base = get_addr_base_and_unit_offset (base, &base_offset);
> > +  if (!base)
> > +    {
> 
> Can you explain when we get here?  Can base be a reference
> with a variable offset like a[i]?  

Yes, I have added a comment with this example.

> I suppose it can't be a
> bit-field-ref, as on top of that we should never build another
> ref.

No.  I tried to think of a scenario when that could happen but no, for
several reasons it can't.

> 
> > +      gimple stmt;
> > +      tree tmp, addr;
> > +
> > +      gcc_checking_assert (gsi);
> > +      tmp = create_tmp_reg (build_pointer_type (TREE_TYPE (prev_base)), NULL);
> > +      add_referenced_var (tmp);
> > +      tmp = make_ssa_name (tmp, NULL);
> > +      addr = build_fold_addr_expr (unshare_expr (prev_base));
> > +      stmt = gimple_build_assign (tmp, addr);
> > +      SSA_NAME_DEF_STMT (tmp) = stmt;
> > +      if (insert_after)
> > +	gsi_insert_after (gsi, stmt, GSI_NEW_STMT);
> > +      else
> > +	gsi_insert_before (gsi, stmt, GSI_SAME_STMT);
> > +
> 
> I think you need to update_stmt (stmt) here.

Right.

> 
> > +      off = build_int_cst (reference_alias_ptr_type (prev_base),
> > +			   offset / BITS_PER_UNIT);
> > +      base = tmp;
> > +    }
> > +  else if (TREE_CODE (base) == MEM_REF)
> > +    {
> > +      off = build_int_cst (TREE_TYPE (TREE_OPERAND (base, 1)),
> > +			   base_offset + offset / BITS_PER_UNIT);
> > +      off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1), off, 0);
> > +      base = unshare_expr (TREE_OPERAND (base, 0));
> > +    }
> > +  else
> > +    {
> > +      off = build_int_cst (reference_alias_ptr_type (base),
> > +			   base_offset + offset / BITS_PER_UNIT);
> > +      base = build_fold_addr_expr (unshare_expr (base));
> > +    }
> > +
> > +  return fold_build2_loc (loc, MEM_REF, exp_type, base, off);
> > +}
> 
> Ok sofar.
> 
> > +/* Construct a memory reference to a part of an aggregate BASE at the given
> > +   OFFSET and of the same type as MODEL.  In case this is a reference to a
> > +   bit-field, the function will replicate the last component_ref of model's
> > +   expr to access it.  GSI and INSERT_AFTER have the same meaning as in
> > +   build_ref_for_offset.  */
> > +
> > +static tree
> > +build_ref_for_model (tree base, HOST_WIDE_INT offset,
> > +		     struct access *model, gimple_stmt_iterator *gsi,
> > +		     bool insert_after)
> > +{
> > +  tree t, exp_type;
> > +  bool bitfield;
> > +
> > +  if (offset % BITS_PER_UNIT != 0
> > +      || model->size < BITS_PER_UNIT
> > +      || exact_log2 (model->size) == -1)
> > +    {
>          /* This access looks like a bitfield.  */
> > +      gcc_checking_assert (TREE_CODE (model->expr) == COMPONENT_REF);
> 
> If you have the COMPONENT_REF, why not do
> 
>    if (TREE_CODE (model->expr) == COMPONENT_REF
>        && DECL_BIT_FIELD (TREE_OPERAND (model->expr, 1)))
> 
> instead of the offset/size checks?  That way it is 100% obvious.
> 

Yeah, this is much nicer.

> > +      offset -= int_bit_position (TREE_OPERAND (model->expr, 1));
> > +      gcc_assert (offset % BITS_PER_UNIT == 0);
> 
> We're doing that check in build_ref_for_offset.

True, I removed this one.

> 
> > +      exp_type = TREE_TYPE (TREE_OPERAND (model->expr, 0));
> > +      bitfield = true;
> > +    }
> > +  else
> > +    {
> > +      exp_type = model->type;
> > +      bitfield = false;
> > +    }
> > +
> > +  t = build_ref_for_offset (base, offset, exp_type, gsi, insert_after);
> 
> Replicating this call in both arms above would make it easier to
> distinguish both cases.

OK

>  Which makes me wonder if in either of
> the case we can pass NULL for gsi?  (relates to the question in
> build_ref_for_offset)

Yes, IPA-CP and inlining do that.  This is safe because they always
pass a base which is an is_gimple_ip_invariant - an address of (a part
of) a global object.

> 
> > +  if (bitfield)
> > +    t = fold_build3_loc (EXPR_LOCATION (base), COMPONENT_REF, model->type, t,
> 
> You use base location here, shouldn't this be model->expr location
> instead?  If there are more callers to build_ref_for_offset I think
> it would make sense to pass the location to use for the built MEM_REF
> to it, rather than hard-coding the base location inside.

Yes, that is exactly my plan.  Actually, I think most of the time the
location of the expression or the statement that is currently being
replaced/processed is what I'll pass down.  I intend to look at all
location handling in SRA (mainly because of PR 45505).  I'll prepare a
followup patch.

> >
...

> > @@ -1418,31 +1509,6 @@ build_ref_for_offset_1 (tree *res, tree
> >      }
> >  }
> 
> Ok.  I see the point about warnings, but when are MEM_REF trees
> displayed in a debugger?

Thinking about it again, they most certainly aren't.  It was the
warnings I was primarily concerned about.

> > @@ -1823,13 +1889,7 @@ analyze_access_subtree (struct access *r
> >  
> >    if (allow_replacements && scalar && !root->first_child
> >        && (root->grp_hint
> > -	  || (root->grp_write && (direct_read || root->grp_assignment_read)))
> > -      /* We must not ICE later on when trying to build an access to the
> > -	 original data within the aggregate even when it is impossible to do in
> > -	 a defined way like in the PR 42703 testcase.  Therefore we check
> > -	 pre-emptively here that we will be able to do that.  */
> > -      && build_ref_for_offset (NULL, TREE_TYPE (root->base), root->offset,
> > -			       root->type, false))
> > +	  || (root->grp_write && (direct_read || root->grp_assignment_read))))
> >      {
> >        if (dump_file && (dump_flags & TDF_DETAILS))
> >  	{
> > @@ -1914,12 +1974,11 @@ create_artificial_child_access (struct a
> >  {
> >    struct access *access;
> >    struct access **child;
> > -  tree expr = parent->base;;
> > +  tree expr = parent->base;
> >  
> >    gcc_assert (!model->grp_unscalarizable_region);
> > -
> > -  if (!build_ref_for_offset (&expr, TREE_TYPE (expr), new_offset,
> > -			     model->type, false))
> > +  if (!build_user_friendly_ref_for_offset (&expr, TREE_TYPE (expr), new_offset,
> > +					   model->type))
> >      return NULL;
> 
> Hm, so we don't fall back to creating a MEM_REF here?  Or is this
> one case of relaxing checks that you want to postpone for later
> (which I think is fine)?

Yes, I'll need to add some flag to make sure any resulting
replacements would have their TREE_NO_WARNING set.

Most of the checks I'd like to go over and see whether we could do
without them are those looking for V_C_Es.  However, at the moment
I'm having some problems bootstrapping Ada without them.

...

> > Index: mine/gcc/testsuite/gcc.dg/tree-ssa/pr45144.c
> > ===================================================================
> > --- mine.orig/gcc/testsuite/gcc.dg/tree-ssa/pr45144.c
> > +++ mine/gcc/testsuite/gcc.dg/tree-ssa/pr45144.c
> > @@ -42,5 +42,5 @@ bar (unsigned orig, unsigned *new)
> >    *new = foo (&a);
> >  }
> >  
> > -/* { dg-final { scan-tree-dump "x = a;" "optimized"} } */
> > +/* { dg-final { scan-tree-dump " = VIEW_CONVERT_EXPR<unsigned int>\\(a\\);" "optimized"} } */
> 
> Nice.
> 
> >  /* { dg-final { cleanup-tree-dump "optimized" } } */
> > Index: mine/gcc/ipa-prop.c
> > ===================================================================
> > --- mine.orig/gcc/ipa-prop.c
> > +++ mine/gcc/ipa-prop.c
> > @@ -916,23 +916,27 @@ ipa_compute_jump_functions (struct cgrap
> >  static tree
> >  ipa_get_member_ptr_load_param (tree rhs, bool use_delta)
> >  {
> > -  tree rec, fld;
> > +  tree rec, ref_offset, fld_offset;
> >    tree ptr_field;
> >    tree delta_field;
> >  
> > -  if (TREE_CODE (rhs) != COMPONENT_REF)
> > +  if (TREE_CODE (rhs) != MEM_REF)
> >      return NULL_TREE;
> 
> Are you sure we never have a COMPONENT_REF here?  We are not
> generally lowering them to MEM_REFs.

The whole pattern matching relies on that ESRA has been run on the
code (and I guess the subsequent early passes too), otherwise it would
not work.  So yes, I'm quite sure checking for MEM_REF is sufficient.

> 
> The patch looks ok in general, minus my minor comments.
> 

Thanks a lot, this is what I have just bootstrapped and tested on
x86_64-linux and what I am going to commit.

Martin

2010-09-09  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/44972
	* tree-sra.c: Include toplev.h.
	(build_ref_for_offset): Entirely reimplemented.
	(build_ref_for_model): New function.
	(build_user_friendly_ref_for_offset): New function.
	(analyze_access_subtree): Removed build_ref_for_offset check.
	(propagate_subaccesses_across_link): Likewise.
	(create_artificial_child_access): Use
	build_user_friendly_ref_for_offset.
	(propagate_subaccesses_across_link): Likewise.
	(ref_expr_for_all_replacements_p): Removed.
	(generate_subtree_copies): Updated comment.  Use build_ref_for_model.
	(sra_modify_expr): Use build_ref_for_model.
	(load_assign_lhs_subreplacements): Likewise.
	(sra_modify_assign): Removed ref_expr_for_all_replacements_p checks,
	checks for return values of build_ref_for_offset.
	* ipa-cp.c (ipcp_lattice_from_jfunc): No need to check return value of
	build_ref_for_offset.
	* ipa-prop.h: Include gimple.h
	* ipa-prop.c (ipa_compute_jump_functions): Update to look for MEM_REFs.
	(ipa_analyze_indirect_call_uses): Update comment.
	* Makefile.in (tree-sra.o): Add $(GIMPLE_H) to dependencies.
	(IPA_PROP_H): Likewise.

	* testsuite/gcc.dg/ipa/ipa-sra-1.c: Adjust scanning expressions.
	* testsuite/gcc.dg/tree-ssa/pr45144.c: Likewise.
	* testsuite/gcc.dg/tree-ssa/forwprop-5.c: Likewise and scan optimzed
	dump instead.
	* testsuite/g++.dg/torture/pr34850.C: Remove expected warning.
        * gcc/testsuite/g++.dg/torture/pr44972.C: New test.

[2/2] Reimplementation of build_ref_for_offset

Commit Message

Patch