diff mbox

[RFC] Remove call to build_ref_for_offset in IPA-SRA

Message ID 20100805131751.GC1800@virgil.arch.suse.de
State New
Headers show

Commit Message

Martin Jambor Aug. 5, 2010, 1:17 p.m. UTC
Hi,

On Tue, Aug 03, 2010 at 12:03:03PM +0200, Richard Guenther wrote:
> On Mon, Aug 2, 2010 at 10:20 PM, Martin Jambor <mjambor@suse.cz> wrote:
> > Hi,
> >
> > the following is another patch in the series that eventually should
> > re-implement build_ref_for_offset.  It basically re-implements it in
> > the IPA-SRA user of the function, I might eventually decide to use the
> > new implementation of the function instead but I fairly like it
> > separate because this (current) user of the function 1) always gets
> > offsets aligned to bytes and 2) it can be fed pointers, unlike all
> > others.
> >
> > Anyway, the problem with this patch is of course the hunk in expr.c.
> > However, without it I get ICEs when compiling testcase
> > g++.dg/torture/pr36445.C on x86_64-linux even though I believe I
> > produce correct gimple.  As far as I understand the code, if the V_C_E
> > path is not taken and we try to build and expand a BIT_FIELD_REF, I
> > get something that is not BLKmode because its argument is not and this
> > is not expected by emit_block_move which attempts to use it.  I am not
> > sure what exactly and exactly under what circumstances needs to behave
> > differently.  I intend to go back to this problem but I am new to the
> > code, and thus I would appreciate any help with that.  Otherwise the
> > patch bootstraps and tests fine.
> >
> > Thanks,
> >
> > Martin
> >
> >
> > 2010-07-28  Martin Jambor  <mjambor@suse.cz>
> >
> >        * ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
> >        calling build_ref_for_offset.
> >        * expr.c (expand_expr_real_1): When expanding MEM_REF, use V_C_E also
> >        when sizes don't match but expected mode is BLKmode.
> >
> >        * testsuite/g++.dg/torture/pr34850.C: Remove expected warning.
> >
> > Index: mine/gcc/ipa-prop.c
> > ===================================================================
> > --- mine.orig/gcc/ipa-prop.c
> > +++ mine/gcc/ipa-prop.c
> > @@ -2149,40 +2149,63 @@ ipa_modify_call_arguments (struct cgraph
> >        }
> >       else if (!adj->remove_param)
> >        {
> > -         tree expr, orig_expr;
> > -         bool allow_ptr, repl_found;
> > +         tree expr, base, off;
> > +         location_t loc;
> >
> > -         orig_expr = expr = gimple_call_arg (stmt, adj->base_index);
> > -         if (TREE_CODE (expr) == ADDR_EXPR)
> > -           {
> > -             allow_ptr = false;
> > -             expr = TREE_OPERAND (expr, 0);
> > -           }
> > -         else
> > -           allow_ptr = true;
> > +         gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0);
> > +         base = gimple_call_arg (stmt, adj->base_index);
> > +         loc = EXPR_LOCATION (base);
> 
> If I read the following code correctly we build an access to
> &base + offset here?  The old allow_ptr code and the
> replacement looks odd.  Can't we just store a 'deref_p' flag
> in the ipa-parm adjustment structure?
> 
> Please add a comment what we are doing here.
> 
> What are we actually replacing?  A ptr argument that is
> dereferenced by a scalar arg?  An aggregate by-value arg
> with separate by-value args?

Note that this is the part where we transform calls in callees of the
function we are actually transforming.  I'm adding the following
comment:

	  /* We create a new parameter out of the value of the old one, we can
	     do the following kind of transformations:

	     - A scalar passed by reference is converted to a scalar passed by
               value.  (adj->by_ref is false and the type of the original
               actual argument is a pointer to a scalar).

             - A part of an aggregate is passed instead of the whole aggregate.
               The part can be passed either by value or by reference, this is
               determined by value of adj->by_ref.  Moreover, the code below
               handles both situations when the original aggregate is passed by
               value (its type is not a pointer) and when it is passed by
               reference (it is a pointer to an aggregate).

	     When the new argument is passed by reference (adj->by_ref is true)
	     it must be a part of an aggregate and therefore we form it by
	     simply taking the address of a reference inside the original
	     aggregate.  */


Turning scalars for which we have a decl in this function and which
are passed by reference into arguments by value is currently really
done by folding the resulting MEM_REF.  Perhaps I should special-case
it here.  But sometimes we're just dereferencing a pointer in the
caller and don't have a decl.

