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

Richard.


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