diff mbox

Do not compare types in operands_equal_p if OEP_ADDRESS_OF is set

Message ID 20151005230254.GA43326@kam.mff.cuni.cz
State New
Headers show

Commit Message

Jan Hubicka Oct. 5, 2015, 11:02 p.m. UTC 
Hi,
While looking for uses of useless_type_conversion on non-gimple register types
I run across few that seem to be completely unnecesary and I would like to get
rid of them in hope to get rid of code comparing functions/method type and
possibly more. 

usless_type_conversion is about operations on the types in gimple expressions,
not about memory accesses nor about function calls.

First on is in fold-const.c that may be used on MEM_REF of aggregate type.
As discussed earlier, the type compare is unnecesary when we only care about
address that seems to be the most comon case we get into this path.

I also disabled type matching done by operand_equal_p and cleaned up the
conditional of MEM_REF into multiple ones - for example it was passing
OEP_ADDRESS_OF when comparing TYPE_SIZE which is quite a nonsense.

I wonder what to do about OPE_CONSTANT_ADDRESS_OF.  This flag does not seem
to be used at all in current tree nor documented somehow.
I also made operand_equal_p to skip AA compare when -fno-strict-aliasing
is used.

Bootstrapped/regtested x86_64-linux, OK?

Honza

	* fold-const.c (operand_equal_p): When in OEP_ADDRESS_OF
	do not require types to match; also relax checking of MEM_REF.

Comments

Richard Biener Oct. 6, 2015, 8:41 a.m. UTC | #1 
On Tue, 6 Oct 2015, Jan Hubicka wrote:

> Hi,
> While looking for uses of useless_type_conversion on non-gimple register types
> I run across few that seem to be completely unnecesary and I would like to get
> rid of them in hope to get rid of code comparing functions/method type and
> possibly more. 
> 
> usless_type_conversion is about operations on the types in gimple expressions,
> not about memory accesses nor about function calls.
> 
> First on is in fold-const.c that may be used on MEM_REF of aggregate type.
> As discussed earlier, the type compare is unnecesary when we only care about
> address that seems to be the most comon case we get into this path.
> 
> I also disabled type matching done by operand_equal_p and cleaned up the
> conditional of MEM_REF into multiple ones - for example it was passing
> OEP_ADDRESS_OF when comparing TYPE_SIZE which is quite a nonsense.
> 
> I wonder what to do about OPE_CONSTANT_ADDRESS_OF.  This flag does not seem
> to be used at all in current tree nor documented somehow.

Eric added that.  It's set when seeing ADDR_EXPRs and has an extra
special handling when TREE_SIDE_EFFECTS are tested.  It matters for
Ada I guess.

> I also made operand_equal_p to skip AA compare when -fno-strict-aliasing
> is used.
>
> Bootstrapped/regtested x86_64-linux, OK?

See comments below - otherwise it looks good.

Richard.

> Honza
> 
> 	* fold-const.c (operand_equal_p): When in OEP_ADDRESS_OF
> 	do not require types to match; also relax checking of MEM_REF.
> Index: fold-const.c
> ===================================================================
> --- fold-const.c	(revision 228131)
> +++ fold-const.c	(working copy)
> @@ -2712,26 +2712,31 @@ operand_equal_p (const_tree arg0, const_
>    if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
>      return tree_int_cst_equal (arg0, arg1);
>  
> -  /* If both types don't have the same signedness, then we can't consider
> -     them equal.  We must check this before the STRIP_NOPS calls
> -     because they may change the signedness of the arguments.  As pointers
> -     strictly don't have a signedness, require either two pointers or
> -     two non-pointers as well.  */
> -  if (TYPE_UNSIGNED (TREE_TYPE (arg0)) != TYPE_UNSIGNED (TREE_TYPE (arg1))
> -      || POINTER_TYPE_P (TREE_TYPE (arg0)) != POINTER_TYPE_P (TREE_TYPE (arg1)))
> -    return 0;
> +  if (!(flags & OEP_ADDRESS_OF))
> +    {
> +      /* If both types don't have the same signedness, then we can't consider
> +	 them equal.  We must check this before the STRIP_NOPS calls
> +	 because they may change the signedness of the arguments.  As pointers
> +	 strictly don't have a signedness, require either two pointers or
> +	 two non-pointers as well.  */
> +      if (TYPE_UNSIGNED (TREE_TYPE (arg0)) != TYPE_UNSIGNED (TREE_TYPE (arg1))
> +	  || POINTER_TYPE_P (TREE_TYPE (arg0))
> +			     != POINTER_TYPE_P (TREE_TYPE (arg1)))
> +	return 0;
>  
> -  /* We cannot consider pointers to different address space equal.  */
> -  if (POINTER_TYPE_P (TREE_TYPE (arg0)) && POINTER_TYPE_P (TREE_TYPE (arg1))
> -      && (TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg0)))
> -	  != TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg1)))))
> -    return 0;
> +      /* We cannot consider pointers to different address space equal.  */
> +      if (POINTER_TYPE_P (TREE_TYPE (arg0))
> +			  && POINTER_TYPE_P (TREE_TYPE (arg1))
> +	  && (TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg0)))
> +	      != TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg1)))))
> +	return 0;
>  
> -  /* If both types don't have the same precision, then it is not safe
> -     to strip NOPs.  */
> -  if (element_precision (TREE_TYPE (arg0))
> -      != element_precision (TREE_TYPE (arg1)))
> -    return 0;
> +      /* If both types don't have the same precision, then it is not safe
> +	 to strip NOPs.  */
> +      if (element_precision (TREE_TYPE (arg0))
> +	  != element_precision (TREE_TYPE (arg1)))
> +	return 0;