> 
> > -         repl_found = build_ref_for_offset (&expr, TREE_TYPE (expr),
> > -                                            adj->offset, adj->type,
> > -                                            allow_ptr);
> > -         if (repl_found)
> > +         if ((TREE_CODE (base) == ADDR_EXPR
> > +              && DECL_P (TREE_OPERAND (base, 0)))
> > +             || (TREE_CODE (base) != ADDR_EXPR
> > +                 && POINTER_TYPE_P (TREE_TYPE (base))))
> > +           off = build_int_cst (TREE_TYPE (base), adj->offset / BITS_PER_UNIT);
> > +         else
> >            {
> > -             if (adj->by_ref)
> > -               expr = build_fold_addr_expr (expr);
> > +             HOST_WIDE_INT base_offset;
> > +             tree prev_base;
> > +
> > +             if (TREE_CODE (base) == ADDR_EXPR)
> > +               base = TREE_OPERAND (base, 0);
> > +             prev_base = base;
> > +             base = get_addr_base_and_unit_offset (base, &base_offset);
> > +             if (!base)
> 
> Ugh - when does that happen?  (it shouldn't)

Any time we bump into an array_ref with a non-constant index (such as
in ptr->array[i].structure).  I believe we actually discussed this on
IRC.  What happens next is that I build a MEM_REF of and ADDR_EXPR of
a handled_component (which may possibly be of a MEM_REF) which is
bogus on its own but it is handled well by the subsequent call to
force_gimple_operand (I hadle it myself in the actual reimplementation
of build_ref_for_offsets because there I would not call
force_gimple_operand anyway).

> 
> > +               {
> > +                 base = build_fold_addr_expr (prev_base);
> > +                 off = build_int_cst (TREE_TYPE (base),
> > +                                      adj->offset / BITS_PER_UNIT);
> > +               }
> > +             else if (TREE_CODE (base) == MEM_REF)
> > +               {
> > +                 off = build_int_cst (TREE_TYPE (TREE_OPERAND (base,1)),
> > +                                      base_offset
> > +                                      + adj->offset / BITS_PER_UNIT);
> > +                 off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1),
> > +                                        off, 0);
> > +                 base = TREE_OPERAND (base, 0);
> > +               }
> > +             else
> > +               {
> > +                 off = build_int_cst (build_pointer_type (TREE_TYPE (base)),
> > +                                      base_offset
> > +                                      + adj->offset / BITS_PER_UNIT);
> > +                 base = build_fold_addr_expr (base);
> 
> You are choosing some random types for the offset.  Please try
> to preserve that of the original access.  (It's not clear to me what
> we are replacing with what here ...)
> 
> See tree.c:reference_alias_ptr_type for a way to get the alias
> type of an existing reference.

Well, I don't really understand what you mean by random here.  It is
true that I don't go for the TYPE_MAIN_VARIANT, is that the problem?
I have changed the patch a bit to actually use
reference_alias_ptr_type.  If the original argument came from a
MEM_REF, I do preserve the type from it.  If we have a base decl, I
use a pointer to it (or now its main variant).  Only when I just have
a pointer I simply trust its type.  Is that wrong now, do I somehow
have to propagate the reference type from the callee to the caller
here?

> 
> > +               }
> >            }
> > -         else
> > +
> > +         expr = fold_build2_loc (loc, MEM_REF, adj->type, base, off);
> > +         if (adj->by_ref)
> > +           expr = build_fold_addr_expr (expr);
> 
> Separating the by_ref (or allow_ptr?!) cases could make things
> easier to understand and avoid stripping off and re-building ADDR_EXPRs
> all the time?

With this patch there is no allow_ptr any more, all the other callers
pass false for this parameter to build_ref_for_offset - moreover,
allow_ptr simply allowed the original parameter - the one which is
being transformed into a new one - to be passed by reference (and it
was more of a sanity check anyway.  On the contrary, when adj->by_ref
is true, it means that the newly created parameter should be passed by
reference.  We make it such by simply encapsulating the reference into
an aggregate in an ADDR_EXPR.  Building of the reference would be
exactly the same and so I don't think separating the case would help
to streamline this.

> 
> I have a hard time extracting patches from google-mail, so if you
> can bounce me the patch to my suse address I can have a look
> at the expr.c hunk.

I believe that in gmail, in the menu next to the reply button, there
is a "Show original" command that can help with extracting patches.

Anyway, thanks for looking into this.  Below is a context diff of the
slightly amended version which might be more readable and I will also
attach unified diff. I'll also CC your suse address.

Martin


2010-08-04  Martin Jambor  <mjambor@suse.cz>

	* ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
	calling build_ref_for_offset.

	* expr.c (expand_expr_real_1): When expanding MEM_REF, use V_C_E also
	when sizes don't match but expected mode is BLKmode.

	* testsuite/g++.dg/torture/pr34850.C: Remove expected warning.

Index: mine/gcc/ipa-prop.c
===================================================================
*** mine.orig/gcc/ipa-prop.c
--- mine/gcc/ipa-prop.c
*************** ipa_modify_call_arguments (struct cgraph
*** 2149,2188 ****
  	}
        else if (!adj->remove_param)
  	{
! 	  tree expr, orig_expr;
! 	  bool allow_ptr, repl_found;
  
! 	  orig_expr = expr = gimple_call_arg (stmt, adj->base_index);
! 	  if (TREE_CODE (expr) == ADDR_EXPR)
! 	    {
! 	      allow_ptr = false;
! 	      expr = TREE_OPERAND (expr, 0);
! 	    }
! 	  else
! 	    allow_ptr = true;
  
! 	  repl_found = build_ref_for_offset (&expr, TREE_TYPE (expr),
! 					     adj->offset, adj->type,
! 					     allow_ptr);
! 	  if (repl_found)
  	    {
! 	      if (adj->by_ref)
! 		expr = build_fold_addr_expr (expr);
  	    }
! 	  else
  	    {
! 	      tree ptrtype = build_pointer_type (adj->type);
! 	      expr = orig_expr;
! 	      if (!POINTER_TYPE_P (TREE_TYPE (expr)))
! 		expr = build_fold_addr_expr (expr);
! 	      if (!useless_type_conversion_p (ptrtype, TREE_TYPE (expr)))
! 		expr = fold_convert (ptrtype, expr);
! 	      expr = fold_build2 (POINTER_PLUS_EXPR, ptrtype, expr,
! 				  build_int_cst (sizetype,
! 						 adj->offset / BITS_PER_UNIT));
! 	      if (!adj->by_ref)
! 		expr = fold_build1 (INDIRECT_REF, adj->type, expr);
  	    }
  	  expr = force_gimple_operand_gsi (&gsi, expr,
  					   adj->by_ref
  					   || is_gimple_reg_type (adj->type),
--- 2149,2232 ----
  	}
        else if (!adj->remove_param)
  	{
! 	  tree expr, base, off;
! 	  location_t loc;
  
! 	  /* We create a new parameter out of the value of the old one, we can
! 	     do the following kind of transformations:
! 
! 	     - A scalar passed by reference is converted to a scalar passed by
!                value.  (adj->by_ref is false and the type of the original
!                actual argument is a pointer to a scalar).
! 
!              - A part of an aggregate is passed instead of the whole aggregate.
!                The part can be passed either by value or by reference, this is
!                determined by value of adj->by_ref.  Moreover, the code below
!                handles both situations when the original aggregate is passed by
!                value (its type is not a pointer) and when it is passed by
!                reference (it is a pointer to an aggregate).
! 
! 	     When the new argument is passed by reference (adj->by_ref is true)
! 	     it must be a part of an aggregate and therefore we form it by
! 	     simply taking the address of a reference inside the original
! 	     aggregate.  */
! 
! 	  gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0);
! 	  base = gimple_call_arg (stmt, adj->base_index);
! 	  loc = EXPR_LOCATION (base);
  
! 	  if (TREE_CODE (base) == ADDR_EXPR
! 	      && DECL_P (TREE_OPERAND (base, 0)))
! 	    off = build_int_cst (reference_alias_ptr_type (base),
! 				 adj->offset / BITS_PER_UNIT);
! 	  else if (TREE_CODE (base) != ADDR_EXPR
! 		   && POINTER_TYPE_P (TREE_TYPE (base)))
! 	    off = build_int_cst (TREE_TYPE (base), adj->offset / BITS_PER_UNIT);
! 	  else
  	    {
! 	      HOST_WIDE_INT base_offset;
! 	      tree prev_base;
! 
! 	      if (TREE_CODE (base) == ADDR_EXPR)
! 		base = TREE_OPERAND (base, 0);
! 	      prev_base = base;
! 	      base = get_addr_base_and_unit_offset (base, &base_offset);
! 	      if (!base)
! 		{
! 		  base = build_fold_addr_expr (prev_base);
! 		  off = build_int_cst (reference_alias_ptr_type (prev_base),
! 				       adj->offset / BITS_PER_UNIT);
! 		}
! 	      else if (TREE_CODE (base) == MEM_REF)
! 		{
! 		  off = build_int_cst (TREE_TYPE (TREE_OPERAND (base,1)),
! 				       base_offset
! 				       + adj->offset / BITS_PER_UNIT);
! 		  off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1),
! 					 off, 0);
! 		  base = TREE_OPERAND (base, 0);
! 		}
! 	      else
! 		{
! 		  off = build_int_cst (reference_alias_ptr_type (base),
! 				       base_offset
! 				       + adj->offset / BITS_PER_UNIT);
! 		  base = build_fold_addr_expr (base);
! 		}
  	    }
