On Wed, 3 Jun 2020, Richard Biener wrote:
>
> The following patch avoids keeping the inherited MEM_ATTRs when
> set_mem_attributes_minus_bitpos is called with a variable ARRAY_REF.
> The inherited ones may not reflect the correct offset.
>
> We try to preserve the MEM_EXPR from the base object expansion
> but we have to make sure to also preserve the corresponding MEM_ALIAS_SET
> then since otherwise path-based disambiguation gets confused.
> The other choice we'd have would be to drop the MEM_EXPR but keep
> the alias-set from the full reference. Or, of course "improve"
> MEM_EXPR generation to not "fail" in case the ARRAY_REF offset is
> variable (we keep variable ARRAY_REFs in the MEM_EXPR if there's
> a COMPONENT_REF ontop of the ref for example). I
> understand the "inheriting" be exactly useful in the
> case we otherwise give up (I've added an assert that we're not
> inheriting from sth when creating a stack slot).
>
> The expand_assignment fix could be treated separately (in different
> code paths we are already preserving MEM_ALIAS_SET this way).
>
> As said the alternative fix for set_mem_attrs_minus_bitpos would
> be to remove all the ARRAY_REF special-casing and do the same
> as we do for MEM_REF.
>
> Bootstrap and regtest running on x86_64-unknown-linux-gnu.
Testing passed but only with the assert removed - at least when
expanding SSA partitions we run into this case (but with a NULL
previous MEM_EXPR).
> Any comments?
>
> Thanks,
> Richard.
>
> 2020-06-03 Richard Biener <rguent...@suse.de>
>
> PR middle-end/95493
> * emit-rtl.c (set_mem_attrs_minus_bitpos): Clear
> offset-known when the ARRAY_REF is variable. If we end up
> inherting the MEM_EXPR from the original MEM_ATTRS then also
> inherit the corresponding MEM_ALIAS_SET. Avoid altering
> MEM_KEEP_ALIAS_SET_P in that case.
> * expr.c (expand_assignment): Preserve MEM_ALIAS_SET when emitting
> the store.
>
> * g++.dg/torture/pr95493.C: New testcase.
> ---
> gcc/emit-rtl.c | 59 ++++++++++++++----------
> gcc/expr.c | 6 ++-
> gcc/testsuite/g++.dg/torture/pr95493.C | 62 ++++++++++++++++++++++++++
> 3 files changed, 103 insertions(+), 24 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/torture/pr95493.C
>
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index 2b790636366..09521b6f85e 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -1964,8 +1964,9 @@ set_mem_attributes_minus_bitpos (rtx ref, tree t, int
> objectp,
> refattrs = MEM_ATTRS (ref);
> if (refattrs)
> {
> - /* ??? Can this ever happen? Calling this routine on a MEM that
> - already carries memory attributes should probably be invalid. */
> + gcc_assert (! TYPE_P (t));
> + /* We are calling this function with attributes set up from expanding
> + the base of a memory reference as returned by get_inner_referece. */
> attrs.expr = refattrs->expr;
> attrs.offset_known_p = refattrs->offset_known_p;
> attrs.offset = refattrs->offset;
> @@ -2052,11 +2053,6 @@ set_mem_attributes_minus_bitpos (rtx ref, tree t, int
> objectp,
> as = TYPE_ADDR_SPACE (TREE_TYPE (base));
> }
>
> - /* If this expression uses it's parent's alias set, mark it such
> - that we won't change it. */
> - if (component_uses_parent_alias_set_from (t) != NULL_TREE)
> - MEM_KEEP_ALIAS_SET_P (ref) = 1;
> -
> /* If this is a decl, set the attributes of the MEM from it. */
> if (DECL_P (t))
> {
> @@ -2114,6 +2110,9 @@ set_mem_attributes_minus_bitpos (rtx ref, tree t, int
> objectp,
> }
> while (TREE_CODE (t2) == ARRAY_REF);
>
> + attrs.offset_known_p = false;
> + attrs.offset = 0;
> + apply_bitpos = 0;
> if (DECL_P (t2)
> || (TREE_CODE (t2) == COMPONENT_REF
> /* For trailing arrays t2 doesn't have a size that
> @@ -2141,24 +2140,38 @@ set_mem_attributes_minus_bitpos (rtx ref, tree t, int
> objectp,
> apply_bitpos = bitpos;
> }
>
> - /* If this is a reference based on a partitioned decl replace the
> - base with a MEM_REF of the pointer representative we created
> - during stack slot partitioning. */
> - if (attrs.expr
> - && VAR_P (base)
> - && ! is_global_var (base)
> - && cfun->gimple_df->decls_to_pointers != NULL)
> + /* If we chose to keep the MEM_EXPR from the inner reference
> + expansion make sure to also keep the corresponding alias-set
> + since otherwise path-based disambiguation is confused. */
> + if (refattrs
> + && attrs.expr == refattrs->expr)
> + attrs.alias = refattrs->alias;
> + else if (attrs.expr)
> {
> - tree *namep = cfun->gimple_df->decls_to_pointers->get (base);
> - if (namep)
> + /* If this expression uses it's parent's alias set, mark it such
> + that we won't change it. */
> + if (component_uses_parent_alias_set_from (t) != NULL_TREE)
> + MEM_KEEP_ALIAS_SET_P (ref) = 1;
> +
> + /* If this is a reference based on a partitioned decl replace the
> + base with a MEM_REF of the pointer representative we created
> + during stack slot partitioning. */
> + if (attrs.expr
> + && VAR_P (base)
> + && ! is_global_var (base)
> + && cfun->gimple_df->decls_to_pointers != NULL)
> {
> - attrs.expr = unshare_expr (attrs.expr);
> - tree *orig_base = &attrs.expr;
> - while (handled_component_p (*orig_base))
> - orig_base = &TREE_OPERAND (*orig_base, 0);
> - tree aptrt = reference_alias_ptr_type (*orig_base);
> - *orig_base = build2 (MEM_REF, TREE_TYPE (*orig_base), *namep,
> - build_int_cst (aptrt, 0));
> + tree *namep = cfun->gimple_df->decls_to_pointers->get (base);
> + if (namep)
> + {
> + attrs.expr = unshare_expr (attrs.expr);
> + tree *orig_base = &attrs.expr;
> + while (handled_component_p (*orig_base))
> + orig_base = &TREE_OPERAND (*orig_base, 0);
> + tree aptrt = reference_alias_ptr_type (*orig_base);
> + *orig_base = build2 (MEM_REF, TREE_TYPE (*orig_base), *namep,
> + build_int_cst (aptrt, 0));
> + }
> }
> }
>
> diff --git a/gcc/expr.c b/gcc/expr.c
> index 6b75028e7f1..2357178fce4 100644
> --- a/gcc/expr.c
> +++ b/gcc/expr.c
> @@ -5354,6 +5354,7 @@ expand_assignment (tree to, tree from, bool nontemporal)
> }
> else
> {
> + alias_set_type alias_set;
> if (MEM_P (to_rtx))
> {
> /* If the field is at offset zero, we could have been given the
> @@ -5362,7 +5363,10 @@ expand_assignment (tree to, tree from, bool
> nontemporal)
> set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos);
> if (volatilep)
> MEM_VOLATILE_P (to_rtx) = 1;
> + alias_set = MEM_ALIAS_SET (to_rtx);
> }
> + else
> + alias_set = get_alias_set (to);
>
> gcc_checking_assert (known_ge (bitpos, 0));
> if (optimize_bitfield_assignment_op (bitsize, bitpos,
> @@ -5373,7 +5377,7 @@ expand_assignment (tree to, tree from, bool nontemporal)
> else
> result = store_field (to_rtx, bitsize, bitpos,
> bitregion_start, bitregion_end,
> - mode1, from, get_alias_set (to),
> + mode1, from, alias_set,
> nontemporal, reversep);
> }
>
> diff --git a/gcc/testsuite/g++.dg/torture/pr95493.C
> b/gcc/testsuite/g++.dg/torture/pr95493.C
> new file mode 100644
> index 00000000000..5e05056854d
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/torture/pr95493.C
> @@ -0,0 +1,62 @@
> +// { dg-do run }
> +// { dg-additional-options "-std=c++17" }
> +
> +struct verify
> +{
> + const bool m_failed = false;
> +
> + [[gnu::noinline]] void print_hex(const void* x, int n) const
> + {
> + const auto* bytes = static_cast<const unsigned char*>(x);
> + for (int i = 0; i < n; ++i)
> + __builtin_printf((i && i % 4 == 0) ? "'%02x" : "%02x", bytes[i]);
> + __builtin_printf("\n");
> + }
> +
> + template <typename... Ts>
> + verify(bool ok, const Ts&... extra_info) : m_failed(!ok)
> + {
> + if (m_failed)
> + (print_hex(&extra_info, sizeof(extra_info)), ...);
> + }
> +
> + ~verify()
> + {
> + if (m_failed)
> + __builtin_abort();
> + }
> +};
> +
> +using K [[gnu::vector_size(16)]] = int;
> +
> +int
> +main()
> +{
> + int count = 1;
> + asm("" : "+m"(count));
> + verify(count == 1, 0, "", 0);
> +
> + {
> + struct SW
> + {
> + K d;
> + };
> + struct
> + {
> + SW d;
> + } xx;
> + SW& x = xx.d;
> + x = SW(); // [0, 0, 0, 0]
> + for (int i = 3; i >= 2; --i)
> + {
> + x.d[i] = -1; // [0, 0, 0, -1] ...
> + int a = [](K y) {
> + for (int j = 0; j < 4; ++j)
> + if (y[j] != 0)
> + return j;
> + return -1;
> + }(x.d);
> + verify(a == i, 0, 0, 0, 0, i, x);
> + }
> + }
> +}
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
