Re: [PATCH] Fix the constant mess in PRE

["Daniel Berlin" <dberlin at dberlin dot org>]

In any case, if your patch works without the other workarounds i had,
i'd rather use your patch, since it would be nice to go back to
constants being their own reps.

On Wed, Jul 16, 2008 at 3:27 PM, Daniel Berlin <dberlin@dberlin.org> wrote:
> I already applied a similar patch on mainline yesterday, with even
> more sharing guards (because it turns out we share in more places we
> shouldn't)..
>
> Using constants as their own reps is fine by me, it would simplify the
> code in fully_constant_expression.
>
>
> On Wed, Jul 16, 2008 at 9:42 AM, Richard Guenther <rguenther@suse.de> wrote:
>>
>> I debugged in detail the tuples branch miscompile of cp/class.c today
>> (see the reduced testcase in the patch) and came to the conclusion
>> that it won't ever work reliably to share expressions for equally
>> valued constants.  So the following patch breaks the sharing to
>> only share constants if they are of compatible type.
>>
>> I have verified this fixes the tuples bootstrap failure with
>>
>> 2008-07-16  Richard Guenther  <rguenther@suse.de>
>>
>>        * tree-ssa-pre.c (get_constant_for_value_id): Only hand out
>>        constants of the correct type.
>>        (fully_constant_expression): Pass the required type to
>>        get_constant_for_value_id.
>>
>> reverted.  The patch should apply to the trunk as well, and I'll check
>> if we can revert some of the previous workarounds there.
>>
>> The patch is still in testing (fingers crossing ;)).
>>
>> Danny, does this look ok?  Was there a reason to not use constants
>> as their representative?
>>
>> Thanks,
>> Richard.
>>
>> 2008-07-16  Richard Guenther  <rguenther@suse.de>
>>
>>        * tree-ssa-sccvn.h (vn_hash_constant_with_type): New function.
>>        (vn_constant_eq_with_type): Likewise.
>>        * tree-ssa-sccvn.c (vn_constant_eq): Use vn_constant_eq_with_type.
>>        (get_or_alloc_constant_value_id): Use vn_hash_constant_with_type.
>>        * tree-ssa-pre.c (pre_expr_eq): Use vn_constant_eq_with_type.
>>        (pre_expr_hash): Use vn_hash_constant_with_type.
>>        (get_representative_for): Use constants as their representative.
>>
>>        * gcc.dg/torture/20080716-1.c: New testcase.
>>
>> Index: tree-ssa-sccvn.h
>> ===================================================================
>> *** tree-ssa-sccvn.h    (revision 137870)
>> --- tree-ssa-sccvn.h    (working copy)
>> *************** typedef struct vn_constant_s
>> *** 104,110 ****
>>    hashval_t hashcode;
>>    tree constant;
>>  } *vn_constant_t;
>> !
>>  typedef struct vn_ssa_aux
>>  {
>>    /* Value number. This may be an SSA name or a constant.  */
>> --- 104,136 ----
>>    hashval_t hashcode;
>>    tree constant;
>>  } *vn_constant_t;
>> !
>> ! /* Hash the constant CONSTANT with distinguishing type incompatible
>> !    constants in the types_compatible_p sense.  */
>> !
>> ! static inline hashval_t
>> ! vn_hash_constant_with_type (tree constant)
>> ! {
>> !   tree type = TREE_TYPE (constant);
>> !   return (iterative_hash_expr (constant, 0)
>> !         + (INTEGRAL_TYPE_P (type)
>> !            ? TYPE_PRECISION (type) + TYPE_UNSIGNED (type) : 0));
>> ! }
>> !
>> ! /* Compare the constants C1 and C2 with distinguishing type incompatible
>> !    constants in the types_compatible_p sense.  */
>> !
>> ! static inline bool
>> ! vn_constant_eq_with_type (tree c1, tree c2)
>> ! {
>> !   tree type1 = TREE_TYPE (c1);
>> !   tree type2 = TREE_TYPE (c2);
>> !   return (expressions_equal_p (c1, c2)
>> !         && (!INTEGRAL_TYPE_P (type1)
>> !             || ((TYPE_PRECISION (type1) == TYPE_PRECISION (type2)
>> !                 && (TYPE_UNSIGNED (type1) == TYPE_UNSIGNED (type2))))));
>> ! }
>> !
>>  typedef struct vn_ssa_aux
>>  {
>>    /* Value number. This may be an SSA name or a constant.  */
>> Index: tree-ssa-sccvn.c
>> ===================================================================
>> *** tree-ssa-sccvn.c    (revision 137870)
>> --- tree-ssa-sccvn.c    (working copy)
>> *************** vn_constant_eq (const void *p1, const vo
>> *** 316,322 ****
>>    const struct vn_constant_s *vc1 = (const struct vn_constant_s *) p1;
>>    const struct vn_constant_s *vc2 = (const struct vn_constant_s *) p2;
>>
>> !   return expressions_equal_p (vc1->constant, vc2->constant);
>>  }
>>
>>  /* Hash table hash function for vn_constant_t.  */
>> --- 316,322 ----
>>    const struct vn_constant_s *vc1 = (const struct vn_constant_s *) p1;
>>    const struct vn_constant_s *vc2 = (const struct vn_constant_s *) p2;
>>
>> !   return vn_constant_eq_with_type (vc1->constant, vc2->constant);
>>  }
>>
>>  /* Hash table hash function for vn_constant_t.  */
>> *************** get_or_alloc_constant_value_id (tree con
>> *** 337,343 ****
>>    void **slot;
>>    vn_constant_t vc = XNEW (struct vn_constant_s);
>>
>> !   vc->hashcode = iterative_hash_expr (constant, 0);
>>    vc->constant = constant;
>>    slot = htab_find_slot_with_hash (constant_to_value_id, vc,
>>                                   vc->hashcode, INSERT);
>> --- 337,343 ----
>>    void **slot;
>>    vn_constant_t vc = XNEW (struct vn_constant_s);
>>
>> !   vc->hashcode = vn_hash_constant_with_type (constant);
>>    vc->constant = constant;
>>    slot = htab_find_slot_with_hash (constant_to_value_id, vc,
>>                                   vc->hashcode, INSERT);
>> Index: testsuite/gcc.dg/torture/20080716-1.c
>> ===================================================================
>> *** testsuite/gcc.dg/torture/20080716-1.c       (revision 0)
>> --- testsuite/gcc.dg/torture/20080716-1.c       (revision 0)
>> ***************
>> *** 0 ****
>> --- 1,58 ----
>> + /* { dg-do run } */
>> + /* { dg-require-effective-target lp64 } */
>> +
>> + typedef unsigned long size_t;
>> + struct tree_base
>> + {
>> +   int code;
>> + };
>> + struct tree_decl_minimal
>> + {
>> +   struct tree_base base;
>> +   const char *name;
>> + };
>> + typedef union tree_node {
>> +   struct tree_base base;
>> +   struct tree_decl_minimal decl_minimal;
>> + } *tree;
>> + struct tree_overload
>> + {
>> +   struct tree_base common;
>> +   tree function;
>> + };
>> + typedef struct VEC_tree_base { unsigned num; unsigned alloc; tree vec[1]; } VEC_tree_base;
>> + typedef struct VEC_tree_gc { VEC_tree_base base; } VEC_tree_gc;
>> + static __inline__ unsigned VEC_tree_base_length (const VEC_tree_base *vec_)
>> + { return vec_ ? vec_->num : 0; }
>> + static __inline__ int VEC_tree_base_iterate (const VEC_tree_base *vec_, unsigned ix_, tree *ptr)
>> + {
>> +   if (vec_ && ix_ < vec_->num) { *ptr = vec_->vec[ix_]; return 1; } else { *ptr = 0; return 0; }
>> + }
>> + extern void abort (void);
>> + void __attribute__((noinline)) foo (size_t x)
>> + {
>> +   if (x != 18446744073709551614UL)
>> +     abort ();
>> + }
>> + void
>> + resort_type_method_vec (VEC_tree_gc *method_vec)
>> + {
>> +   int len = (VEC_tree_base_length(((method_vec) ? &(method_vec)->base : 0)));
>> +   size_t slot;
>> +   tree fn;
>> +
>> +   for (slot = 2;
>> +        (VEC_tree_base_iterate(((method_vec) ? &(method_vec)->base : 0),slot,&(fn)));
>> +        ++slot)
>> +     if (!(((((((fn)->base.code) == 225) ? (((struct tree_overload*)(fn))->function) : (fn)))->decl_minimal.name)))
>> +       break;
>> +
>> +   if (len - slot > 1)
>> +     foo (len - slot);
>> + }
>> +
>> + int main ()
>> + {
>> +   resort_type_method_vec ((void *)0);
>> +   return 0;
>> + }
>> Index: tree-ssa-pre.c
>> ===================================================================
>> *** tree-ssa-pre.c      (revision 137871)
>> --- tree-ssa-pre.c      (working copy)
>> *************** pre_expr_eq (const void *p1, const void
>> *** 193,200 ****
>>    switch (e1->kind)
>>      {
>>      case CONSTANT:
>> !       return expressions_equal_p (PRE_EXPR_CONSTANT (e1),
>> !                                 PRE_EXPR_CONSTANT (e2));
>>      case NAME:
>>        return PRE_EXPR_NAME (e1) == PRE_EXPR_NAME (e2);
>>      case NARY:
>> --- 193,200 ----
>>    switch (e1->kind)
>>      {
>>      case CONSTANT:
>> !       return vn_constant_eq_with_type (PRE_EXPR_CONSTANT (e1),
>> !                                      PRE_EXPR_CONSTANT (e2));
>>      case NAME:
>>        return PRE_EXPR_NAME (e1) == PRE_EXPR_NAME (e2);
>>      case NARY:
>> *************** pre_expr_hash (const void *p1)
>> *** 214,220 ****
>>    switch (e->kind)
>>      {
>>      case CONSTANT:
>> !       return iterative_hash_expr (PRE_EXPR_CONSTANT (e), 0);
>>      case NAME:
>>        return iterative_hash_expr (PRE_EXPR_NAME (e), 0);
>>      case NARY:
>> --- 214,220 ----
>>    switch (e->kind)
>>      {
>>      case CONSTANT:
>> !       return vn_hash_constant_with_type (PRE_EXPR_CONSTANT (e));
>>      case NAME:
>>        return iterative_hash_expr (PRE_EXPR_NAME (e), 0);
>>      case NARY:
>> *************** get_representative_for (const pre_expr e
>> *** 1229,1234 ****
>> --- 1229,1235 ----
>>      case NAME:
>>        return PRE_EXPR_NAME (e);
>>      case CONSTANT:
>> +       return PRE_EXPR_CONSTANT (e);
>>      case NARY:
>>      case REFERENCE:
>>        {
>>
>