! 
! 	  expr = fold_build2_loc (loc, MEM_REF, adj->type, base, off);
! 	  if (adj->by_ref)
! 	    expr = build_fold_addr_expr (expr);
! 
! 	  /* !!! Remove after testing */
! 	  if (dump_file)
  	    {
! 	      fprintf (dump_file, "Built MEM_REF: ");
! 	      print_generic_expr (dump_file, expr, 0);
! 	      fprintf (dump_file, "\n");
  	    }
+ 
  	  expr = force_gimple_operand_gsi (&gsi, expr,
  					   adj->by_ref
  					   || is_gimple_reg_type (adj->type),
Index: mine/gcc/expr.c
===================================================================
*** mine.orig/gcc/expr.c
--- mine/gcc/expr.c
*************** expand_expr_real_1 (tree exp, rtx target
*** 8709,8716 ****
  		tree bftype;
  		if (offset == 0
  		    && host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1)
! 		    && (GET_MODE_BITSIZE (DECL_MODE (base))
! 			== TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))))
  		  return expand_expr (build1 (VIEW_CONVERT_EXPR,
  					      TREE_TYPE (exp), base),
  				      target, tmode, modifier);
--- 8709,8719 ----
  		tree bftype;
  		if (offset == 0
  		    && host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1)
! 		    && ((GET_MODE_BITSIZE (DECL_MODE (base))
! 			 == TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp))))
! 			|| (TYPE_MODE (TREE_TYPE (exp)) == BLKmode
! 			    && (GET_MODE_BITSIZE (DECL_MODE (base))
! 			   >= TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))))))
  		  return expand_expr (build1 (VIEW_CONVERT_EXPR,
  					      TREE_TYPE (exp), base),
  				      target, tmode, modifier);
