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: [PATCH] Bug fix: *var and MEM[(const int *)var] (var has int* type) are not treated as the same data ref.


On Tue, 24 Sep 2013, Richard Biener wrote:

> On Tue, 24 Sep 2013, Jakub Jelinek wrote:
> 
> > Hi!
> > 
> > On Mon, Sep 23, 2013 at 05:26:13PM -0700, Cong Hou wrote:
> > 
> > Missing ChangeLog entry.
> > 
> > > --- gcc/testsuite/gcc.dg/alias-14.c (revision 0)
> > > +++ gcc/testsuite/gcc.dg/alias-14.c (revision 0)
> > 
> > Vectorizer tests should go into gcc.dg/vect/ instead, or, if they are
> > for a single target (but there is no reason why this should be a single
> > target), into gcc.target/<target>/.
> > 
> > > --- gcc/fold-const.c (revision 202662)
> > > +++ gcc/fold-const.c (working copy)
> > > @@ -2693,8 +2693,9 @@ operand_equal_p (const_tree arg0, const_
> > >         && operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
> > >     TYPE_SIZE (TREE_TYPE (arg1)), flags)))
> > >    && types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1))
> > > -  && (TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg0, 1)))
> > > -      == TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg1, 1))))
> > > +  && types_compatible_p (
> > > +       TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg0, 1))),
> > > +       TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg1, 1))))
> > >    && OP_SAME (0) && OP_SAME (1));
> > 
> > This looks wrong.  types_compatible_p will happily return true say
> > for unsigned long and unsigned long long types on x86_64, because
> > they are both integral types with the same precision, but the second
> > argument of MEM_REF contains aliasing information, where the distinction
> > between the two is important.
> > So, while == comparison of main variant is too strict, types_compatible_p
> > is too weak, so I guess you need to write a new predicate that will either
> > handle the == and a few special cases that are safe to be handled, or
> > look for what exactly we use the type of the second MEM_REF argument
> > and check those properties.  We certainly need that
> > get_deref_alias_set_1 and get_deref_alias_set return the same values
> > for both the types, but whether that is the only information we are using,
> > not sure, CCing Richard.
> 
> Using TYPE_MAIN_VARIANT is exactly correct - this is the best we
> can do that will work with all frontends.  TYPE_MAIN_VARIANT
> guarantees that the alias-sets stay the same:
> 
>       /* If the innermost reference is a MEM_REF that has a
>          conversion embedded treat it like a VIEW_CONVERT_EXPR above,
>          using the memory access type for determining the alias-set.  */
>      if (TREE_CODE (inner) == MEM_REF
>          && TYPE_MAIN_VARIANT (TREE_TYPE (inner))
>             != TYPE_MAIN_VARIANT
>                (TREE_TYPE (TREE_TYPE (TREE_OPERAND (inner, 1)))))
>        return get_deref_alias_set (TREE_OPERAND (inner, 1));
> 
> so we cannot change the compatibility checks without touching the
> alias-set deriving code.  For the testcase in question we have
> MEM[(const int &)_7] vs. MEM[(int *)_7] and unfortunately pointer
> and reference types are not variant types.
> 
> We also cannot easily resort to pointed-to types as our all-beloved
> ref-all qualification is on the pointer type rather than on the
> pointed-to type.
> 
> But yes, we could implement a more complicated predicate
> 
> bool
> alias_ptr_types_compatible_p (const_tree t1, const_tree t2)
> {
>   t1 = TYPE_MAIN_VARIANT (t1);
>   t2 = TYPE_MAIN_VARIANT (t2);
>   if (t1 == t2)
>     return true;
> 
>   if (TYPE_REF_CAN_ALIAS_ALL (t1)
>       || TYPE_REF_CAN_ALIAS_ALL (t2))
>     return false;
> 
>   return (TYPE_MAIN_VARIANT (TREE_TYPE (t1))
>           == TYPE_MAIN_VARIANT (TREE_TYPE (t2)));
> }
> 
> Note that the fold-const code in question is
> 
>           return ((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))
>                   && (TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg0, 
> 1)))
>                       == TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg1, 
> 1))))
>                   && OP_SAME (0) && OP_SAME (1));
> 
> which you may notice uses types_compatible_p on the reference type
> which is at least suspicious as that can let through reference trees
> that will end up using different alias sets due to the stricter
> check in get_alias_set.
> 
> So in addition to alias_ptr_types_compatible_p we may want to have
> 
> bool
> reference_alias_ptr_types_compatible_p (const_tree t1, const_tree t2)
> {
> ...
> }
> 
> abstracting this away for the actual reference trees (also noting
> that reference_alias_ptr_type isn't 1:1 following what get_alias_set
> does either).
> 
> I will give this a try.

