On Mon, 31 Mar 2025, Martin Jambor wrote:
> Hi,
>
> the testcase in PR 118924, when compiled on Aarch64, contains an
> gimple aggregate assignment statement in between different types which
> are types_compatible_p but behave differently for the purposes of
> alias analysis.
>
> SRA replaces the statement with a series of scalar assignments which
> however have LHSs access chains modeled on the RHS type and so do not
> alias with a subsequent reads and so are DSEd.
>
> SRA clearly gets its "same_access_path" logic subtly wrong. One issue
> is that the same_access_path_p function probably should be implemented
> more along the lines of (parts of ao_compare::compare_ao_refs) instead
> of internally relying on operand_equal_p. That is however not the
> problem in the PR and so I will deal with it only later.
>
> The issue here is that even when the access path is the same, it must
> not be bolted on an aggregate type that does not match. This patch
> does that, taking just one simple function from the
> ao_compare::compare_ao_refs machinery and using it to detect the
> situation. The rest is just merging the information in between
> accesses of the same access group.
>
> I looked at how many times we come across such assignment during
> "make stage2-bubble" of GCC (configured with only c and C++ and
> without multilib and libsanitizers) and on an x86_64 there were 87924
> such assignments (though now I realize not all of them had to be
> aggregate), so they do happen. The patch leads to about 5% increase
> of cases where we don't use an "access path" but resort to a
> MEM_REF (from 90209 to 95204). On an Aarch64, there were 92268 such
> assignments and the increase of falling back to MEM_REFs was by
> 4% (but from a bigger base 132983 to 107991).
>
> Bootstrapped on x86_64-linux and Aarch64-linux. OK for master and then
> for all active release branches?
I'm unsure about the lto_streaming_safe arg, in fact the implementation is
if (lto_streaming_safe)
return type1 == type2;
else
return TYPE_CANONICAL (type1) == TYPE_CANONICAL (type2);
but I think we guarantee (we _need_ to guarantee!) that when
TYPE_CANONICAL (type1) == TYPE_CANONICAL (type2), after LTO streaming
it's still TYPE_CANONICAL (type1) == TYPE_CANONICAL (type2). Otherwise
assignments previously valid GIMPLE might no longer be valid and
things that aliased previously might no longer alias.
But that's an implementation bug in types_equal_for_same_type_for_tbaa_p,
but I think you should be able to pass in false as if the implementation
were fixed. IMO if necessary the implementation itself should
use sth like if (flag_lto && !in_lto_p) rather than leaving it up to
the caller ... the only existing caller uses
lto_streaming_expected_p () as arg, which is similar to my proposal.
I'd say you want to export at a forwarder
bool
types_equal_for_same_type_for_tbaa_p (tree type1, tree type2)
{
return types_equal_for_same_type_for_tbaa_p (type1, type2,
lto_streaming_expected_p ());
}
instead as it should be an internal detail.
OK with that change.
Can you fixup the comment
/* Return ture if TYPE1 and TYPE2 will always give the same answer
when compared wit hother types using same_type_for_tbaa_p. */
when you are there? The function is called same_type_for_tbaa
and 'wit hother' should be 'with other'
Richard.
> Thanks,
>
> Martin
>
>
> gcc/ChangeLog:
>
> 2025-03-24 Martin Jambor <mjam...@suse.cz>
>
> PR tree-optimization/118924
> * tree-ssa-alias-compare.h (types_equal_for_same_type_for_tbaa_p):
> Declare.
> * tree-ssa-alias.cc (types_equal_for_same_type_for_tbaa_p): Make
> public.
> * tree-sra.cc: Include tree-ssa-alias-compare.h.
> (create_access): Initialzie grp_same_access_path to true.
> (build_accesses_from_assign): Detect tbaa hazards and clear
> grp_same_access_path fields of involved accesses when they occur.
> (sort_and_splice_var_accesses): Take previous values of
> grp_same_access_path into account.
>
> gcc/testsuite/ChangeLog:
>
> 2025-03-25 Martin Jambor <mjam...@suse.cz>
>
> PR tree-optimization/118924
> * g++.dg/tree-ssa/pr118924.C: New test.
> ---
> gcc/testsuite/g++.dg/tree-ssa/pr118924.C | 29 ++++++++++++++++++++++++
> gcc/tree-sra.cc | 18 ++++++++++++---
> gcc/tree-ssa-alias-compare.h | 3 +++
> gcc/tree-ssa-alias.cc | 2 +-
> 4 files changed, 48 insertions(+), 4 deletions(-)
> create mode 100644 gcc/testsuite/g++.dg/tree-ssa/pr118924.C
>
> diff --git a/gcc/testsuite/g++.dg/tree-ssa/pr118924.C
> b/gcc/testsuite/g++.dg/tree-ssa/pr118924.C
> new file mode 100644
> index 00000000000..c95eacafc9c
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/tree-ssa/pr118924.C
> @@ -0,0 +1,29 @@
> +/* { dg-do run } */
> +/* { dg-options "-std=c++17 -O2" } */
> +
> +template <int Size> struct Vector {
> + int m_data[Size];
> + Vector(int, int, int) {}
> +};
> +enum class E { POINTS, LINES, TRIANGLES };
> +
> +__attribute__((noipa))
> +void getName(E type) {
> + static E check = E::POINTS;
> + if (type == check)
> + check = (E)((int)check + 1);
> + else
> + __builtin_abort ();
> +}
> +
> +int main() {
> + int arr[]{0, 1, 2};
> + for (auto dim : arr) {
> + Vector<3> localInvs(1, 1, 1);
> + localInvs.m_data[dim] = 8;
> + }
> + E types[] = {E::POINTS, E::LINES, E::TRIANGLES};
> + for (auto primType : types)
> + getName(primType);
> + return 0;
> +}
> diff --git a/gcc/tree-sra.cc b/gcc/tree-sra.cc
> index c26559edc66..22d5d1ce999 100644
> --- a/gcc/tree-sra.cc
> +++ b/gcc/tree-sra.cc
> @@ -100,6 +100,7 @@ along with GCC; see the file COPYING3. If not see
> #include "builtins.h"
> #include "tree-sra.h"
> #include "opts.h"
> +#include "tree-ssa-alias-compare.h"
>
> /* Enumeration of all aggregate reductions we can do. */
> enum sra_mode { SRA_MODE_EARLY_IPA, /* early call regularization */
> @@ -979,6 +980,7 @@ create_access (tree expr, gimple *stmt, bool write)
> access->type = TREE_TYPE (expr);
> access->write = write;
> access->grp_unscalarizable_region = unscalarizable_region;
> + access->grp_same_access_path = true;
> access->stmt = stmt;
> access->reverse = reverse;
>
> @@ -1522,6 +1524,10 @@ build_accesses_from_assign (gimple *stmt)
> racc = build_access_from_expr_1 (rhs, stmt, false);
> lacc = build_access_from_expr_1 (lhs, stmt, true);
>
> + bool tbaa_hazard
> + = !types_equal_for_same_type_for_tbaa_p (TREE_TYPE (lhs), TREE_TYPE
> (rhs),
> + sra_mode == SRA_MODE_EARLY_INTRA);
> +
> if (lacc)
> {
> lacc->grp_assignment_write = 1;
> @@ -1536,6 +1542,8 @@ build_accesses_from_assign (gimple *stmt)
> bitmap_set_bit (cannot_scalarize_away_bitmap,
> DECL_UID (lacc->base));
> }
> + if (tbaa_hazard)
> + lacc->grp_same_access_path = false;
> }
>
> if (racc)
> @@ -1555,6 +1563,8 @@ build_accesses_from_assign (gimple *stmt)
> }
> if (storage_order_barrier_p (lhs))
> racc->grp_unscalarizable_region = 1;
> + if (tbaa_hazard)
> + racc->grp_same_access_path = false;
> }
>
> if (lacc && racc
> @@ -2396,7 +2406,7 @@ sort_and_splice_var_accesses (tree var)
> bool grp_partial_lhs = access->grp_partial_lhs;
> bool first_scalar = is_gimple_reg_type (access->type);
> bool unscalarizable_region = access->grp_unscalarizable_region;
> - bool grp_same_access_path = true;
> + bool grp_same_access_path = access->grp_same_access_path;
> bool bf_non_full_precision
> = (INTEGRAL_TYPE_P (access->type)
> && TYPE_PRECISION (access->type) != access->size
> @@ -2432,7 +2442,8 @@ sort_and_splice_var_accesses (tree var)
> return NULL;
> }
>
> - grp_same_access_path = path_comparable_for_same_access (access->expr);
> + if (grp_same_access_path)
> + grp_same_access_path = path_comparable_for_same_access (access->expr);
>
> j = i + 1;
> while (j < access_count)
> @@ -2496,7 +2507,8 @@ sort_and_splice_var_accesses (tree var)
> }
>
> if (grp_same_access_path
> - && !same_access_path_p (access->expr, ac2->expr))
> + && (!ac2->grp_same_access_path
> + || !same_access_path_p (access->expr, ac2->expr)))
> grp_same_access_path = false;
>
> ac2->group_representative = access;
> diff --git a/gcc/tree-ssa-alias-compare.h b/gcc/tree-ssa-alias-compare.h
> index 4c4856c9124..0d48cb160f9 100644
> --- a/gcc/tree-ssa-alias-compare.h
> +++ b/gcc/tree-ssa-alias-compare.h
> @@ -40,4 +40,7 @@ class ao_compare : public operand_compare
> inchash::hash &hstate);
> };
>
> +bool types_equal_for_same_type_for_tbaa_p (tree type1, tree type2,
> + bool lto_streaming_safe);
> +
> #endif
> diff --git a/gcc/tree-ssa-alias.cc b/gcc/tree-ssa-alias.cc
> index e93d5187d50..914daea10fc 100644
> --- a/gcc/tree-ssa-alias.cc
> +++ b/gcc/tree-ssa-alias.cc
> @@ -4167,7 +4167,7 @@ attr_fnspec::verify ()
> /* Return ture if TYPE1 and TYPE2 will always give the same answer
> when compared wit hother types using same_type_for_tbaa_p. */
>
> -static bool
> +bool
> types_equal_for_same_type_for_tbaa_p (tree type1, tree type2,
> bool lto_streaming_safe)
> {
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
