Do not compare types in operands_equal_p if OEP_ADDRESS_OF is set

Do not compare types in operands_equal_p if OEP_ADDRESS_OF is set

[Jan Hubicka]

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.
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.  */

Re: Do not compare types in operands_equal_p if OEP_ADDRESS_OF is set

[Richard Biener]

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.  */
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Do not compare types in operands_equal_p if OEP_ADDRESS_OF is set

[Eric Botcazou]

> 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.

-- 
Eric Botcazou

Re: Do not compare types in operands_equal_p if OEP_ADDRESS_OF is set

[Jan Hubicka]

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)

Re: Do not compare types in operands_equal_p if OEP_ADDRESS_OF is set

[Jan Hubicka]

> > 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.

Yep, I noticed it, but just after reading the sources for few times. The
use is well hidden :)

Here is updated patch adding Richard's feedback and also making most of
OEP_ADDRESS_OF special cases to also handle OEP_CONSTANT_ADDRESS_OF.
OEP_CONSTANT_ADDRESS_OF means we care about address and we know the whole
expr is constant, so it is stronger than OEP_ADDRESS_OF.

I also added documentation and cleaned up handling of ADDR_EXPR.  There are two
cases handing ADDR_EXPR.
One handles TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1) and the other the
rest of cases, so we do not need the conditional in the code quoted above.
This will hopefully make it more obvious when the OEP_CONSTANT_ADDRESS_OF
is set and used.

I also added sanity check that OEP_ADDRESSOF|OEP_CONSTANT_ADDRESS_OF is not
used for INTEGER_CST and NOP_EXPR.  There are many other cases where we can't
take address, but this seems strong enough to catch the wrong recursion which
forgets to clear the flag and forced me to fix the OEP_CONSTANT_ADDRESS_OF
handling.

I wonder if the INDIRECT_REF also needs the TBAA check that we do for MEM_REF?
While we lower that early, I think we can still unify the code like in case
of
  cond ? ref_alias_set_1 : ref_alias_set_2

Bootstrapped/regtested x86_64-linux, OK?

Honza

	* fold-const.c (operand_equal_p): Document OEP_ADDRESS_OF
	and OEP_CONSTANT_ADDRESS_OF; make most of OEP_ADDRESS_OF
	special cases to also handle OEP_CONSTANT_ADDRESS_OF; skip
	type checking for OPE_*ADDRESS_OF.
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 228131)
+++ fold-const.c	(working copy)
@@ -2691,7 +2691,12 @@ combine_comparisons (location_t loc,
 
    If OEP_PURE_SAME is set, then pure functions with identical arguments
    are considered the same.  It is used when the caller has other ways
-   to ensure that global memory is unchanged in between.  */
+   to ensure that global memory is unchanged in between.
+
+   If OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF is set, we are actually comparing
+   addresses of objects, not values of expressions.  OEP_CONSTANT_ADDRESS_OF
+   is used for ADDR_EXPR with TREE_CONSTANT flag set and we further ignore
+   any side effects on SAVE_EXPRs then.  */
 
 int
 operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
@@ -2710,31 +2715,48 @@ operand_equal_p (const_tree arg0, const_
   /* Check equality of integer constants before bailing out due to
      precision differences.  */
   if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
-    return tree_int_cst_equal (arg0, arg1);
+    {
+      /* Address of INTEGER_CST is not defined; check that we did not forget
+	 to drop the OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags.  */
+      gcc_checking_assert (!(flags
+			     & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)));
+      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 | OEP_CONSTANT_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);
+      STRIP_NOPS (arg0);
+      STRIP_NOPS (arg1);
+    }
+  else
+    /* Addresses of NOP_EXPR (and many other things) are not well defined. 
+       Check that we did not forget to drop the
+       OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags.  */
+    gcc_checking_assert (TREE_CODE (arg0) != NOP_EXPR
+			 && TREE_CODE (arg1) != NOP_EXPR);
 
   /* In case both args are comparisons but with different comparison
      code, try to swap the comparison operands of one arg to produce
@@ -2757,7 +2779,7 @@ operand_equal_p (const_tree arg0, const_
       /* NOP_EXPR and CONVERT_EXPR are considered equal.  */
       if (CONVERT_EXPR_P (arg0) && CONVERT_EXPR_P (arg1))
 	;
-      else if (flags & OEP_ADDRESS_OF)
+      else if (flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF))
 	{
 	  /* If we are interested in comparing addresses ignore
 	     MEM_REF wrappings of the base that can appear just for
@@ -2858,9 +2880,10 @@ operand_equal_p (const_tree arg0, const_
 			      TREE_STRING_LENGTH (arg0)));
 
       case ADDR_EXPR:
+	/* Be sure we pass right ADDRESS_OF flag.  */
+	gcc_checking_assert (TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1));
 	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);
+				OEP_CONSTANT_ADDRESS_OF);
       default:
 	break;
       }
@@ -2922,7 +2945,7 @@ operand_equal_p (const_tree arg0, const_
       switch (TREE_CODE (arg0))
 	{
 	case INDIRECT_REF:
-	  if (!(flags & OEP_ADDRESS_OF)
+	  if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF))
 	      && (TYPE_ALIGN (TREE_TYPE (arg0))
 		  != TYPE_ALIGN (TREE_TYPE (arg1))))
 	    return 0;
@@ -2935,27 +2958,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 | OEP_CONSTANT_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)))
+		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.  */
@@ -3001,6 +3031,8 @@ operand_equal_p (const_tree arg0, const_
       switch (TREE_CODE (arg0))
 	{
 	case ADDR_EXPR:
+	  /* Be sure we pass right ADDRESS_OF flag.  */
+	  gcc_checking_assert (!(TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1)));
 	  return operand_equal_p (TREE_OPERAND (arg0, 0),
 				  TREE_OPERAND (arg1, 0),
 				  flags | OEP_ADDRESS_OF);

Re: Do not compare types in operands_equal_p if OEP_ADDRESS_OF is set

[Richard Biener]

On Wed, 7 Oct 2015, Jan Hubicka wrote:

> > > 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.
> 
> Yep, I noticed it, but just after reading the sources for few times. The
> use is well hidden :)
> 
> Here is updated patch adding Richard's feedback and also making most of
> OEP_ADDRESS_OF special cases to also handle OEP_CONSTANT_ADDRESS_OF.
> OEP_CONSTANT_ADDRESS_OF means we care about address and we know the whole
> expr is constant, so it is stronger than OEP_ADDRESS_OF.

Yeah, we always set OEP_ADDRESS_OF when we set OEP_CONSTANT_ADDRESS_OF
though ...

> I also added documentation and cleaned up handling of ADDR_EXPR.  There are two
> cases handing ADDR_EXPR.
> One handles TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1) and the other the
> rest of cases, so we do not need the conditional in the code quoted above.
> This will hopefully make it more obvious when the OEP_CONSTANT_ADDRESS_OF
> is set and used.
> 
> I also added sanity check that OEP_ADDRESSOF|OEP_CONSTANT_ADDRESS_OF is not
> used for INTEGER_CST and NOP_EXPR.  There are many other cases where we can't
> take address, but this seems strong enough to catch the wrong recursion which
> forgets to clear the flag and forced me to fix the OEP_CONSTANT_ADDRESS_OF
> handling.
> 
> I wonder if the INDIRECT_REF also needs the TBAA check that we do for MEM_REF?
> While we lower that early, I think we can still unify the code like in case
> of
>   cond ? ref_alias_set_1 : ref_alias_set_2

I believe that INDIRECT_REFs have to be "type safe", that is,
TREE_TYPE (ref) == TREE_TYPE (TREE_TYPE (TREE_OPERAND (ref, 0))) and
thus in theory type checks should catch mismatches.  OTOH we only
have

  /* 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;

and thus even miss basic TYPE_SIZE checks here...  Thus

struct S { int i[3]; };
struct R { int i[7]; };

 cond ? *(struct S *)p : *(struct R *)p;

will probably unify (well, hopefully the FE will complain about
the size mismatch, but the INDIRECT_REFs will be operand_equal_p).

Then we also do STRIP_NOPS on the pointer types which will strip

typedef int myint __attribute__((may_alias));
int *p;

 cond ? *(myint *)p : *p;

So yeah, the INDIRECT_REF case looks very suspicious.

> Bootstrapped/regtested x86_64-linux, OK?

Few comments below

> Honza
> 
> 	* fold-const.c (operand_equal_p): Document OEP_ADDRESS_OF
> 	and OEP_CONSTANT_ADDRESS_OF; make most of OEP_ADDRESS_OF
> 	special cases to also handle OEP_CONSTANT_ADDRESS_OF; skip
> 	type checking for OPE_*ADDRESS_OF.
> Index: fold-const.c
> ===================================================================
> --- fold-const.c	(revision 228131)
> +++ fold-const.c	(working copy)
> @@ -2691,7 +2691,12 @@ combine_comparisons (location_t loc,
>  
>     If OEP_PURE_SAME is set, then pure functions with identical arguments
>     are considered the same.  It is used when the caller has other ways
> -   to ensure that global memory is unchanged in between.  */
> +   to ensure that global memory is unchanged in between.
> +
> +   If OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF is set, we are actually comparing
> +   addresses of objects, not values of expressions.  OEP_CONSTANT_ADDRESS_OF
> +   is used for ADDR_EXPR with TREE_CONSTANT flag set and we further ignore
> +   any side effects on SAVE_EXPRs then.  */
>  
>  int
>  operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
> @@ -2710,31 +2715,48 @@ operand_equal_p (const_tree arg0, const_
>    /* Check equality of integer constants before bailing out due to
>       precision differences.  */
>    if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
> -    return tree_int_cst_equal (arg0, arg1);
> +    {
> +      /* Address of INTEGER_CST is not defined; check that we did not forget
> +	 to drop the OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags.  */
> +      gcc_checking_assert (!(flags
> +			     & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)));
> +      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 | OEP_CONSTANT_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);
> +      STRIP_NOPS (arg0);
> +      STRIP_NOPS (arg1);
> +    }
> +  else
> +    /* Addresses of NOP_EXPR (and many other things) are not well defined. 
> +       Check that we did not forget to drop the
> +       OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags.  */
> +    gcc_checking_assert (TREE_CODE (arg0) != NOP_EXPR
> +			 && TREE_CODE (arg1) != NOP_EXPR);

Use ! CONVERT_EXPR_P (arg0) && ! CONVERT_EXPR_P (arg1)
 
>    /* In case both args are comparisons but with different comparison
>       code, try to swap the comparison operands of one arg to produce
> @@ -2757,7 +2779,7 @@ operand_equal_p (const_tree arg0, const_
>        /* NOP_EXPR and CONVERT_EXPR are considered equal.  */
>        if (CONVERT_EXPR_P (arg0) && CONVERT_EXPR_P (arg1))
>  	;
> -      else if (flags & OEP_ADDRESS_OF)
> +      else if (flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF))
>  	{
>  	  /* If we are interested in comparing addresses ignore
>  	     MEM_REF wrappings of the base that can appear just for
> @@ -2858,9 +2880,10 @@ operand_equal_p (const_tree arg0, const_
>  			      TREE_STRING_LENGTH (arg0)));
>  
>        case ADDR_EXPR:
> +	/* Be sure we pass right ADDRESS_OF flag.  */
> +	gcc_checking_assert (TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1));

Well, it's guarded with if (TREE_CONSTANT (arg0) && ... so the
assert is quite pointless.

