Nice fix! I noticed that this patch is already combined to the trunk.
Thank you very much, Richard!
Cong
On Tue, Sep 24, 2013 at 1:49 AM, Richard Biener <rguent...@suse.de> wrote:
> 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:
