| Message ID | nycvar.YFH.7.76.2001221451211.5566@zhemvz.fhfr.qr |
|---|---|
| State | New |
| Headers | show |
| Series | tree-optimization/93354 FRE redundant store removal validity fix | expand |
On Wed, 22 Jan 2020, Richard Biener wrote: > > This fixes the validity check for redundant store removal by looking > at the actual stmt that represents the last (relevant) change to > the TBAA state rather than the imperfectly tracked alias set in the > expression hash table which then becomes unused (see a followup change > to get rid of that). > > On the way this allows re-enabling of VN_WALKREWRITE, fixing PR91123. > > This also exposes an out-of-bound array access in > real.c:clear_significand_below fixed as well. > > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress. So this didn't work out. The following addresses this in a different way, postponing a fix for PR91123 which needs another adjustment. Bootstrapped on x86_64-unknown-linux-gnu, pushed. This fixes tracking of the alias-set of partial defs for use by redundant store removal. 2020-01-23 Richard Biener <rguenther@suse.de> PR tree-optimization/93381 * tree-ssa-sccvn.c (vn_walk_cb_data::push_partial_def): Take alias-set of the def as argument and record the first one. (vn_walk_cb_data::first_set): New member. (vn_reference_lookup_3): Pass the alias-set of the current def to push_partial_def. Fix alias-set used in the aggregate copy case. (vn_reference_lookup): Consistently set *last_vuse_ptr. * real.c (clear_significand_below): Fix out-of-bound access. * gcc.dg/torture/pr93354.c: New testcase. diff --git a/gcc/real.c b/gcc/real.c index a57e5df113f..46d8126efe4 100644 --- a/gcc/real.c +++ b/gcc/real.c @@ -420,7 +420,10 @@ clear_significand_below (REAL_VALUE_TYPE *r, unsigned int n) for (i = 0; i < w; ++i) r->sig[i] = 0; - r->sig[w] &= ~(((unsigned long)1 << (n % HOST_BITS_PER_LONG)) - 1); + /* We are actually passing N == SIGNIFICAND_BITS which would result + in an out-of-bound access below. */ + if (n % HOST_BITS_PER_LONG != 0) + r->sig[w] &= ~(((unsigned long)1 << (n % HOST_BITS_PER_LONG)) - 1); } /* Divide the significands of A and B, placing the result in R. Return diff --git a/gcc/testsuite/gcc.dg/torture/pr93354.c b/gcc/testsuite/gcc.dg/torture/pr93354.c new file mode 100644 index 00000000000..8ccf1e6d2c0 --- /dev/null +++ b/gcc/testsuite/gcc.dg/torture/pr93354.c @@ -0,0 +1,22 @@ +/* { dg-do run } */ + +typedef __INT32_TYPE__ int32_t; +typedef __INT64_TYPE__ int64_t; +struct X { int32_t i; int32_t j; }; +void foo (int64_t *z) +{ + ((struct X *)z)->i = 0x05060708; + ((struct X *)z)->j = 0x01020304; + *z = 0x0102030405060708; +} + +int main() +{ + int64_t l = 0; + int64_t *p; + asm ("" : "=r" (p) : "0" (&l)); + foo (p); + if (l != 0x0102030405060708) + __builtin_abort (); + return 0; +} diff --git a/gcc/tree-ssa-sccvn.c b/gcc/tree-ssa-sccvn.c index 0b8ee586139..6e0b2202157 100644 --- a/gcc/tree-ssa-sccvn.c +++ b/gcc/tree-ssa-sccvn.c @@ -1693,7 +1693,8 @@ struct vn_walk_cb_data ao_ref_init (&orig_ref, orig_ref_); } ~vn_walk_cb_data (); - void *push_partial_def (const pd_data& pd, tree, HOST_WIDE_INT); + void *push_partial_def (const pd_data& pd, tree, + alias_set_type, HOST_WIDE_INT); vn_reference_t vr; ao_ref orig_ref; @@ -1706,6 +1707,7 @@ struct vn_walk_cb_data /* The first defs range to avoid splay tree setup in most cases. */ pd_range first_range; tree first_vuse; + alias_set_type first_set; splay_tree known_ranges; obstack ranges_obstack; }; @@ -1746,13 +1748,13 @@ pd_tree_dealloc (void *, void *) } /* Push PD to the vector of partial definitions returning a - value when we are ready to combine things with VUSE and MAXSIZEI, + value when we are ready to combine things with VUSE, SET and MAXSIZEI, NULL when we want to continue looking for partial defs or -1 on failure. */ void * vn_walk_cb_data::push_partial_def (const pd_data &pd, tree vuse, - HOST_WIDE_INT maxsizei) + alias_set_type set, HOST_WIDE_INT maxsizei) { const HOST_WIDE_INT bufsize = 64; /* We're using a fixed buffer for encoding so fail early if the object @@ -1773,6 +1775,7 @@ vn_walk_cb_data::push_partial_def (const pd_data &pd, tree vuse, first_range.offset = pd.offset; first_range.size = pd.size; first_vuse = vuse; + first_set = set; last_vuse_ptr = NULL; /* Continue looking for partial defs. */ return NULL; @@ -1903,10 +1906,10 @@ vn_walk_cb_data::push_partial_def (const pd_data &pd, tree vuse, if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, "Successfully combined %u partial definitions\n", ndefs); - /* ??? If we track partial defs alias-set we could use that if it - is the same for all. Use zero for now. */ + /* We are using the alias-set of the first store we encounter which + should be appropriate here. */ return vn_reference_lookup_or_insert_for_pieces - (first_vuse, 0, vr->type, vr->operands, val); + (first_vuse, first_set, vr->type, vr->operands, val); } else { @@ -2492,7 +2495,7 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_, pd.rhs = build_constructor (NULL_TREE, NULL); pd.offset = (offset2i - offseti) / BITS_PER_UNIT; pd.size = leni; - return data->push_partial_def (pd, vuse, maxsizei); + return data->push_partial_def (pd, vuse, 0, maxsizei); } } @@ -2553,7 +2556,8 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_, pd.rhs = gimple_assign_rhs1 (def_stmt); pd.offset = (offset2i - offseti) / BITS_PER_UNIT; pd.size = size2i / BITS_PER_UNIT; - return data->push_partial_def (pd, vuse, maxsizei); + return data->push_partial_def (pd, vuse, get_alias_set (lhs), + maxsizei); } } } @@ -2665,7 +2669,8 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_, pd.rhs = rhs; pd.offset = (offset2i - offseti) / BITS_PER_UNIT; pd.size = size2i / BITS_PER_UNIT; - return data->push_partial_def (pd, vuse, maxsizei); + return data->push_partial_def (pd, vuse, get_alias_set (lhs), + maxsizei); } } } @@ -2751,7 +2756,8 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_, pd.rhs = SSA_VAL (def_rhs); pd.offset = (offset2i - offseti) / BITS_PER_UNIT; pd.size = size2i / BITS_PER_UNIT; - return data->push_partial_def (pd, vuse, maxsizei); + return data->push_partial_def (pd, vuse, get_alias_set (lhs), + maxsizei); } } } @@ -2764,6 +2770,7 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_, || TREE_CODE (gimple_assign_rhs1 (def_stmt)) == MEM_REF || handled_component_p (gimple_assign_rhs1 (def_stmt)))) { + tree lhs = gimple_assign_lhs (def_stmt); tree base2; int i, j, k; auto_vec<vn_reference_op_s> rhs; @@ -2870,7 +2877,7 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_, { if (data->partial_defs.is_empty ()) return vn_reference_lookup_or_insert_for_pieces - (vuse, get_alias_set (rhs1), vr->type, vr->operands, val); + (vuse, get_alias_set (lhs), vr->type, vr->operands, val); /* This is the only interesting case for partial-def handling coming from targets that like to gimplify init-ctors as aggregate copies from constant data like aarch64 for @@ -2882,7 +2889,8 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void *data_, pd.rhs = val; pd.offset = 0; pd.size = maxsizei / BITS_PER_UNIT; - return data->push_partial_def (pd, vuse, maxsizei); + return data->push_partial_def (pd, vuse, get_alias_set (lhs), + maxsizei); } } @@ -3205,6 +3213,8 @@ vn_reference_lookup (tree op, tree vuse, vn_lookup_kind kind, return NULL_TREE; } + if (last_vuse_ptr) + *last_vuse_ptr = vr1.vuse; return vn_reference_lookup_1 (&vr1, vnresult); }
diff --git a/gcc/real.c b/gcc/real.c index a57e5df113f..46d8126efe4 100644 --- a/gcc/real.c +++ b/gcc/real.c @@ -420,7 +420,10 @@ clear_significand_below (REAL_VALUE_TYPE *r, unsigned int n) for (i = 0; i < w; ++i) r->sig[i] = 0; - r->sig[w] &= ~(((unsigned long)1 << (n % HOST_BITS_PER_LONG)) - 1); + /* We are actually passing N == SIGNIFICAND_BITS which would result + in an out-of-bound access below. */ + if (n % HOST_BITS_PER_LONG != 0) + r->sig[w] &= ~(((unsigned long)1 << (n % HOST_BITS_PER_LONG)) - 1); } /* Divide the significands of A and B, placing the result in R. Return diff --git a/gcc/testsuite/gcc.dg/torture/pr93354.c b/gcc/testsuite/gcc.dg/torture/pr93354.c new file mode 100644 index 00000000000..8ccf1e6d2c0 --- /dev/null +++ b/gcc/testsuite/gcc.dg/torture/pr93354.c @@ -0,0 +1,22 @@ +/* { dg-do run } */ + +typedef __INT32_TYPE__ int32_t; +typedef __INT64_TYPE__ int64_t; +struct X { int32_t i; int32_t j; }; +void foo (int64_t *z) +{ + ((struct X *)z)->i = 0x05060708; + ((struct X *)z)->j = 0x01020304; + *z = 0x0102030405060708; +} + +int main() +{ + int64_t l = 0; + int64_t *p; + asm ("" : "=r" (p) : "0" (&l)); + foo (p); + if (l != 0x0102030405060708) + __builtin_abort (); + return 0; +} diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-85.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-85.c new file mode 100644 index 00000000000..c50770caa2f --- /dev/null +++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-fre-85.c @@ -0,0 +1,14 @@ +/* { dg-do compile } */ +/* { dg-options "-O -fstrict-aliasing -fdump-tree-fre1-details" } */ + +struct X { int i; int j; }; + +struct X x, y; +void foo () +{ + x.i = 1; + y = x; + y.i = 1; // redundant +} + +/* { dg-final { scan-tree-dump "Deleted redundant store y.i" "fre1" } } */ diff --git a/gcc/tree-ssa-sccvn.c b/gcc/tree-ssa-sccvn.c index 0b8ee586139..ef272047bb3 100644 --- a/gcc/tree-ssa-sccvn.c +++ b/gcc/tree-ssa-sccvn.c @@ -3205,6 +3205,8 @@ vn_reference_lookup (tree op, tree vuse, vn_lookup_kind kind, return NULL_TREE; } + if (last_vuse_ptr) + *last_vuse_ptr = vr1.vuse; return vn_reference_lookup_1 (&vr1, vnresult); } @@ -4543,6 +4545,23 @@ visit_reference_op_load (tree lhs, tree op, gimple *stmt) } +/* Returns whether the store to LHS affects dependence analysis compared + to the next preceeding stmt doing so at VUSE. */ + +static bool +store_affects_tbaa_memory_state (tree lhs, tree vuse) +{ + gassign *def = dyn_cast <gassign *> (SSA_NAME_DEF_STMT (vuse)); + if (!def) + return true; + alias_set_type lhs_set = get_alias_set (lhs); + alias_set_type def_set = get_alias_set (gimple_assign_lhs (def)); + if (lhs_set != def_set + && !alias_set_subset_of (lhs_set, def_set)) + return true; + return false; +} + /* Visit a store to a reference operator LHS, part of STMT, value number it, and return true if the value number of the LHS has changed as a result. */ @@ -4575,7 +4594,8 @@ visit_reference_op_store (tree lhs, tree op, gimple *stmt) Otherwise, the vdefs for the store are used when inserting into the table, since the store generates a new memory state. */ - vn_reference_lookup (lhs, vuse, VN_NOWALK, &vnresult, false); + tree last_vuse = NULL_TREE; + vn_reference_lookup (lhs, vuse, VN_NOWALK, &vnresult, false, &last_vuse); if (vnresult && vnresult->result) { @@ -4587,9 +4607,7 @@ visit_reference_op_store (tree lhs, tree op, gimple *stmt) { /* If the TBAA state isn't compatible for downstream reads we cannot value-number the VDEFs the same. */ - alias_set_type set = get_alias_set (lhs); - if (vnresult->set != set - && ! alias_set_subset_of (set, vnresult->set)) + if (store_affects_tbaa_memory_state (lhs, last_vuse)) resultsame = false; } } @@ -5644,9 +5662,11 @@ eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi) lookup_lhs = NULL_TREE; } tree val = NULL_TREE; + tree last_vuse = NULL_TREE; if (lookup_lhs) - val = vn_reference_lookup (lookup_lhs, gimple_vuse (stmt), VN_WALK, - &vnresult, false); + val = vn_reference_lookup (lookup_lhs, gimple_vuse (stmt), + VN_WALKREWRITE, &vnresult, false, + &last_vuse); if (TREE_CODE (rhs) == SSA_NAME) rhs = VN_INFO (rhs)->valnum; if (val @@ -5660,13 +5680,12 @@ eliminate_dom_walker::eliminate_stmt (basic_block b, gimple_stmt_iterator *gsi) && TREE_CODE (val) == VIEW_CONVERT_EXPR && TREE_OPERAND (val, 0) == rhs))) { - /* We can only remove the later store if the former aliases - at least all accesses the later one does or if the store + /* We now proved the memory contents before and after the store + are the same. We can only remove the later store if the former + aliases at least all accesses the later one does or if the store was to readonly memory storing the same value. */ - alias_set_type set = get_alias_set (lhs); if (! vnresult - || vnresult->set == set - || alias_set_subset_of (set, vnresult->set)) + || !store_affects_tbaa_memory_state (lhs, last_vuse)) { if (dump_file && (dump_flags & TDF_DETAILS)) {