The following is an untested (well, the testcase from PR58513 is now
vectorized) patch doing that refactoring.

Richard.

Index: gcc/tree.c
===================================================================
*** gcc/tree.c	(revision 202859)
--- gcc/tree.c	(working copy)
*************** mem_ref_offset (const_tree t)
*** 4263,4286 ****
    return tree_to_double_int (toff).sext (TYPE_PRECISION (TREE_TYPE (toff)));
  }
  
- /* Return the pointer-type relevant for TBAA purposes from the
-    gimple memory reference tree T.  This is the type to be used for
-    the offset operand of MEM_REF or TARGET_MEM_REF replacements of T.  */
- 
- tree
- reference_alias_ptr_type (const_tree t)
- {
-   const_tree base = t;
-   while (handled_component_p (base))
-     base = TREE_OPERAND (base, 0);
-   if (TREE_CODE (base) == MEM_REF)
-     return TREE_TYPE (TREE_OPERAND (base, 1));
-   else if (TREE_CODE (base) == TARGET_MEM_REF)
-     return TREE_TYPE (TMR_OFFSET (base)); 
-   else
-     return build_pointer_type (TYPE_MAIN_VARIANT (TREE_TYPE (base)));
- }
- 
  /* Return an invariant ADDR_EXPR of type TYPE taking the address of BASE
     offsetted by OFFSET units.  */
  
--- 4263,4268 ----
Index: gcc/tree.h
===================================================================
*** gcc/tree.h	(revision 202859)
--- gcc/tree.h	(working copy)
*************** extern tree build_simple_mem_ref_loc (lo
*** 4345,4351 ****
  #define build_simple_mem_ref(T)\
  	build_simple_mem_ref_loc (UNKNOWN_LOCATION, T)
  extern double_int mem_ref_offset (const_tree);
- extern tree reference_alias_ptr_type (const_tree);
  extern tree build_invariant_address (tree, tree, HOST_WIDE_INT);
  extern tree constant_boolean_node (bool, tree);
  extern tree div_if_zero_remainder (enum tree_code, const_tree, const_tree);
--- 4345,4350 ----
Index: gcc/alias.c
===================================================================
*** gcc/alias.c	(revision 202859)
--- gcc/alias.c	(working copy)
*************** component_uses_parent_alias_set (const_t
*** 547,552 ****
--- 547,564 ----
      }
  }
  
+ 
+ /* Return whether the pointer-type T effective for aliasing may
+    access everything and thus the reference has to be assigned
+    alias-set zero.  */
+ 
+ static bool
+ ref_all_alias_ptr_type_p (const_tree t)
+ {
+   return (TREE_CODE (TREE_TYPE (t)) == VOID_TYPE
+ 	  || TYPE_REF_CAN_ALIAS_ALL (t));
+ }
+ 
  /* Return the alias set for the memory pointed to by T, which may be
     either a type or an expression.  Return -1 if there is nothing
     special about dereferencing T.  */