It's odd that you move this under the !OEP_ADDRESS_OF case but
not the STRIP_NOPS itself.

> +    }
>  
>    STRIP_NOPS (arg0);
>    STRIP_NOPS (arg1);
> @@ -2935,27 +2940,34 @@ operand_equal_p (const_tree arg0, const_
>  
>  	case TARGET_MEM_REF:
>  	case MEM_REF:
> -	  /* Require equal access sizes, and similar pointer types.
> -	     We can have incomplete types for array references of
> -	     variable-sized arrays from the Fortran frontend
> -	     though.  Also verify the types are compatible.  */
> -	  if (!((TYPE_SIZE (TREE_TYPE (arg0)) == TYPE_SIZE (TREE_TYPE (arg1))
> -		   || (TYPE_SIZE (TREE_TYPE (arg0))
> -		       && TYPE_SIZE (TREE_TYPE (arg1))
> -		       && operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
> -					   TYPE_SIZE (TREE_TYPE (arg1)), flags)))
> -		  && types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1))
> -		  && ((flags & OEP_ADDRESS_OF)
> -		      || (alias_ptr_types_compatible_p
> -			    (TREE_TYPE (TREE_OPERAND (arg0, 1)),
> -			     TREE_TYPE (TREE_OPERAND (arg1, 1)))
> -			  && (MR_DEPENDENCE_CLIQUE (arg0)
> -			      == MR_DEPENDENCE_CLIQUE (arg1))
> -			  && (MR_DEPENDENCE_BASE (arg0)
> -			      == MR_DEPENDENCE_BASE (arg1))
> -			  && (TYPE_ALIGN (TREE_TYPE (arg0))
> -			    == TYPE_ALIGN (TREE_TYPE (arg1)))))))
> -	    return 0;
> +	  if (!(flags & OEP_ADDRESS_OF))
> +	    {
> +	      /* Require equal access sizes */
> +	      if (TYPE_SIZE (TREE_TYPE (arg0)) != TYPE_SIZE (TREE_TYPE (arg1))
> +		  && (!TYPE_SIZE (TREE_TYPE (arg0))
> +		      || !TYPE_SIZE (TREE_TYPE (arg1))
> +		      || !operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
> +					   TYPE_SIZE (TREE_TYPE (arg1)),
> +					   flags & ~OEP_CONSTANT_ADDRESS_OF)))

so you still pass OEP_ADDRESS_OF ...

> +		return 0;
> +	      /* Verify that access happens in similar types.  */
> +	      if (!types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1)))
> +		return 0;
> +	      /* Verify that accesses are TBAA compatible.  */
> +	      if ((flag_strict_aliasing
> +		   && !alias_ptr_types_compatible_p
> +		        (TREE_TYPE (TREE_OPERAND (arg0, 1)),
> +		         TREE_TYPE (TREE_OPERAND (arg1, 1))))
> +		  || MR_DEPENDENCE_CLIQUE (arg0)
> +		     != MR_DEPENDENCE_CLIQUE (arg1)
> +		  || MR_DEPENDENCE_BASE (arg0)
> +		     != MR_DEPENDENCE_BASE (arg1))
> +		return 0;
> +	    }
> +	   /* Verify that alignment is compatible.  */
> +	   if (TYPE_ALIGN (TREE_TYPE (arg0))
> +	       != TYPE_ALIGN (TREE_TYPE (arg1)))
> +	      return 0;