>  	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);
> +				OEP_CONSTANT_ADDRESS_OF);
>        default:
>  	break;
>        }
> @@ -2922,7 +2945,7 @@ operand_equal_p (const_tree arg0, const_
>        switch (TREE_CODE (arg0))
>  	{
>  	case INDIRECT_REF:
> -	  if (!(flags & OEP_ADDRESS_OF)
> +	  if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF))
>  	      && (TYPE_ALIGN (TREE_TYPE (arg0))
>  		  != TYPE_ALIGN (TREE_TYPE (arg1))))
>  	    return 0;
> @@ -2935,27 +2958,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 | OEP_CONSTANT_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)))
> +		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.  */
> @@ -3001,6 +3031,8 @@ operand_equal_p (const_tree arg0, const_
>        switch (TREE_CODE (arg0))
>  	{
>  	case ADDR_EXPR:
> +	  /* Be sure we pass right ADDRESS_OF flag.  */
> +	  gcc_checking_assert (!(TREE_CONSTANT (arg0) && TREE_CONSTANT (arg1)));

Likewise.

Otherwise the patch looks ok.

Thanks,
Richard.

>  	  return operand_equal_p (TREE_OPERAND (arg0, 0),
>  				  TREE_OPERAND (arg1, 0),
>  				  flags | OEP_ADDRESS_OF);
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

[fortran] Re: Do not compare types in operands_equal_p if OEP_ADDRESS_OF is set

[Jan Hubicka]

> > -  STRIP_NOPS (arg0);
> > -  STRIP_NOPS (arg1);
> > +      STRIP_NOPS (arg0);
> > +      STRIP_NOPS (arg1);
> > +    }
> > +  else
> > +    /* Addresses of NOP_EXPR (and many other things) are not well defined. 
> > +       Check that we did not forget to drop the
> > +       OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags.  */
> > +    gcc_checking_assert (TREE_CODE (arg0) != NOP_EXPR
> > +			 && TREE_CODE (arg1) != NOP_EXPR);
> 
> Use ! CONVERT_EXPR_P (arg0) && ! CONVERT_EXPR_P (arg1)

Hi,
the x86_64 testing actually shows one regression at
gfortran.dg/coarray/coindexed_1.f90.  The problem is the new addrt that we do
not take an address of NOP_EXPR.  Fortran indeed does that:

 <addr_expr 0x7ffff6caa2e0
    type <pointer_type 0x7ffff6c9fdc8
        type <array_type 0x7ffff6c9fb28 type <integer_type 0x7ffff6adb540 character(kind=1)>
            string-flag BLK
            size <integer_cst 0x7ffff6c917b0 constant 56>
            unit size <integer_cst 0x7ffff6c91780 constant 7>
            align 8 symtab 0 alias set -1 canonical type 0x7ffff6c9fb28 domain <integer_type 0x7ffff6c9fa80>
            pointer_to_this <pointer_type 0x7ffff6c9fdc8>>
        unsigned DI
        size <integer_cst 0x7ffff6ad7bd0 constant 64>
        unit size <integer_cst 0x7ffff6ad7be8 constant 8>
        align 64 symtab 0 alias set -1 canonical type 0x7ffff6c9fdc8>
  
    arg 0 <nop_expr 0x7ffff6caa2a0 type <array_type 0x7ffff6c9fb28>
  
        arg 0 <var_decl 0x7ffff6ae1a20 str2a type <array_type 0x7ffff6c9fbd0>
            used static decl_0 BLK file /aux/hubicka/trunk-9/gcc/testsuite/gfortran.dg/coarray/coindexed_1.f90 line 10 col 0 size <integer_cst 0x7ffff6c917b0 56> unit size <integer_cst 0x7ffff6c91780 7>
            align 8 context <function_decl 0x7ffff6c97a80 char_test> chain <var_decl 0x7ffff6ae1990 str1b>>>


A NOP_EXPR truning one array type to another seems somewhat suspicious. 
Perhaps it should be VIEW_CONVERT_EXPR?  I tracked down that the NOP_EXPR is
introduced by:

 #3  0x000000000068c800 in gfc_conv_array_ref (se=se@entry=0x7fffffffe320, ar=ar@entry=0x1db8e78, expr=expr@entry=0x1db8da0, where=where@entry=0x1db8df0)
    at ../../gcc/fortran/trans-array.c:3299
3299                se->expr = fold_convert (TYPE_MAIN_VARIANT (TREE_TYPE (se->expr)),
(gdb) l
3294                  && TREE_CODE (TREE_TYPE (se->expr)) == POINTER_TYPE)
3295                se->expr = build_fold_indirect_ref_loc (input_location, se->expr);
3296
3297              /* Use the actual tree type and not the wrapped coarray.  */
3298              if (!se->want_pointer)
3299                se->expr = fold_convert (TYPE_MAIN_VARIANT (TREE_TYPE (se->expr)),
3300                                         se->expr);
3301            }

So it seems to be fuly concious decision to add the conversion.
I suppose this may make sense to the frontend trees.  It seems resonable to
however drop the NOP_EXPR before building ADDR_EXPR as follows. Calling
get_base_address on something that contains a NOP_EXPR in address is also
a bad idea.

Bootstrapping/regtesting x86_64-linux, seems sane?

Honza


	* trans.c (gfc_build_addr_expr): Do not build ADDR_EXPR of NOP_EXPR.