*************** component_uses_parent_alias_set (const_t
*** 554,564 ****
  static alias_set_type
  get_deref_alias_set_1 (tree t)
  {
-   /* If we're not doing any alias analysis, just assume everything
-      aliases everything else.  */
-   if (!flag_strict_aliasing)
-     return 0;
- 
    /* All we care about is the type.  */
    if (! TYPE_P (t))
      t = TREE_TYPE (t);
--- 566,571 ----
*************** get_deref_alias_set_1 (tree t)
*** 566,573 ****
    /* If we have an INDIRECT_REF via a void pointer, we don't
       know anything about what that might alias.  Likewise if the
       pointer is marked that way.  */
!   if (TREE_CODE (TREE_TYPE (t)) == VOID_TYPE
!       || TYPE_REF_CAN_ALIAS_ALL (t))
      return 0;
  
    return -1;
--- 573,579 ----
    /* If we have an INDIRECT_REF via a void pointer, we don't
       know anything about what that might alias.  Likewise if the
       pointer is marked that way.  */
!   if (ref_all_alias_ptr_type_p (t))
      return 0;
  
    return -1;
*************** get_deref_alias_set_1 (tree t)
*** 579,584 ****
--- 585,595 ----
  alias_set_type
  get_deref_alias_set (tree t)
  {
+   /* If we're not doing any alias analysis, just assume everything
+      aliases everything else.  */
+   if (!flag_strict_aliasing)
+     return 0;
+ 
    alias_set_type set = get_deref_alias_set_1 (t);
  
    /* Fall back to the alias-set of the pointed-to type.  */
*************** get_deref_alias_set (tree t)
*** 592,597 ****
--- 603,698 ----
    return set;
  }
  
+ static tree
+ reference_alias_ptr_type_1 (tree *t)
+ {
+   tree inner;
+ 
+   /* Get the base object of the reference.  */
+   inner = *t;
+   while (handled_component_p (inner))
+     {
+       /* If there is a VIEW_CONVERT_EXPR in the chain we cannot use
+ 	 the type of any component references that wrap it to
+ 	 determine the alias-set.  */
+       if (TREE_CODE (inner) == VIEW_CONVERT_EXPR)
+ 	*t = TREE_OPERAND (inner, 0);
+       inner = TREE_OPERAND (inner, 0);
+     }
+ 
+   /* Handle pointer dereferences here, they can override the
+      alias-set.  */
+   if (INDIRECT_REF_P (inner)
+       && ref_all_alias_ptr_type_p (TREE_TYPE (TREE_OPERAND (inner, 0))))
+     return TREE_TYPE (TREE_OPERAND (inner, 0));
+   else if (TREE_CODE (inner) == TARGET_MEM_REF)
+     return TREE_TYPE (TMR_OFFSET (inner));
+   else if (TREE_CODE (inner) == MEM_REF
+ 	   && ref_all_alias_ptr_type_p (TREE_TYPE (TREE_OPERAND (inner, 1))))
+     return TREE_TYPE (TREE_OPERAND (inner, 1));
+ 
+   /* If the innermost reference is a MEM_REF that has a
+      conversion embedded treat it like a VIEW_CONVERT_EXPR above,
+      using the memory access type for determining the alias-set.  */
+   if (TREE_CODE (inner) == MEM_REF
+       && (TYPE_MAIN_VARIANT (TREE_TYPE (inner))
+ 	  != TYPE_MAIN_VARIANT
+ 	       (TREE_TYPE (TREE_TYPE (TREE_OPERAND (inner, 1))))))
+     return TREE_TYPE (TREE_OPERAND (inner, 1));
+ 
+   /* Otherwise, pick up the outermost object that we could have a pointer
+      to, processing conversions as above.  */
+   /* ???  Ick, this is worse than quadratic!  */
+   while (component_uses_parent_alias_set (*t))
+     {
+       *t = TREE_OPERAND (*t, 0);
+       STRIP_NOPS (*t);
+     }
+ 
+   return NULL_TREE;
+ }
+ 
+ /* Return the pointer-type relevant for TBAA purposes from the
+    gimple memory reference tree T.  This is the type to be used for
+    the offset operand of MEM_REF or TARGET_MEM_REF replacements of T
+    and guarantees that get_alias_set will return the same alias
+    set for T and the replacement.  */
+ 
+ tree
+ reference_alias_ptr_type (tree t)
+ {
+   tree ptype = reference_alias_ptr_type_1 (&t);
+   /* If there is a given pointer type for aliasing purposes, return it.  */
+   if (ptype != NULL_TREE)
+     return ptype;
+ 
+   /* Otherwise build one from the outermost component reference we
+      may use.  */
+   if (TREE_CODE (t) == MEM_REF
+       || TREE_CODE (t) == TARGET_MEM_REF)
+     return TREE_TYPE (TREE_OPERAND (t, 1));
+   else
+     return build_pointer_type (TYPE_MAIN_VARIANT (TREE_TYPE (t)));
+ }
+ 
+ /* Return whether the pointer-types T1 and T2 used to determine
+    two alias sets of two references will yield the same answer
+    from get_deref_alias_set.  */
+ 
+ bool
+ alias_ptr_types_compatible_p (tree t1, tree t2)
+ {
+   if (TYPE_MAIN_VARIANT (t1) == TYPE_MAIN_VARIANT (t2))
+     return true;
+ 
+   if (ref_all_alias_ptr_type_p (t1)
+       || ref_all_alias_ptr_type_p (t2))
+     return false;
+ 
+   return (TYPE_MAIN_VARIANT (TREE_TYPE (t1))
+ 	  == TYPE_MAIN_VARIANT (TREE_TYPE (t2)));
+ }
+ 
  /* Return the alias set for T, which may be either a type or an
     expression.  Call language-specific routine for help, if needed.  */
  
*************** get_alias_set (tree t)
*** 615,622 ****
       aren't types.  */
    if (! TYPE_P (t))
      {
-       tree inner;
- 
        /* Give the language a chance to do something with this tree
  	 before we look at it.  */
        STRIP_NOPS (t);
--- 716,721 ----
*************** get_alias_set (tree t)
*** 624,674 ****
        if (set != -1)
  	return set;
  
!       /* Get the base object of the reference.  */
!       inner = t;
!       while (handled_component_p (inner))
! 	{
! 	  /* If there is a VIEW_CONVERT_EXPR in the chain we cannot use
! 	     the type of any component references that wrap it to
! 	     determine the alias-set.  */
! 	  if (TREE_CODE (inner) == VIEW_CONVERT_EXPR)
! 	    t = TREE_OPERAND (inner, 0);
! 	  inner = TREE_OPERAND (inner, 0);
! 	}
! 
!       /* Handle pointer dereferences here, they can override the
! 	 alias-set.  */
!       if (INDIRECT_REF_P (inner))
! 	{
! 	  set = get_deref_alias_set_1 (TREE_OPERAND (inner, 0));
! 	  if (set != -1)
! 	    return set;
! 	}
!       else if (TREE_CODE (inner) == TARGET_MEM_REF)
! 	return get_deref_alias_set (TMR_OFFSET (inner));
!       else if (TREE_CODE (inner) == MEM_REF)
! 	{
! 	  set = get_deref_alias_set_1 (TREE_OPERAND (inner, 1));
! 	  if (set != -1)
! 	    return set;
! 	}
! 
!       /* If the innermost reference is a MEM_REF that has a
! 	 conversion embedded treat it like a VIEW_CONVERT_EXPR above,
! 	 using the memory access type for determining the alias-set.  */
!      if (TREE_CODE (inner) == MEM_REF
! 	 && TYPE_MAIN_VARIANT (TREE_TYPE (inner))
! 	    != TYPE_MAIN_VARIANT
! 	       (TREE_TYPE (TREE_TYPE (TREE_OPERAND (inner, 1)))))
!        return get_deref_alias_set (TREE_OPERAND (inner, 1));
! 
!       /* Otherwise, pick up the outermost object that we could have a pointer
! 	 to, processing conversions as above.  */
!       while (component_uses_parent_alias_set (t))
! 	{
! 	  t = TREE_OPERAND (t, 0);
! 	  STRIP_NOPS (t);
! 	}
  
        /* If we've already determined the alias set for a decl, just return
  	 it.  This is necessary for C++ anonymous unions, whose component
--- 723,733 ----
        if (set != -1)
  	return set;
  
!       /* Get the alias pointer-type to use or the outermost object
!          that we could have a pointer to.  */
!       tree ptype = reference_alias_ptr_type_1 (&t);
!       if (ptype != NULL)
! 	return get_deref_alias_set (ptype);
  
        /* If we've already determined the alias set for a decl, just return
  	 it.  This is necessary for C++ anonymous unions, whose component
Index: gcc/alias.h
===================================================================
*** gcc/alias.h	(revision 202859)
--- gcc/alias.h	(working copy)
*************** extern int alias_sets_conflict_p (alias_
*** 41,46 ****
--- 41,48 ----
  extern int alias_sets_must_conflict_p (alias_set_type, alias_set_type);
  extern int objects_must_conflict_p (tree, tree);
  extern int nonoverlapping_memrefs_p (const_rtx, const_rtx, bool);
+ tree reference_alias_ptr_type (tree);
+ bool alias_ptr_types_compatible_p (tree, tree);
  
  /* This alias set can be used to force a memory to conflict with all
     other memories, creating a barrier across which no memory reference
Index: gcc/fold-const.c
===================================================================
*** gcc/fold-const.c	(revision 202859)
--- gcc/fold-const.c	(working copy)
*************** operand_equal_p (const_tree arg0, const_
*** 2693,2700 ****
  		       && operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
  					   TYPE_SIZE (TREE_TYPE (arg1)), flags)))
  		  && types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1))
! 		  && (TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg0, 1)))
! 		      == TYPE_MAIN_VARIANT (TREE_TYPE (TREE_OPERAND (arg1, 1))))
  		  && OP_SAME (0) && OP_SAME (1));
  
  	case ARRAY_REF:
--- 2693,2701 ----
  		       && operand_equal_p (TYPE_SIZE (TREE_TYPE (arg0)),
  					   TYPE_SIZE (TREE_TYPE (arg1)), flags)))
  		  && types_compatible_p (TREE_TYPE (arg0), TREE_TYPE (arg1))
! 		  && alias_ptr_types_compatible_p
! 		       (TREE_TYPE (TREE_OPERAND (arg0, 1)),
! 			TREE_TYPE (TREE_OPERAND (arg1, 1)))
  		  && OP_SAME (0) && OP_SAME (1));
  
  	case ARRAY_REF:


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