why's compatible alignment required for OEP_ADDRESS_OF?  We only
look at type alignment on memory references (see get_pointer_alignment
vs. get_object_alignment).

>  	  flags &= ~(OEP_CONSTANT_ADDRESS_OF|OEP_ADDRESS_OF);
>  	  return (OP_SAME (0) && OP_SAME (1)
>  		  /* TARGET_MEM_REF require equal extra operands.  */
> 
>
Eric Botcazou Oct. 6, 2015, 8:58 a.m. UTC | #2 
> I also disabled type matching done by operand_equal_p and cleaned up the
> conditional of MEM_REF into multiple ones - for example it was passing
> OEP_ADDRESS_OF when comparing TYPE_SIZE which is quite a nonsense.
> 
> I wonder what to do about OPE_CONSTANT_ADDRESS_OF.  This flag does not seem
> to be used at all in current tree nor documented somehow.

It is used and (un-)documented as OEP_ADDRESS_OF, see the ADDR_EXPR case:

     case ADDR_EXPR:
 return operand_equal_p (TREE_OPERAND (arg0, 0), TREE_OPERAND (arg1, 0),
			TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1)
			? OEP_CONSTANT_ADDRESS_OF | OEP_ADDRESS_OF : 0);

So it's OEP_ADDRESS_OF but for constant addresses.
Jan Hubicka Oct. 6, 2015, 6 p.m. UTC | #3 
Hi,

I see, OEP_CONSTANT_ADDRESS_OF is set in:
return operand_equal_p (TREE_OPERAND (arg0, 0), TREE_OPERAND (arg1, 0), 
                                TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1)    
                                ? OEP_CONSTANT_ADDRESS_OF | OEP_ADDRESS_OF : 0);
so it is not additive to OEP_ADDRESS_OF, I suppose the existing checks for OEP_ADDRESS_OF
in MEM_REF and INDIRECT_REF should also check for OEP_CONSTANT_ADDRESS_OF.  I will sent
separate patch for that.
> > -  if (element_precision (TREE_TYPE (arg0))
> > -      != element_precision (TREE_TYPE (arg1)))
> > -    return 0;
> > +      /* If both types don't have the same precision, then it is not safe
> > +	 to strip NOPs.  */
> > +      if (element_precision (TREE_TYPE (arg0))
> > +	  != element_precision (TREE_TYPE (arg1)))
> > +	return 0;
> 
> It's odd that you move this under the !OEP_ADDRESS_OF case but
> not the STRIP_NOPS itself.

Hmm, I suppose NOP_EXPR should not happen here as it does not have address defined.
I will try to assert there and move the statement around.
> 
> > +    }
> >  
> >    STRIP_NOPS (arg0);
> >    STRIP_NOPS (arg1);
> > @@ -2935,27 +2940,34 @@ operand_equal_p (const_tree arg0, const_
> >  
> >  	case TARGET_MEM_REF:
> >  	case MEM_REF:
> > -	  /* Require equal access sizes, and similar pointer types.
> > -	     We can have incomplete types for array references of
> > -	     variable-sized arrays from the Fortran frontend
> > -	     though.  Also verify the types are compatible.  */
> > -	  if (!((TYPE_SIZE (TREE_TYPE (arg0)) == TYPE_SIZE (TREE_TYPE (arg1))
> > -		   || (TYPE_SIZE (TREE_TYPE (arg0))
> > -		       && TYPE_SIZE (TREE_TYPE (arg1))
> > -		       && operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
> > -					   TYPE_SIZE (TREE_TYPE (arg1)), flags)))
> > -		  && types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1))
> > -		  && ((flags & OEP_ADDRESS_OF)
> > -		      || (alias_ptr_types_compatible_p
> > -			    (TREE_TYPE (TREE_OPERAND (arg0, 1)),
> > -			     TREE_TYPE (TREE_OPERAND (arg1, 1)))
> > -			  && (MR_DEPENDENCE_CLIQUE (arg0)
> > -			      == MR_DEPENDENCE_CLIQUE (arg1))
> > -			  && (MR_DEPENDENCE_BASE (arg0)
> > -			      == MR_DEPENDENCE_BASE (arg1))
> > -			  && (TYPE_ALIGN (TREE_TYPE (arg0))
> > -			    == TYPE_ALIGN (TREE_TYPE (arg1)))))))
> > -	    return 0;
> > +	  if (!(flags & OEP_ADDRESS_OF))
> > +	    {
> > +	      /* Require equal access sizes */
> > +	      if (TYPE_SIZE (TREE_TYPE (arg0)) != TYPE_SIZE (TREE_TYPE (arg1))
> > +		  && (!TYPE_SIZE (TREE_TYPE (arg0))
> > +		      || !TYPE_SIZE (TREE_TYPE (arg1))
> > +		      || !operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
> > +					   TYPE_SIZE (TREE_TYPE (arg1)),
> > +					   flags & ~OEP_CONSTANT_ADDRESS_OF)))
> 
> so you still pass OEP_ADDRESS_OF ...

