Patchwork [1/2] Remove the IPA-SRA call to build_ref_for_offset

login
register
mail settings
Submitter Martin Jambor
Date Sept. 8, 2010, 4:43 p.m.
Message ID <20100908164349.353922665@virgil.suse.cz>
Download mbox | patch
Permalink /patch/64176/
State New
Headers show

Comments

Martin Jambor - Sept. 8, 2010, 4:43 p.m.
Hi,

this is the same patch I sent here in August:
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg00609.html

It reimplements parameter alterations in callers of a function being
modified by IPA-SRA so that it always produces MEM_REF relying on
force_gimple_operand_gsi to break up refs if necessary (e.g. if we
have a mem_ref of an addr_expr of an array_ref with variable index of
another mem_ref etc...).

I have bootstrapped and tested this patch separately on x86_64-linux.
OK for trunk?

Thanks,

Martin


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

	PR tree-optimization/44972
	* ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
	calling build_ref_for_offset.

	* testsuite/g++.dg/torture/pr34850.C: Remove expected warning.
Richard Guenther - Sept. 9, 2010, 8:49 a.m.
On Wed, 8 Sep 2010, Martin Jambor wrote:

> Hi,
> 
> this is the same patch I sent here in August:
> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg00609.html
> 
> It reimplements parameter alterations in callers of a function being
> modified by IPA-SRA so that it always produces MEM_REF relying on
> force_gimple_operand_gsi to break up refs if necessary (e.g. if we
> have a mem_ref of an addr_expr of an array_ref with variable index of
> another mem_ref etc...).
> 
> I have bootstrapped and tested this patch separately on x86_64-linux.
> OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
> 
> Martin
> 
> 
> 2010-08-08  Martin Jambor  <mjambor@suse.cz>
> 
> 	PR tree-optimization/44972
> 	* ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
> 	calling build_ref_for_offset.
> 
> 	* 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
> @@ -2153,40 +2153,77 @@ 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:
>  
> -	  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);
> -	    }
> +	     - 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
>  	    {
> -	      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);
> +	      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);
> +	      /* Aggregate arguments can have non-invariant addresses.  */
> +	      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);
> +
>  	  expr = force_gimple_operand_gsi (&gsi, expr,
>  					   adj->by_ref
>  					   || is_gimple_reg_type (adj->type),
> 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)    {
> 
>
H.J. Lu - Sept. 11, 2010, 12:21 a.m.
On Wed, Sep 8, 2010 at 9:43 AM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> this is the same patch I sent here in August:
> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg00609.html
>
> It reimplements parameter alterations in callers of a function being
> modified by IPA-SRA so that it always produces MEM_REF relying on
> force_gimple_operand_gsi to break up refs if necessary (e.g. if we
> have a mem_ref of an addr_expr of an array_ref with variable index of
> another mem_ref etc...).
>
> I have bootstrapped and tested this patch separately on x86_64-linux.
> OK for trunk?
>
> Thanks,
>
> Martin
>
>
> 2010-08-08  Martin Jambor  <mjambor@suse.cz>
>
>        PR tree-optimization/44972
>        * ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
>        calling build_ref_for_offset.
>
>        * testsuite/g++.dg/torture/pr34850.C: Remove expected warning.
>

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45644
H.J. Lu - Dec. 6, 2010, 6:56 p.m.
On Fri, Sep 10, 2010 at 5:21 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Sep 8, 2010 at 9:43 AM, Martin Jambor <mjambor@suse.cz> wrote:
>> Hi,
>>
>> this is the same patch I sent here in August:
>> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg00609.html
>>
>> It reimplements parameter alterations in callers of a function being
>> modified by IPA-SRA so that it always produces MEM_REF relying on
>> force_gimple_operand_gsi to break up refs if necessary (e.g. if we
>> have a mem_ref of an addr_expr of an array_ref with variable index of
>> another mem_ref etc...).
>>
>> I have bootstrapped and tested this patch separately on x86_64-linux.
>> OK for trunk?
>>
>> Thanks,
>>
>> Martin
>>
>>
>> 2010-08-08  Martin Jambor  <mjambor@suse.cz>
>>
>>        PR tree-optimization/44972
>>        * ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
>>        calling build_ref_for_offset.
>>
>>        * testsuite/g++.dg/torture/pr34850.C: Remove expected warning.
>>
>
> This caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45644
>

This also caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46823
H.J. Lu - Jan. 13, 2011, 11:03 p.m.
On Mon, Dec 6, 2010 at 10:56 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Sep 10, 2010 at 5:21 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Wed, Sep 8, 2010 at 9:43 AM, Martin Jambor <mjambor@suse.cz> wrote:
>>> Hi,
>>>
>>> this is the same patch I sent here in August:
>>> http://gcc.gnu.org/ml/gcc-patches/2010-08/msg00609.html
>>>
>>> It reimplements parameter alterations in callers of a function being
>>> modified by IPA-SRA so that it always produces MEM_REF relying on
>>> force_gimple_operand_gsi to break up refs if necessary (e.g. if we
>>> have a mem_ref of an addr_expr of an array_ref with variable index of
>>> another mem_ref etc...).
>>>
>>> I have bootstrapped and tested this patch separately on x86_64-linux.
>>> OK for trunk?
>>>
>>> Thanks,
>>>
>>> Martin
>>>
>>>
>>> 2010-08-08  Martin Jambor  <mjambor@suse.cz>
>>>
>>>        PR tree-optimization/44972
>>>        * ipa-prop.c (ipa_modify_call_arguments): Build MEM_REF instead of
>>>        calling build_ref_for_offset.
>>>
>>>        * testsuite/g++.dg/torture/pr34850.C: Remove expected warning.
>>>
>>
>> This caused:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=45644
>>
>
> This also caused:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46823
>

This also caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47283

Patch

Index: mine/gcc/ipa-prop.c
===================================================================
--- mine.orig/gcc/ipa-prop.c
+++ mine/gcc/ipa-prop.c
@@ -2153,40 +2153,77 @@  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:
 
-	  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);
-	    }
+	     - 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
 	    {
-	      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);
+	      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);
+	      /* Aggregate arguments can have non-invariant addresses.  */
+	      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);
+
 	  expr = force_gimple_operand_gsi (&gsi, expr,
 					   adj->by_ref
 					   || is_gimple_reg_type (adj->type),
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)    {