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: [RFC PATCH] Remove call to build_ref_for_offset in IPA-SRA


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)    {
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)    {

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