I don't because it is tested earlier 
  if (!(flags & OEP_ADDRESS_OF))
> 
> > +		return 0;
> > +	      /* Verify that access happens in similar types.  */
> > +	      if (!types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1)))
> > +		return 0;
> > +	      /* Verify that accesses are TBAA compatible.  */
> > +	      if ((flag_strict_aliasing
> > +		   && !alias_ptr_types_compatible_p
> > +		        (TREE_TYPE (TREE_OPERAND (arg0, 1)),
> > +		         TREE_TYPE (TREE_OPERAND (arg1, 1))))
> > +		  || MR_DEPENDENCE_CLIQUE (arg0)
> > +		     != MR_DEPENDENCE_CLIQUE (arg1)
> > +		  || MR_DEPENDENCE_BASE (arg0)
> > +		     != MR_DEPENDENCE_BASE (arg1))
> > +		return 0;
> > +	    }
> > +	   /* Verify that alignment is compatible.  */
> > +	   if (TYPE_ALIGN (TREE_TYPE (arg0))
> > +	       != TYPE_ALIGN (TREE_TYPE (arg1)))
> > +	      return 0;
> 
> why's compatible alignment required for OEP_ADDRESS_OF?  We only
> look at type alignment on memory references (see get_pointer_alignment
> vs. get_object_alignment).

I actually tested it with the TYPE_ALIGN test in the condtional, too, so I know
it works.  Later I dediced to play safe and that possibly get_pointer_alignment
may want to do that.  Will move it back
to "if (!(flags & OEP_ADDRESS_OF))"

Honza
> 
> >  	  flags &= ~(OEP_CONSTANT_ADDRESS_OF|OEP_ADDRESS_OF);
> >  	  return (OP_SAME (0) && OP_SAME (1)
> >  		  /* TARGET_MEM_REF require equal extra operands.  */
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
diff mbox

Patch

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 228131)
+++ fold-const.c	(working copy)
@@ -2712,26 +2712,31 @@  operand_equal_p (const_tree arg0, const_
   if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
     return tree_int_cst_equal (arg0, arg1);
 
-  /* If both types don't have the same signedness, then we can't consider
-     them equal.  We must check this before the STRIP_NOPS calls
-     because they may change the signedness of the arguments.  As pointers
-     strictly don't have a signedness, require either two pointers or
-     two non-pointers as well.  */
-  if (TYPE_UNSIGNED (TREE_TYPE (arg0)) != TYPE_UNSIGNED (TREE_TYPE (arg1))
-      || POINTER_TYPE_P (TREE_TYPE (arg0)) != POINTER_TYPE_P (TREE_TYPE (arg1)))
-    return 0;
+  if (!(flags & OEP_ADDRESS_OF))
+    {
+      /* If both types don't have the same signedness, then we can't consider
+	 them equal.  We must check this before the STRIP_NOPS calls
+	 because they may change the signedness of the arguments.  As pointers
+	 strictly don't have a signedness, require either two pointers or
+	 two non-pointers as well.  */
+      if (TYPE_UNSIGNED (TREE_TYPE (arg0)) != TYPE_UNSIGNED (TREE_TYPE (arg1))
+	  || POINTER_TYPE_P (TREE_TYPE (arg0))
+			     != POINTER_TYPE_P (TREE_TYPE (arg1)))
+	return 0;
 
-  /* We cannot consider pointers to different address space equal.  */
-  if (POINTER_TYPE_P (TREE_TYPE (arg0)) && POINTER_TYPE_P (TREE_TYPE (arg1))
-      && (TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg0)))
-	  != TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg1)))))
-    return 0;
+      /* We cannot consider pointers to different address space equal.  */
+      if (POINTER_TYPE_P (TREE_TYPE (arg0))
+			  && POINTER_TYPE_P (TREE_TYPE (arg1))
+	  && (TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg0)))
+	      != TYPE_ADDR_SPACE (TREE_TYPE (TREE_TYPE (arg1)))))
+	return 0;
 
-  /* If both types don't have the same precision, then it is not safe
-     to strip NOPs.  */
-  if (element_precision (TREE_TYPE (arg0))
-      != element_precision (TREE_TYPE (arg1)))
-    return 0;
+      /* If both types don't have the same precision, then it is not safe
+	 to strip NOPs.  */
+      if (element_precision (TREE_TYPE (arg0))
+	  != element_precision (TREE_TYPE (arg1)))
+	return 0;
+    }
 
   STRIP_NOPS (arg0);
   STRIP_NOPS (arg1);
@@ -2935,27 +2940,34 @@  operand_equal_p (const_tree arg0, const_
 
 	case TARGET_MEM_REF:
 	case MEM_REF:
-	  /* Require equal access sizes, and similar pointer types.
-	     We can have incomplete types for array references of
-	     variable-sized arrays from the Fortran frontend
-	     though.  Also verify the types are compatible.  */
-	  if (!((TYPE_SIZE (TREE_TYPE (arg0)) == TYPE_SIZE (TREE_TYPE (arg1))
-		   || (TYPE_SIZE (TREE_TYPE (arg0))
-		       && TYPE_SIZE (TREE_TYPE (arg1))
-		       && operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
-					   TYPE_SIZE (TREE_TYPE (arg1)), flags)))
-		  && types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1))
-		  && ((flags & OEP_ADDRESS_OF)
-		      || (alias_ptr_types_compatible_p
-			    (TREE_TYPE (TREE_OPERAND (arg0, 1)),
-			     TREE_TYPE (TREE_OPERAND (arg1, 1)))
-			  && (MR_DEPENDENCE_CLIQUE (arg0)
-			      == MR_DEPENDENCE_CLIQUE (arg1))
-			  && (MR_DEPENDENCE_BASE (arg0)
-			      == MR_DEPENDENCE_BASE (arg1))
-			  && (TYPE_ALIGN (TREE_TYPE (arg0))
-			    == TYPE_ALIGN (TREE_TYPE (arg1)))))))
-	    return 0;
+	  if (!(flags & OEP_ADDRESS_OF))
+	    {
+	      /* Require equal access sizes */
+	      if (TYPE_SIZE (TREE_TYPE (arg0)) != TYPE_SIZE (TREE_TYPE (arg1))
+		  && (!TYPE_SIZE (TREE_TYPE (arg0))
+		      || !TYPE_SIZE (TREE_TYPE (arg1))
+		      || !operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
+					   TYPE_SIZE (TREE_TYPE (arg1)),
+					   flags & ~OEP_CONSTANT_ADDRESS_OF)))
+		return 0;
+	      /* Verify that access happens in similar types.  */
+	      if (!types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1)))
+		return 0;
+	      /* Verify that accesses are TBAA compatible.  */
+	      if ((flag_strict_aliasing
+		   && !alias_ptr_types_compatible_p
+		        (TREE_TYPE (TREE_OPERAND (arg0, 1)),
+		         TREE_TYPE (TREE_OPERAND (arg1, 1))))
+		  || MR_DEPENDENCE_CLIQUE (arg0)
+		     != MR_DEPENDENCE_CLIQUE (arg1)
+		  || MR_DEPENDENCE_BASE (arg0)
+		     != MR_DEPENDENCE_BASE (arg1))
+		return 0;
+	    }
+	   /* Verify that alignment is compatible.  */
+	   if (TYPE_ALIGN (TREE_TYPE (arg0))
+	       != TYPE_ALIGN (TREE_TYPE (arg1)))
+	      return 0;
 	  flags &= ~(OEP_CONSTANT_ADDRESS_OF|OEP_ADDRESS_OF);
 	  return (OP_SAME (0) && OP_SAME (1)
 		  /* TARGET_MEM_REF require equal extra operands.  */