Index: mine/gcc/testsuite/g++.dg/torture/pr34850.C
===================================================================
*** mine.orig/gcc/testsuite/g++.dg/torture/pr34850.C
--- mine/gcc/testsuite/g++.dg/torture/pr34850.C
*************** extern "C" {
*** 11,17 ****
      extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__, __artificial__))
      void * memset (void *__dest, int __ch, size_t __len) throw () {
  	if (__builtin_constant_p (__len) && __len == 0)
! 	    __warn_memset_zero_len (); /* { dg-warning "" } */
      }
  }
  inline void clear_mem(void* ptr, u32bit n)    {
--- 11,17 ----
      extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__, __artificial__))
      void * memset (void *__dest, int __ch, size_t __len) throw () {
  	if (__builtin_constant_p (__len) && __len == 0)
! 	    __warn_memset_zero_len ();
      }
  }
  inline void clear_mem(void* ptr, u32bit n)    {

Comments

Richard Biener Aug. 5, 2010, 1:39 p.m. UTC | #1
On Thu, 5 Aug 2010, Martin Jambor wrote:

> Hi,
> 
> On Tue, Aug 03, 2010 at 12:03:03PM +0200, Richard Guenther wrote:
> > On Mon, Aug 2, 2010 at 10:20 PM, Martin Jambor <mjambor@suse.cz> wrote:
> > > Hi,
> > >
> > > the following is another patch in the series that eventually should
> > > re-implement build_ref_for_offset.  It basically re-implements it in
> > > the IPA-SRA user of the function, I might eventually decide to use the
> > > new implementation of the function instead but I fairly like it
> > > separate because this (current) user of the function 1) always gets
> > > offsets aligned to bytes and 2) it can be fed pointers, unlike all
> > > others.
> > >
> > > Anyway, the problem with this patch is of course the hunk in expr.c.
> > > However, without it I get ICEs when compiling testcase
> > > g++.dg/torture/pr36445.C on x86_64-linux even though I believe I
> > > produce correct gimple.  As far as I understand the code, if the V_C_E
> > > path is not taken and we try to build and expand a BIT_FIELD_REF, I
> > > get something that is not BLKmode because its argument is not and this
> > > is not expected by emit_block_move which attempts to use it.  I am not
> > > sure what exactly and exactly under what circumstances needs to behave
> > > differently.  I intend to go back to this problem but I am new to the
> > > code, and thus I would appreciate any help with that.  Otherwise the
> > > patch bootstraps and tests fine.
> > >
> > > Thanks,
> > >
> > > Martin
> > >
> > >
> > > 2010-07-28  Martin Jambor  <mjambor@suse.cz>
> > >
> > >        * ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
> > >        calling build_ref_for_offset.
> > >        * expr.c (expand_expr_real_1): When expanding MEM_REF, use V_C_E also
> > >        when sizes don't match but expected mode is BLKmode.
> > >
> > >        * testsuite/g++.dg/torture/pr34850.C: Remove expected warning.
> > >
> > > Index: mine/gcc/ipa-prop.c
> > > ===================================================================
> > > --- mine.orig/gcc/ipa-prop.c
> > > +++ mine/gcc/ipa-prop.c
> > > @@ -2149,40 +2149,63 @@ ipa_modify_call_arguments (struct cgraph
> > >        }
> > >       else if (!adj->remove_param)
> > >        {
> > > -         tree expr, orig_expr;
> > > -         bool allow_ptr, repl_found;
> > > +         tree expr, base, off;
> > > +         location_t loc;
> > >
> > > -         orig_expr = expr = gimple_call_arg (stmt, adj->base_index);
> > > -         if (TREE_CODE (expr) == ADDR_EXPR)
> > > -           {
> > > -             allow_ptr = false;
> > > -             expr = TREE_OPERAND (expr, 0);
> > > -           }
> > > -         else
> > > -           allow_ptr = true;
> > > +         gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0);
> > > +         base = gimple_call_arg (stmt, adj->base_index);
> > > +         loc = EXPR_LOCATION (base);
> > 
> > If I read the following code correctly we build an access to
> > &base + offset here?  The old allow_ptr code and the
> > replacement looks odd.  Can't we just store a 'deref_p' flag
> > in the ipa-parm adjustment structure?
> > 
> > Please add a comment what we are doing here.
> > 
> > What are we actually replacing?  A ptr argument that is
> > dereferenced by a scalar arg?  An aggregate by-value arg
> > with separate by-value args?
> 
> Note that this is the part where we transform calls in callees of the
> function we are actually transforming.  I'm adding the following
> comment:
> 
> 	  /* We create a new parameter out of the value of the old one, we can
> 	     do the following kind of transformations:
> 
> 	     - A scalar passed by reference is converted to a scalar passed by
>                value.  (adj->by_ref is false and the type of the original
>                actual argument is a pointer to a scalar).
> 
>              - A part of an aggregate is passed instead of the whole aggregate.
>                The part can be passed either by value or by reference, this is
>                determined by value of adj->by_ref.  Moreover, the code below
>                handles both situations when the original aggregate is passed by
>                value (its type is not a pointer) and when it is passed by
>                reference (it is a pointer to an aggregate).
> 
> 	     When the new argument is passed by reference (adj->by_ref is true)
> 	     it must be a part of an aggregate and therefore we form it by
> 	     simply taking the address of a reference inside the original
> 	     aggregate.  */
> 
> 
> Turning scalars for which we have a decl in this function and which
> are passed by reference into arguments by value is currently really
> done by folding the resulting MEM_REF.  Perhaps I should special-case
> it here.  But sometimes we're just dereferencing a pointer in the
> caller and don't have a decl.

Thanks, that helps.

