On Mon, 2 Mar 2020, Jakub Jelinek wrote:
> On Mon, Mar 02, 2020 at 12:46:30PM +0100, Richard Biener wrote:
> > > + void *r = data.push_partial_def (pd, 0, prec);
> > > + if (r == (void *) -1)
> > > + return NULL_TREE;
> > > + gcc_assert (r == NULL_TREE);
> > > + }
> > > + pos += tz;
> > > + if (pos == prec)
> > > + break;
> > > + w = wi::lrshift (w, tz);
> > > + tz = wi::ctz (wi::bit_not (w));
> > > + if (pos + tz > prec)
> > > + tz = prec - pos;
> > > + pos += tz;
> > > + w = wi::lrshift (w, tz);
> > > + }
> >
> > I'd do this in the vn_walk_cb_data CTOR instead - you pass mask !=
> > NULL_TREE anyway so you can as well pass mask.
>
> I've tried, but have no idea how to handle the case where
> data.push_partial_def (pd, 0, prec); fails above if it is done in the
> constructor.
> Though, the BIT_AND_EXPR case already checks:
> + && CHAR_BIT == 8
> + && BITS_PER_UNIT == 8
> + && BYTES_BIG_ENDIAN == WORDS_BIG_ENDIAN
> and also checks the pathological cases of mask being all ones or all zeros,
> so it is just the theoretical case of
> maxsizei > bufsize * BITS_PER_UNIT
> so maybe it is moot and we can just assert that push_partial_def
> returned NULL.
>
> > I wonder if we can instead make the above return NULL (finish
> > return (void *)-1) and do sth like
> >
> > if (!wvnresult && mask)
> > return data.masked_result;
> >
> > and thus avoid the type-"unsafe" return frobbing by storing the
> > result value in an extra member of the vn_walk_cb_data struct.
>
> Done that way.
>
> > Any reason you piggy-back on visit_reference_op_load instead of using
> > vn_reference_lookup directly? I'd very much prefer that since it
> > doesn't even try to mess with the SSA lattice.
>
> I didn't want to duplicate the VCE case, but it isn't that long.
Ah, I'm not sure this will ever trigger though, does it?
> So, like this if it passes bootstrap/regtest?
Yes, this is OK (remove the VCE case at your discretion).
Thanks,
Richard.
> 2020-03-02 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/93582
> * tree-ssa-sccvn.h (vn_reference_lookup): Add mask argument.
> * tree-ssa-sccvn.c (struct vn_walk_cb_data): Add mask and masked_result
> members, initialize them in the constructor and if mask is non-NULL,
> artificially push_partial_def {} for the portions of the mask that
> contain zeros.
> (vn_walk_cb_data::finish): If mask is non-NULL, set masked_result to
> val and return (void *)-1. Formatting fix.
> (vn_reference_lookup_pieces): Adjust vn_walk_cb_data initialization.
> Formatting fix.
> (vn_reference_lookup): Add mask argument. If non-NULL, don't call
> fully_constant_vn_reference_p nor vn_reference_lookup_1 and return
> data.mask_result.
> (visit_nary_op): Handle BIT_AND_EXPR of a memory load and INTEGER_CST
> mask.
> (visit_stmt): Formatting fix.
>
> * gcc.dg/tree-ssa/pr93582-10.c: New test.
> * gcc.dg/pr93582.c: New test.
> * gcc.c-torture/execute/pr93582.c: New test.
>
> --- gcc/tree-ssa-sccvn.h.jj 2020-02-28 17:32:56.391363613 +0100
> +++ gcc/tree-ssa-sccvn.h 2020-03-02 13:52:17.488680037 +0100
> @@ -256,7 +256,7 @@ tree vn_reference_lookup_pieces (tree, a
> vec<vn_reference_op_s> ,
> vn_reference_t *, vn_lookup_kind);
> tree vn_reference_lookup (tree, tree, vn_lookup_kind, vn_reference_t *, bool,
> - tree * = NULL);
> + tree * = NULL, tree = NULL_TREE);
> void vn_reference_lookup_call (gcall *, vn_reference_t *, vn_reference_t);
> vn_reference_t vn_reference_insert_pieces (tree, alias_set_type, tree,
> vec<vn_reference_op_s> ,
> --- gcc/tree-ssa-sccvn.c.jj 2020-02-28 17:32:56.390363628 +0100
> +++ gcc/tree-ssa-sccvn.c 2020-03-02 15:48:12.982620557 +0100
> @@ -1686,15 +1686,55 @@ struct pd_data
> struct vn_walk_cb_data
> {
> vn_walk_cb_data (vn_reference_t vr_, tree orig_ref_, tree *last_vuse_ptr_,
> - vn_lookup_kind vn_walk_kind_, bool tbaa_p_)
> + vn_lookup_kind vn_walk_kind_, bool tbaa_p_, tree mask_)
> : vr (vr_), last_vuse_ptr (last_vuse_ptr_), last_vuse (NULL_TREE),
> - vn_walk_kind (vn_walk_kind_), tbaa_p (tbaa_p_),
> - saved_operands (vNULL), first_set (-2), known_ranges (NULL)
> - {
> - if (!last_vuse_ptr)
> - last_vuse_ptr = &last_vuse;
> - ao_ref_init (&orig_ref, orig_ref_);
> - }
> + mask (mask_), masked_result (NULL_TREE), vn_walk_kind (vn_walk_kind_),
> + tbaa_p (tbaa_p_), saved_operands (vNULL), first_set (-2),
> + known_ranges (NULL)
> + {
> + if (!last_vuse_ptr)
> + last_vuse_ptr = &last_vuse;
> + ao_ref_init (&orig_ref, orig_ref_);
> + if (mask)
> + {
> + wide_int w = wi::to_wide (mask);
> + unsigned int pos = 0, prec = w.get_precision ();
> + pd_data pd;
> + pd.rhs = build_constructor (NULL_TREE, NULL);
> + /* When bitwise and with a constant is done on a memory load,
> + we don't really need all the bits to be defined or defined
> + to constants, we don't really care what is in the position
> + corresponding to 0 bits in the mask.
> + So, push the ranges of those 0 bits in the mask as artificial
> + zero stores and let the partial def handling code do the
> + rest. */
> + while (pos < prec)
> + {
> + int tz = wi::ctz (w);
> + if (pos + tz > prec)
> + tz = prec - pos;
> + if (tz)
> + {
> + if (BYTES_BIG_ENDIAN)
> + pd.offset = prec - pos - tz;
> + else
> + pd.offset = pos;
> + pd.size = tz;
> + void *r = push_partial_def (pd, 0, prec);
> + gcc_assert (r == NULL_TREE);
> + }
> + pos += tz;
> + if (pos == prec)
> + break;
> + w = wi::lrshift (w, tz);
> + tz = wi::ctz (wi::bit_not (w));
> + if (pos + tz > prec)
> + tz = prec - pos;
> + pos += tz;
> + w = wi::lrshift (w, tz);
> + }
> + }
> + }
> ~vn_walk_cb_data ();
> void *finish (alias_set_type, tree);
> void *push_partial_def (const pd_data& pd, alias_set_type, HOST_WIDE_INT);
> @@ -1703,6 +1743,8 @@ struct vn_walk_cb_data
> ao_ref orig_ref;
> tree *last_vuse_ptr;
> tree last_vuse;
> + tree mask;
> + tree masked_result;
> vn_lookup_kind vn_walk_kind;
> bool tbaa_p;
> vec<vn_reference_op_s> saved_operands;
> @@ -1731,9 +1773,15 @@ vn_walk_cb_data::finish (alias_set_type
> {
> if (first_set != -2)
> set = first_set;
> - return vn_reference_lookup_or_insert_for_pieces
> - (last_vuse, set, vr->type,
> - saved_operands.exists () ? saved_operands : vr->operands, val);
> + if (mask)
> + {
> + masked_result = val;
> + return (void *) -1;
> + }
> + vec<vn_reference_op_s> &operands
> + = saved_operands.exists () ? saved_operands : vr->operands;
> + return vn_reference_lookup_or_insert_for_pieces (last_vuse, set, vr->type,
> + operands, val);
> }
>
> /* pd_range splay-tree helpers. */
> @@ -3380,13 +3428,13 @@ vn_reference_lookup_pieces (tree vuse, a
> {
> ao_ref r;
> unsigned limit = param_sccvn_max_alias_queries_per_access;
> - vn_walk_cb_data data (&vr1, NULL_TREE, NULL, kind, true);
> + vn_walk_cb_data data (&vr1, NULL_TREE, NULL, kind, true, NULL_TREE);
> if (ao_ref_init_from_vn_reference (&r, set, type, vr1.operands))
> - *vnresult =
> - (vn_reference_t)walk_non_aliased_vuses (&r, vr1.vuse, true,
> - vn_reference_lookup_2,
> - vn_reference_lookup_3,
> - vuse_valueize, limit, &data);
> + *vnresult
> + = ((vn_reference_t)
> + walk_non_aliased_vuses (&r, vr1.vuse, true, vn_reference_lookup_2,
> + vn_reference_lookup_3, vuse_valueize,
> + limit, &data));
> gcc_checking_assert (vr1.operands == shared_lookup_references);
> }
>
> @@ -3402,15 +3450,19 @@ vn_reference_lookup_pieces (tree vuse, a
> was NULL.. VNRESULT will be filled in with the vn_reference_t
> stored in the hashtable if one exists. When TBAA_P is false assume
> we are looking up a store and treat it as having alias-set zero.
> - *LAST_VUSE_PTR will be updated with the VUSE the value lookup succeeded.
> */
> + *LAST_VUSE_PTR will be updated with the VUSE the value lookup succeeded.
> + MASK is either NULL_TREE, or can be an INTEGER_CST if the result of the
> + load is bitwise anded with MASK and so we are only interested in a subset
> + of the bits and can ignore if the other bits are uninitialized or
> + not initialized with constants. */
>
> tree
> vn_reference_lookup (tree op, tree vuse, vn_lookup_kind kind,
> - vn_reference_t *vnresult, bool tbaa_p, tree *last_vuse_ptr)
> + vn_reference_t *vnresult, bool tbaa_p,
> + tree *last_vuse_ptr, tree mask)
> {
> vec<vn_reference_op_s> operands;
> struct vn_reference_s vr1;
> - tree cst;
> bool valuezied_anything;
>
> if (vnresult)
> @@ -3422,11 +3474,11 @@ vn_reference_lookup (tree op, tree vuse,
> vr1.type = TREE_TYPE (op);
> vr1.set = get_alias_set (op);
> vr1.hashcode = vn_reference_compute_hash (&vr1);
> - if ((cst = fully_constant_vn_reference_p (&vr1)))
> - return cst;
> + if (mask == NULL_TREE)
> + if (tree cst = fully_constant_vn_reference_p (&vr1))
> + return cst;
>
> - if (kind != VN_NOWALK
> - && vr1.vuse)
> + if (kind != VN_NOWALK && vr1.vuse)
> {
> vn_reference_t wvnresult;
> ao_ref r;
> @@ -3438,25 +3490,31 @@ vn_reference_lookup (tree op, tree vuse,
> vr1.operands))
> ao_ref_init (&r, op);
> vn_walk_cb_data data (&vr1, r.ref ? NULL_TREE : op,
> - last_vuse_ptr, kind, tbaa_p);
> - wvnresult =
> - (vn_reference_t)walk_non_aliased_vuses (&r, vr1.vuse, tbaa_p,
> - vn_reference_lookup_2,
> - vn_reference_lookup_3,
> - vuse_valueize, limit, &data);
> + last_vuse_ptr, kind, tbaa_p, mask);
> +
> + wvnresult
> + = ((vn_reference_t)
> + walk_non_aliased_vuses (&r, vr1.vuse, tbaa_p, vn_reference_lookup_2,
> + vn_reference_lookup_3, vuse_valueize, limit,
> + &data));
> gcc_checking_assert (vr1.operands == shared_lookup_references);
> if (wvnresult)
> {
> + gcc_assert (mask == NULL_TREE);
> if (vnresult)
> *vnresult = wvnresult;
> return wvnresult->result;
> }
> + else if (mask)
> + return data.masked_result;
>
> return NULL_TREE;
> }
>
> if (last_vuse_ptr)
> *last_vuse_ptr = vr1.vuse;
> + if (mask)
> + return NULL_TREE;
> return vn_reference_lookup_1 (&vr1, vnresult);
> }
>
> @@ -4664,7 +4722,57 @@ visit_nary_op (tree lhs, gassign *stmt)
> }
> }
> }
> - default:;
> + break;
> + case BIT_AND_EXPR:
> + if (INTEGRAL_TYPE_P (type)
> + && TREE_CODE (rhs1) == SSA_NAME
> + && TREE_CODE (gimple_assign_rhs2 (stmt)) == INTEGER_CST
> + && !SSA_NAME_OCCURS_IN_ABNORMAL_PHI (rhs1)
> + && default_vn_walk_kind != VN_NOWALK
> + && CHAR_BIT == 8
> + && BITS_PER_UNIT == 8
> + && BYTES_BIG_ENDIAN == WORDS_BIG_ENDIAN
> + && !integer_all_onesp (gimple_assign_rhs2 (stmt))
> + && !integer_zerop (gimple_assign_rhs2 (stmt)))
> + {
> + gassign *ass = dyn_cast <gassign *> (SSA_NAME_DEF_STMT (rhs1));
> + if (ass
> + && !gimple_has_volatile_ops (ass)
> + && vn_get_stmt_kind (ass) == VN_REFERENCE)
> + {
> + tree last_vuse = gimple_vuse (ass);
> + tree op = gimple_assign_rhs1 (ass);
> + tree result = vn_reference_lookup (op, gimple_vuse (ass),
> + default_vn_walk_kind,
> + NULL, true, &last_vuse,
> + gimple_assign_rhs2 (stmt));
> + if (result)
> + {
> + /* We handle type-punning through unions by value-numbering
> + based on offset and size of the access. Be prepared to
> + handle a type-mismatch here via creating a
> + VIEW_CONVERT_EXPR. */
> + if (!useless_type_conversion_p (TREE_TYPE (result),
> + TREE_TYPE (op)))
> + {
> + /* We will be setting the value number of lhs to the
> + value number of
> + VIEW_CONVERT_EXPR <TREE_TYPE (result)> (result).
> + So first simplify and lookup this expression to see
> + if it is already available. */
> + gimple_match_op res_op (gimple_match_cond::UNCOND,
> + VIEW_CONVERT_EXPR,
> + TREE_TYPE (op), result);
> + result = vn_nary_build_or_lookup (&res_op);
> + }
> + }
> + if (result)
> + return set_ssa_val_to (lhs, result);
> + }
> + }
> + break;
> + default:
> + break;
> }
>
> bool changed = set_ssa_val_to (lhs, lhs);
> @@ -5175,14 +5283,14 @@ visit_stmt (gimple *stmt, bool backedges
> switch (vn_get_stmt_kind (ass))
> {
> case VN_NARY:
> - changed = visit_nary_op (lhs, ass);
> - break;
> + changed = visit_nary_op (lhs, ass);
> + break;
> case VN_REFERENCE:
> - changed = visit_reference_op_load (lhs, rhs1, ass);
> - break;
> + changed = visit_reference_op_load (lhs, rhs1, ass);
> + break;
> default:
> - changed = defs_to_varying (ass);
> - break;
> + changed = defs_to_varying (ass);
> + break;
> }
> }
> }
> --- gcc/testsuite/gcc.dg/tree-ssa/pr93582-10.c.jj 2020-03-02
> 13:52:17.504679803 +0100
> +++ gcc/testsuite/gcc.dg/tree-ssa/pr93582-10.c 2020-03-02
> 13:52:17.504679803 +0100
> @@ -0,0 +1,29 @@
> +/* PR tree-optimization/93582 */
> +/* { dg-do compile { target int32 } } */
> +/* { dg-options "-O2 -fdump-tree-fre1" } */
> +/* { dg-final { scan-tree-dump "return 72876566;" "fre1" { target le } } } */
> +/* { dg-final { scan-tree-dump "return 559957376;" "fre1" { target be } } }
> */
> +
> +union U {
> + struct S { int a : 12, b : 5, c : 10, d : 5; } s;
> + unsigned int i;
> +};
> +struct A { char a[12]; union U u; };
> +void bar (struct A *);
> +
> +unsigned
> +foo (void)
> +{
> + struct A a;
> + bar (&a);
> + a.u.s.a = 1590;
> + a.u.s.c = -404;
> +#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> +#define M 0x67e0a5f
> +#elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> +#define M 0xa5f067e0
> +#else
> +#define M 0
> +#endif
> + return a.u.i & M;
> +}
> --- gcc/testsuite/gcc.dg/pr93582.c.jj 2020-03-02 13:52:17.504679803 +0100
> +++ gcc/testsuite/gcc.dg/pr93582.c 2020-03-02 13:52:17.504679803 +0100
> @@ -0,0 +1,57 @@
> +/* PR tree-optimization/93582 */
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Warray-bounds" } */
> +
> +struct S {
> + unsigned int s1:1;
> + unsigned int s2:1;
> + unsigned int s3:1;
> + unsigned int s4:1;
> + unsigned int s5:4;
> + unsigned char s6;
> + unsigned short s7;
> + unsigned short s8;
> +};
> +struct T {
> + int t1;
> + int t2;
> +};
> +
> +static inline int
> +bar (struct S *x)
> +{
> + if (x->s4)
> + return ((struct T *)(x + 1))->t1 + ((struct T *)(x + 1))->t2; /* {
> dg-bogus "array subscript 1 is outside array bounds of" } */
> + else
> + return 0;
> +}
> +
> +int
> +foo (int x, int y)
> +{
> + struct S s;
> /* { dg-bogus "while referencing" } */
> + s.s6 = x;
> + s.s7 = y & 0x1FFF;
> + s.s4 = 0;
> + return bar (&s);
> +}
> +
> +static inline int
> +qux (struct S *x)
> +{
> + int s4 = x->s4;
> + if (s4)
> + return ((struct T *)(x + 1))->t1 + ((struct T *)(x + 1))->t2;
> + else
> + return 0;
> +}
> +
> +int
> +baz (int x, int y)
> +{
> + struct S s;
> + s.s6 = x;
> + s.s7 = y & 0x1FFF;
> + s.s4 = 0;
> + return qux (&s);
> +}
> --- gcc/testsuite/gcc.c-torture/execute/pr93582.c.jj 2020-03-02
> 13:52:17.504679803 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr93582.c 2020-03-02
> 13:52:17.504679803 +0100
> @@ -0,0 +1,22 @@
> +/* PR tree-optimization/93582 */
> +
> +short a;
> +int b, c;
> +
> +__attribute__((noipa)) void
> +foo (void)
> +{
> + b = c;
> + a &= 7;
> +}
> +
> +int
> +main ()
> +{
> + c = 27;
> + a = 14;
> + foo ();
> + if (b != 27 || a != 6)
> + __builtin_abort ();
> + return 0;
> +}
>
>
> Jakub
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