Index: trans.c
===================================================================
--- trans.c	(revision 228582)
+++ trans.c	(working copy)
@@ -297,6 +297,7 @@ gfc_build_addr_expr (tree type, tree t)
     }
   else
     {
+      STRIP_NOPS (t);
       tree base = get_base_address (t);
       if (base && DECL_P (base))
         TREE_ADDRESSABLE (base) = 1;

Re: [fortran] Re: Do not compare types in operands_equal_p if OEP_ADDRESS_OF is set

[Richard Biener]

On Thu, 8 Oct 2015, Jan Hubicka wrote:

> > > -  STRIP_NOPS (arg0);
> > > -  STRIP_NOPS (arg1);
> > > +      STRIP_NOPS (arg0);
> > > +      STRIP_NOPS (arg1);
> > > +    }
> > > +  else
> > > +    /* Addresses of NOP_EXPR (and many other things) are not well defined. 
> > > +       Check that we did not forget to drop the
> > > +       OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags.  */
> > > +    gcc_checking_assert (TREE_CODE (arg0) != NOP_EXPR
> > > +			 && TREE_CODE (arg1) != NOP_EXPR);
> > 
> > Use ! CONVERT_EXPR_P (arg0) && ! CONVERT_EXPR_P (arg1)
> 
> Hi,
> the x86_64 testing actually shows one regression at
> gfortran.dg/coarray/coindexed_1.f90.  The problem is the new addrt that we do
> not take an address of NOP_EXPR.  Fortran indeed does that:
> 
>  <addr_expr 0x7ffff6caa2e0
>     type <pointer_type 0x7ffff6c9fdc8
>         type <array_type 0x7ffff6c9fb28 type <integer_type 0x7ffff6adb540 character(kind=1)>
>             string-flag BLK
>             size <integer_cst 0x7ffff6c917b0 constant 56>
>             unit size <integer_cst 0x7ffff6c91780 constant 7>
>             align 8 symtab 0 alias set -1 canonical type 0x7ffff6c9fb28 domain <integer_type 0x7ffff6c9fa80>
>             pointer_to_this <pointer_type 0x7ffff6c9fdc8>>
>         unsigned DI
>         size <integer_cst 0x7ffff6ad7bd0 constant 64>
>         unit size <integer_cst 0x7ffff6ad7be8 constant 8>
>         align 64 symtab 0 alias set -1 canonical type 0x7ffff6c9fdc8>
>   
>     arg 0 <nop_expr 0x7ffff6caa2a0 type <array_type 0x7ffff6c9fb28>
>   
>         arg 0 <var_decl 0x7ffff6ae1a20 str2a type <array_type 0x7ffff6c9fbd0>
>             used static decl_0 BLK file /aux/hubicka/trunk-9/gcc/testsuite/gfortran.dg/coarray/coindexed_1.f90 line 10 col 0 size <integer_cst 0x7ffff6c917b0 56> unit size <integer_cst 0x7ffff6c91780 7>
>             align 8 context <function_decl 0x7ffff6c97a80 char_test> chain <var_decl 0x7ffff6ae1990 str1b>>>
> 
> 
> A NOP_EXPR truning one array type to another seems somewhat suspicious. 
> Perhaps it should be VIEW_CONVERT_EXPR?  I tracked down that the NOP_EXPR is
> introduced by:
> 
>  #3  0x000000000068c800 in gfc_conv_array_ref (se=se@entry=0x7fffffffe320, ar=ar@entry=0x1db8e78, expr=expr@entry=0x1db8da0, where=where@entry=0x1db8df0)
>     at ../../gcc/fortran/trans-array.c:3299
> 3299                se->expr = fold_convert (TYPE_MAIN_VARIANT (TREE_TYPE (se->expr)),
> (gdb) l
> 3294                  && TREE_CODE (TREE_TYPE (se->expr)) == POINTER_TYPE)
> 3295                se->expr = build_fold_indirect_ref_loc (input_location, se->expr);
> 3296
> 3297              /* Use the actual tree type and not the wrapped coarray.  */
> 3298              if (!se->want_pointer)
> 3299                se->expr = fold_convert (TYPE_MAIN_VARIANT (TREE_TYPE (se->expr)),
> 3300                                         se->expr);
> 3301            }
> 
> So it seems to be fuly concious decision to add the conversion.
> I suppose this may make sense to the frontend trees.  It seems resonable to
> however drop the NOP_EXPR before building ADDR_EXPR as follows. Calling
> get_base_address on something that contains a NOP_EXPR in address is also
> a bad idea.
> 
> Bootstrapping/regtesting x86_64-linux, seems sane?

Hmm, I'd rather not use NOP_EXPR on aggregate types.  I also see no reason
to have NOPs to a main variant.  It's btw the only NOP we allow on 
aggregates in fold_convert...

Using a VIEW_CONVERT_EXPR would be cleaner IMHO.

Of course your patch also works and I'll leave the decision to the
frontend maintainers.

Thanks,
Richard.