> > 
> > > -         repl_found = build_ref_for_offset (&expr, TREE_TYPE (expr),
> > > -                                            adj->offset, adj->type,
> > > -                                            allow_ptr);
> > > -         if (repl_found)
> > > +         if ((TREE_CODE (base) == ADDR_EXPR
> > > +              && DECL_P (TREE_OPERAND (base, 0)))
> > > +             || (TREE_CODE (base) != ADDR_EXPR
> > > +                 && POINTER_TYPE_P (TREE_TYPE (base))))
> > > +           off = build_int_cst (TREE_TYPE (base), adj->offset / BITS_PER_UNIT);
> > > +         else
> > >            {
> > > -             if (adj->by_ref)
> > > -               expr = build_fold_addr_expr (expr);
> > > +             HOST_WIDE_INT base_offset;
> > > +             tree prev_base;
> > > +
> > > +             if (TREE_CODE (base) == ADDR_EXPR)
> > > +               base = TREE_OPERAND (base, 0);
> > > +             prev_base = base;
> > > +             base = get_addr_base_and_unit_offset (base, &base_offset);
> > > +             if (!base)
> > 
> > Ugh - when does that happen?  (it shouldn't)
> 
> Any time we bump into an array_ref with a non-constant index (such as
> in ptr->array[i].structure).  I believe we actually discussed this on
> IRC.  What happens next is that I build a MEM_REF of and ADDR_EXPR of
> a handled_component (which may possibly be of a MEM_REF) which is
> bogus on its own but it is handled well by the subsequent call to
> force_gimple_operand (I hadle it myself in the actual reimplementation
> of build_ref_for_offsets because there I would not call
> force_gimple_operand anyway).

Can you add a comment?  Like /* Aggregate arguments can have non-invariant
addresses.  */

> > 
> > > +               {
> > > +                 base = build_fold_addr_expr (prev_base);
> > > +                 off = build_int_cst (TREE_TYPE (base),
> > > +                                      adj->offset / BITS_PER_UNIT);
> > > +               }
> > > +             else if (TREE_CODE (base) == MEM_REF)
> > > +               {
> > > +                 off = build_int_cst (TREE_TYPE (TREE_OPERAND (base,1)),
> > > +                                      base_offset
> > > +                                      + adj->offset / BITS_PER_UNIT);
> > > +                 off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1),
> > > +                                        off, 0);
> > > +                 base = TREE_OPERAND (base, 0);
> > > +               }
> > > +             else
> > > +               {
> > > +                 off = build_int_cst (build_pointer_type (TREE_TYPE (base)),
> > > +                                      base_offset
> > > +                                      + adj->offset / BITS_PER_UNIT);
> > > +                 base = build_fold_addr_expr (base);
> > 
> > You are choosing some random types for the offset.  Please try
> > to preserve that of the original access.  (It's not clear to me what
> > we are replacing with what here ...)
> > 
> > See tree.c:reference_alias_ptr_type for a way to get the alias
> > type of an existing reference.
> 
> Well, I don't really understand what you mean by random here.  It is
> true that I don't go for the TYPE_MAIN_VARIANT, is that the problem?
> I have changed the patch a bit to actually use
> reference_alias_ptr_type.  If the original argument came from a
> MEM_REF, I do preserve the type from it.  If we have a base decl, I
> use a pointer to it (or now its main variant).  Only when I just have
> a pointer I simply trust its type.  Is that wrong now, do I somehow
> have to propagate the reference type from the callee to the caller
> here?

It depends really where the original dereference happens.  If the caller
passes a pointer to the callee and the callee ends up dereferencing it
then the access alias type needs to be transfered from the callee
dereference to the caller if it chooses to pass by value instead
(and thus performs the dereference).  But as far as I see this
information is currently not available - you only have access to
the call statement and to what is in adj->type (what type is this?).
So we might want to defer this correctness fix to a followup.

> > 
> > > +               }
> > >            }
> > > -         else
> > > +
> > > +         expr = fold_build2_loc (loc, MEM_REF, adj->type, base, off);
> > > +         if (adj->by_ref)
> > > +           expr = build_fold_addr_expr (expr);
> > 
> > Separating the by_ref (or allow_ptr?!) cases could make things
> > easier to understand and avoid stripping off and re-building ADDR_EXPRs
> > all the time?
> 
> With this patch there is no allow_ptr any more, all the other callers
> pass false for this parameter to build_ref_for_offset - moreover,
> allow_ptr simply allowed the original parameter - the one which is
> being transformed into a new one - to be passed by reference (and it
> was more of a sanity check anyway.  On the contrary, when adj->by_ref
> is true, it means that the newly created parameter should be passed by
> reference.  We make it such by simply encapsulating the reference into
> an aggregate in an ADDR_EXPR.  Building of the reference would be
> exactly the same and so I don't think separating the case would help
> to streamline this.

Yep, I was confused and with the big comment it now makes sense.

> > 
> > I have a hard time extracting patches from google-mail, so if you
> > can bounce me the patch to my suse address I can have a look
> > at the expr.c hunk.
> 
> I believe that in gmail, in the menu next to the reply button, there
> is a "Show original" command that can help with extracting patches.

It still mangles whitespace for me :/

> Anyway, thanks for looking into this.  Below is a context diff of the
> slightly amended version which might be more readable and I will also
> attach unified diff. I'll also CC your suse address.

Thanks, I'll look into the expr.c hunk later (it still doesn't make
too much sense to me).

Otherwise the patch looks ok.

Thanks,
Richard.

> Martin
> 
> 
> 2010-08-04  Martin Jambor  <mjambor@suse.cz>
> 
> 	* ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
> 	calling build_ref_for_offset.
> 
> 	* expr.c (expand_expr_real_1): When expanding MEM_REF, use V_C_E also
> 	when sizes don't match but expected mode is BLKmode.
> 
> 	* testsuite/g++.dg/torture/pr34850.C: Remove expected warning.
> 
> Index: mine/gcc/ipa-prop.c
> ===================================================================
> *** mine.orig/gcc/ipa-prop.c
> --- mine/gcc/ipa-prop.c
> *************** ipa_modify_call_arguments (struct cgraph
> *** 2149,2188 ****
>   	}
>         else if (!adj->remove_param)
>   	{
> ! 	  tree expr, orig_expr;
> ! 	  bool allow_ptr, repl_found;
>   
> ! 	  orig_expr = expr = gimple_call_arg (stmt, adj->base_index);
> ! 	  if (TREE_CODE (expr) == ADDR_EXPR)
> ! 	    {
> ! 	      allow_ptr = false;
> ! 	      expr = TREE_OPERAND (expr, 0);
> ! 	    }
> ! 	  else
> ! 	    allow_ptr = true;
>   
> ! 	  repl_found = build_ref_for_offset (&expr, TREE_TYPE (expr),
> ! 					     adj->offset, adj->type,
> ! 					     allow_ptr);
> ! 	  if (repl_found)
>   	    {
> ! 	      if (adj->by_ref)
> ! 		expr = build_fold_addr_expr (expr);
>   	    }
> ! 	  else
>   	    {
> ! 	      tree ptrtype = build_pointer_type (adj->type);
> ! 	      expr = orig_expr;
> ! 	      if (!POINTER_TYPE_P (TREE_TYPE (expr)))
> ! 		expr = build_fold_addr_expr (expr);
> ! 	      if (!useless_type_conversion_p (ptrtype, TREE_TYPE (expr)))
> ! 		expr = fold_convert (ptrtype, expr);
> ! 	      expr = fold_build2 (POINTER_PLUS_EXPR, ptrtype, expr,
> ! 				  build_int_cst (sizetype,
> ! 						 adj->offset / BITS_PER_UNIT));
> ! 	      if (!adj->by_ref)
> ! 		expr = fold_build1 (INDIRECT_REF, adj->type, expr);
>   	    }
>   	  expr = force_gimple_operand_gsi (&gsi, expr,
>   					   adj->by_ref
>   					   || is_gimple_reg_type (adj->type),
> --- 2149,2232 ----
>   	}
>         else if (!adj->remove_param)
>   	{
> ! 	  tree expr, base, off;
> ! 	  location_t loc;
>   
> ! 	  /* We create a new parameter out of the value of the old one, we can
> ! 	     do the following kind of transformations:
> ! 
> ! 	     - A scalar passed by reference is converted to a scalar passed by
> !                value.  (adj->by_ref is false and the type of the original
> !                actual argument is a pointer to a scalar).
> ! 
> !              - A part of an aggregate is passed instead of the whole aggregate.
> !                The part can be passed either by value or by reference, this is
> !                determined by value of adj->by_ref.  Moreover, the code below
> !                handles both situations when the original aggregate is passed by
> !                value (its type is not a pointer) and when it is passed by
> !                reference (it is a pointer to an aggregate).
> ! 
> ! 	     When the new argument is passed by reference (adj->by_ref is true)
> ! 	     it must be a part of an aggregate and therefore we form it by
> ! 	     simply taking the address of a reference inside the original
> ! 	     aggregate.  */
> ! 
> ! 	  gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0);
> ! 	  base = gimple_call_arg (stmt, adj->base_index);
> ! 	  loc = EXPR_LOCATION (base);
>   
> ! 	  if (TREE_CODE (base) == ADDR_EXPR
> ! 	      && DECL_P (TREE_OPERAND (base, 0)))
> ! 	    off = build_int_cst (reference_alias_ptr_type (base),
> ! 				 adj->offset / BITS_PER_UNIT);
> ! 	  else if (TREE_CODE (base) != ADDR_EXPR
> ! 		   && POINTER_TYPE_P (TREE_TYPE (base)))
> ! 	    off = build_int_cst (TREE_TYPE (base), adj->offset / BITS_PER_UNIT);
> ! 	  else
>   	    {
> ! 	      HOST_WIDE_INT base_offset;
> ! 	      tree prev_base;
> ! 
> ! 	      if (TREE_CODE (base) == ADDR_EXPR)
> ! 		base = TREE_OPERAND (base, 0);
> ! 	      prev_base = base;
> ! 	      base = get_addr_base_and_unit_offset (base, &base_offset);
> ! 	      if (!base)
> ! 		{
> ! 		  base = build_fold_addr_expr (prev_base);
> ! 		  off = build_int_cst (reference_alias_ptr_type (prev_base),
> ! 				       adj->offset / BITS_PER_UNIT);
> ! 		}
> ! 	      else if (TREE_CODE (base) == MEM_REF)
> ! 		{
> ! 		  off = build_int_cst (TREE_TYPE (TREE_OPERAND (base,1)),
> ! 				       base_offset
> ! 				       + adj->offset / BITS_PER_UNIT);
> ! 		  off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1),
> ! 					 off, 0);
> ! 		  base = TREE_OPERAND (base, 0);
> ! 		}
> ! 	      else
> ! 		{
> ! 		  off = build_int_cst (reference_alias_ptr_type (base),
> ! 				       base_offset
> ! 				       + adj->offset / BITS_PER_UNIT);
> ! 		  base = build_fold_addr_expr (base);
> ! 		}
>   	    }
> ! 
> ! 	  expr = fold_build2_loc (loc, MEM_REF, adj->type, base, off);
> ! 	  if (adj->by_ref)
> ! 	    expr = build_fold_addr_expr (expr);
> ! 
> ! 	  /* !!! Remove after testing */
> ! 	  if (dump_file)
>   	    {
> ! 	      fprintf (dump_file, "Built MEM_REF: ");
> ! 	      print_generic_expr (dump_file, expr, 0);
> ! 	      fprintf (dump_file, "\n");
>   	    }
> + 
>   	  expr = force_gimple_operand_gsi (&gsi, expr,
>   					   adj->by_ref
>   					   || is_gimple_reg_type (adj->type),
> Index: mine/gcc/expr.c
> ===================================================================
> *** mine.orig/gcc/expr.c
> --- mine/gcc/expr.c
> *************** expand_expr_real_1 (tree exp, rtx target
> *** 8709,8716 ****
>   		tree bftype;
>   		if (offset == 0
>   		    && host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1)
> ! 		    && (GET_MODE_BITSIZE (DECL_MODE (base))
> ! 			== TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))))
>   		  return expand_expr (build1 (VIEW_CONVERT_EXPR,
>   					      TREE_TYPE (exp), base),
>   				      target, tmode, modifier);
> --- 8709,8719 ----
>   		tree bftype;
>   		if (offset == 0
>   		    && host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1)
> ! 		    && ((GET_MODE_BITSIZE (DECL_MODE (base))
> ! 			 == TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp))))
> ! 			|| (TYPE_MODE (TREE_TYPE (exp)) == BLKmode
> ! 			    && (GET_MODE_BITSIZE (DECL_MODE (base))
> ! 			   >= TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))))))
>   		  return expand_expr (build1 (VIEW_CONVERT_EXPR,
>   					      TREE_TYPE (exp), base),
>   				      target, tmode, modifier);
> Index: mine/gcc/testsuite/g++.dg/torture/pr34850.C
> ===================================================================
> *** mine.orig/gcc/testsuite/g++.dg/torture/pr34850.C
> --- mine/gcc/testsuite/g++.dg/torture/pr34850.C
> *************** extern "C" {
> *** 11,17 ****
>       extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__, __artificial__))
>       void * memset (void *__dest, int __ch, size_t __len) throw () {
>   	if (__builtin_constant_p (__len) && __len == 0)
> ! 	    __warn_memset_zero_len (); /* { dg-warning "" } */
>       }
>   }
>   inline void clear_mem(void* ptr, u32bit n)    {
> --- 11,17 ----
>       extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__, __artificial__))
>       void * memset (void *__dest, int __ch, size_t __len) throw () {
>   	if (__builtin_constant_p (__len) && __len == 0)
> ! 	    __warn_memset_zero_len ();
>       }
>   }
>   inline void clear_mem(void* ptr, u32bit n)    {
>
Richard Biener Aug. 5, 2010, 2:57 p.m. UTC | #2
On Thu, 5 Aug 2010, Martin Jambor wrote:

> Hi,
> 
> On Tue, Aug 03, 2010 at 12:03:03PM +0200, Richard Guenther wrote:
> > On Mon, Aug 2, 2010 at 10:20 PM, Martin Jambor <mjambor@suse.cz> wrote:
> > > Hi,
> > >
> > > the following is another patch in the series that eventually should
> > > re-implement build_ref_for_offset.  It basically re-implements it in
> > > the IPA-SRA user of the function, I might eventually decide to use the
> > > new implementation of the function instead but I fairly like it
> > > separate because this (current) user of the function 1) always gets
> > > offsets aligned to bytes and 2) it can be fed pointers, unlike all
> > > others.
> > >
> > > Anyway, the problem with this patch is of course the hunk in expr.c.
> > > However, without it I get ICEs when compiling testcase
> > > g++.dg/torture/pr36445.C on x86_64-linux even though I believe I
> > > produce correct gimple.  As far as I understand the code, if the V_C_E