> Honza
> 
> 
> 	* trans.c (gfc_build_addr_expr): Do not build ADDR_EXPR of NOP_EXPR.
> Index: trans.c
> ===================================================================
> --- trans.c	(revision 228582)
> +++ trans.c	(working copy)
> @@ -297,6 +297,7 @@ gfc_build_addr_expr (tree type, tree t)
>      }
>    else
>      {
> +      STRIP_NOPS (t);
>        tree base = get_base_address (t);
>        if (base && DECL_P (base))
>          TREE_ADDRESSABLE (base) = 1;
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: Do not compare types in operands_equal_p if OEP_ADDRESS_OF is set

[Jan Hubicka]

Hi,
this is final version of patch I commited.  I applied the changes suggested and 
droped the redundant OEP_CONSTANT_ADDRESS_OF & updated documentation that the
flag is supposed to be additive to OEP_ADDRESS_OF

I also disabled the sanity check about NOP_EXPR inside ADDR_EXPR until
we resolve the Fortran FE issue. 

	* fold-const.c (operand_equal_p): Document OEP_ADDRESS_OF
	and OEP_CONSTANT_ADDRESS_OF; skip type compatibility checks
	when OEP_ADDRESS_OF is se.
Index: fold-const.c
===================================================================
--- fold-const.c	(revision 228625)
+++ fold-const.c	(working copy)
@@ -2693,7 +2693,12 @@ combine_comparisons (location_t loc,
 
    If OEP_PURE_SAME is set, then pure functions with identical arguments
    are considered the same.  It is used when the caller has other ways
-   to ensure that global memory is unchanged in between.  */
+   to ensure that global memory is unchanged in between.
+
+   If OEP_ADDRESS_OF is set, we are actually comparing addresses of objects,
+   not values of expressions.  OEP_CONSTANT_ADDRESS_OF in addition to
+   OEP_ADDRESS_OF is used for ADDR_EXPR with TREE_CONSTANT flag set and we
+   further ignore any side effects on SAVE_EXPRs then.  */
 
 int
 operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
@@ -2712,31 +2717,52 @@ operand_equal_p (const_tree arg0, const_
   /* Check equality of integer constants before bailing out due to
      precision differences.  */
   if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST)
-    return tree_int_cst_equal (arg0, arg1);
+    {
+      /* Address of INTEGER_CST is not defined; check that we did not forget
+	 to drop the OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags.  */
+      gcc_checking_assert (!(flags
+			     & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)));
+      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);
+      STRIP_NOPS (arg0);
+      STRIP_NOPS (arg1);
+    }
+#if 0
+  /* FIXME: Fortran FE currently produce ADDR_EXPR of NOP_EXPR. Enable the
+     sanity check once the issue is solved.  */
+  else
+    /* Addresses of conversions and SSA_NAMEs (and many other things)
+       are not defined.  Check that we did not forget to drop the
+       OEP_ADDRESS_OF/OEP_CONSTANT_ADDRESS_OF flags.  */
+    gcc_checking_assert (!CONVERT_EXPR_P (arg0) && !CONVERT_EXPR_P (arg1)
+			 && TREE_CODE (arg0) != SSA_NAME);
+#endif
 
   /* In case both args are comparisons but with different comparison
      code, try to swap the comparison operands of one arg to produce
@@ -2859,9 +2885,11 @@ operand_equal_p (const_tree arg0, const_
 			      TREE_STRING_LENGTH (arg0)));
 
       case ADDR_EXPR:
+	gcc_checking_assert (!(flags
+			       & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF)));
 	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);
+				flags | OEP_ADDRESS_OF
+				| OEP_CONSTANT_ADDRESS_OF);
       default:
 	break;
       }
@@ -2923,7 +2951,7 @@ operand_equal_p (const_tree arg0, const_
       switch (TREE_CODE (arg0))
 	{
 	case INDIRECT_REF:
-	  if (!(flags & OEP_ADDRESS_OF)
+	  if (!(flags & (OEP_ADDRESS_OF | OEP_CONSTANT_ADDRESS_OF))
 	      && (TYPE_ALIGN (TREE_TYPE (arg0))
 		  != TYPE_ALIGN (TREE_TYPE (arg1))))
 	    return 0;
@@ -2936,27 +2964,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 | OEP_CONSTANT_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)))
+		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.  */
@@ -3002,6 +3037,10 @@ operand_equal_p (const_tree arg0, const_
       switch (TREE_CODE (arg0))
 	{
 	case ADDR_EXPR:
+	  /* Be sure we pass right ADDRESS_OF flag.  */
+	  gcc_checking_assert (!(flags
+				 & (OEP_ADDRESS_OF
+				    | OEP_CONSTANT_ADDRESS_OF)));
 	  return operand_equal_p (TREE_OPERAND (arg0, 0),
 				  TREE_OPERAND (arg1, 0),
 				  flags | OEP_ADDRESS_OF);