I'm testing the following instead.

Richard.

Index: gcc/expr.c
===================================================================
*** gcc/expr.c	(revision 162913)
--- gcc/expr.c	(working copy)
*************** store_expr (tree exp, rtx target, int ca
*** 4752,4762 ****
  	{
  	  int unsignedp = TYPE_UNSIGNED (TREE_TYPE (exp));
  	  if (GET_MODE (target) == BLKmode
! 		   || GET_MODE (temp) == BLKmode)
  	    emit_block_move (target, temp, expr_size (exp),
  			     (call_param_p
  			      ? BLOCK_OP_CALL_PARM
  			      : BLOCK_OP_NORMAL));
  	  else
  	    convert_move (target, temp, unsignedp);
  	}
--- 4752,4765 ----
  	{
  	  int unsignedp = TYPE_UNSIGNED (TREE_TYPE (exp));
  	  if (GET_MODE (target) == BLKmode
! 	      && GET_MODE (temp) == BLKmode)
  	    emit_block_move (target, temp, expr_size (exp),
  			     (call_param_p
  			      ? BLOCK_OP_CALL_PARM
  			      : BLOCK_OP_NORMAL));
+ 	  else if (GET_MODE (target) == BLKmode)
+ 	    store_bit_field (target, INTVAL (expr_size (exp)) * BITS_PER_UNIT,
+ 			     0, GET_MODE (temp), temp);
  	  else
  	    convert_move (target, temp, unsignedp);
  	}
diff mbox

Patch

2010-08-04  Martin Jambor  <mjambor@suse.cz>

	* ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
	calling build_ref_for_offset.

	* expr.c (expand_expr_real_1): When expanding MEM_REF, use V_C_E also
	when sizes don't match but expected mode is BLKmode.

	* testsuite/g++.dg/torture/pr34850.C: Remove expected warning.

Index: mine/gcc/ipa-prop.c
===================================================================
--- mine.orig/gcc/ipa-prop.c
+++ mine/gcc/ipa-prop.c
@@ -2149,40 +2149,84 @@  ipa_modify_call_arguments (struct cgraph
 	}
       else if (!adj->remove_param)
 	{
-	  tree expr, orig_expr;
-	  bool allow_ptr, repl_found;
+	  tree expr, base, off;
+	  location_t loc;
 
-	  orig_expr = expr = gimple_call_arg (stmt, adj->base_index);
-	  if (TREE_CODE (expr) == ADDR_EXPR)
-	    {
-	      allow_ptr = false;
-	      expr = TREE_OPERAND (expr, 0);
-	    }
-	  else
-	    allow_ptr = true;
+	  /* We create a new parameter out of the value of the old one, we can
+	     do the following kind of transformations:
+
+	     - A scalar passed by reference is converted to a scalar passed by
+               value.  (adj->by_ref is false and the type of the original
+               actual argument is a pointer to a scalar).
+
+             - A part of an aggregate is passed instead of the whole aggregate.
+               The part can be passed either by value or by reference, this is
+               determined by value of adj->by_ref.  Moreover, the code below
+               handles both situations when the original aggregate is passed by
+               value (its type is not a pointer) and when it is passed by
+               reference (it is a pointer to an aggregate).
+
+	     When the new argument is passed by reference (adj->by_ref is true)
+	     it must be a part of an aggregate and therefore we form it by
+	     simply taking the address of a reference inside the original
+	     aggregate.  */
+
+	  gcc_checking_assert (adj->offset % BITS_PER_UNIT == 0);
+	  base = gimple_call_arg (stmt, adj->base_index);
+	  loc = EXPR_LOCATION (base);
 
-	  repl_found = build_ref_for_offset (&expr, TREE_TYPE (expr),
-					     adj->offset, adj->type,
-					     allow_ptr);
-	  if (repl_found)
+	  if (TREE_CODE (base) == ADDR_EXPR
+	      && DECL_P (TREE_OPERAND (base, 0)))
+	    off = build_int_cst (reference_alias_ptr_type (base),
+				 adj->offset / BITS_PER_UNIT);
+	  else if (TREE_CODE (base) != ADDR_EXPR
+		   && POINTER_TYPE_P (TREE_TYPE (base)))
+	    off = build_int_cst (TREE_TYPE (base), adj->offset / BITS_PER_UNIT);
+	  else
 	    {
-	      if (adj->by_ref)
-		expr = build_fold_addr_expr (expr);
+	      HOST_WIDE_INT base_offset;
+	      tree prev_base;
+
+	      if (TREE_CODE (base) == ADDR_EXPR)
+		base = TREE_OPERAND (base, 0);
+	      prev_base = base;
+	      base = get_addr_base_and_unit_offset (base, &base_offset);
+	      if (!base)
+		{
+		  base = build_fold_addr_expr (prev_base);
+		  off = build_int_cst (reference_alias_ptr_type (prev_base),
+				       adj->offset / BITS_PER_UNIT);
+		}
+	      else if (TREE_CODE (base) == MEM_REF)
+		{
+		  off = build_int_cst (TREE_TYPE (TREE_OPERAND (base,1)),
+				       base_offset
+				       + adj->offset / BITS_PER_UNIT);
+		  off = int_const_binop (PLUS_EXPR, TREE_OPERAND (base, 1),
+					 off, 0);
+		  base = TREE_OPERAND (base, 0);
+		}
+	      else
+		{
+		  off = build_int_cst (reference_alias_ptr_type (base),
+				       base_offset
+				       + adj->offset / BITS_PER_UNIT);
+		  base = build_fold_addr_expr (base);
+		}
 	    }
-	  else
+
+	  expr = fold_build2_loc (loc, MEM_REF, adj->type, base, off);
+	  if (adj->by_ref)
+	    expr = build_fold_addr_expr (expr);
+
+	  /* !!! Remove after testing */
+	  if (dump_file)
 	    {
-	      tree ptrtype = build_pointer_type (adj->type);
-	      expr = orig_expr;
-	      if (!POINTER_TYPE_P (TREE_TYPE (expr)))
-		expr = build_fold_addr_expr (expr);
-	      if (!useless_type_conversion_p (ptrtype, TREE_TYPE (expr)))
-		expr = fold_convert (ptrtype, expr);
-	      expr = fold_build2 (POINTER_PLUS_EXPR, ptrtype, expr,
-				  build_int_cst (sizetype,
-						 adj->offset / BITS_PER_UNIT));
-	      if (!adj->by_ref)
-		expr = fold_build1 (INDIRECT_REF, adj->type, expr);
+	      fprintf (dump_file, "Built MEM_REF: ");
+	      print_generic_expr (dump_file, expr, 0);
+	      fprintf (dump_file, "\n");
 	    }
+
 	  expr = force_gimple_operand_gsi (&gsi, expr,
 					   adj->by_ref
 					   || is_gimple_reg_type (adj->type),
Index: mine/gcc/expr.c
===================================================================
--- mine.orig/gcc/expr.c
+++ mine/gcc/expr.c
@@ -8709,8 +8709,11 @@  expand_expr_real_1 (tree exp, rtx target
 		tree bftype;
 		if (offset == 0
 		    && host_integerp (TYPE_SIZE (TREE_TYPE (exp)), 1)
-		    && (GET_MODE_BITSIZE (DECL_MODE (base))
-			== TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))))
+		    && ((GET_MODE_BITSIZE (DECL_MODE (base))
+			 == TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp))))
+			|| (TYPE_MODE (TREE_TYPE (exp)) == BLKmode
+			    && (GET_MODE_BITSIZE (DECL_MODE (base))
+			   >= TREE_INT_CST_LOW (TYPE_SIZE (TREE_TYPE (exp)))))))
 		  return expand_expr (build1 (VIEW_CONVERT_EXPR,
 					      TREE_TYPE (exp), base),
 				      target, tmode, modifier);
Index: mine/gcc/testsuite/g++.dg/torture/pr34850.C
===================================================================
--- mine.orig/gcc/testsuite/g++.dg/torture/pr34850.C
+++ mine/gcc/testsuite/g++.dg/torture/pr34850.C
@@ -11,7 +11,7 @@  extern "C" {
     extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__, __artificial__))
     void * memset (void *__dest, int __ch, size_t __len) throw () {
 	if (__builtin_constant_p (__len) && __len == 0)
-	    __warn_memset_zero_len (); /* { dg-warning "" } */
+	    __warn_memset_zero_len ();
     }
 }
 inline void clear_mem(void* ptr, u32bit n)    {