Hi,
On Tue, Apr 01 2025, Richard Biener wrote:
> 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.
>>
[...]
>>
>> 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'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'
Thanks, I am about to commit the following then (I did another way of
bootstrapping and testing on x86_64-linux and Aarch64-linux).
Martin
gcc/ChangeLog:
2025-04-04 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: Include ipa-utils.h.
(types_equal_for_same_type_for_tbaa_p): New public overloaded variant.
* 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 | 17 +++++++++++---
gcc/tree-ssa-alias-compare.h | 2 ++
gcc/tree-ssa-alias.cc | 13 ++++++++++-
4 files changed, 57 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..ae7cd57a5f2 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,9 @@ 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));
+
if (lacc)
{
lacc->grp_assignment_write = 1;
@@ -1536,6 +1541,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 +1562,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 +2405,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 +2441,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 +2506,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..fb37337cfaf 100644
--- a/gcc/tree-ssa-alias-compare.h
+++ b/gcc/tree-ssa-alias-compare.h
@@ -40,4 +40,6 @@ class ao_compare : public operand_compare
inchash::hash &hstate);
};
+bool types_equal_for_same_type_for_tbaa_p (tree type1, tree type2);
+
#endif
diff --git a/gcc/tree-ssa-alias.cc b/gcc/tree-ssa-alias.cc
index e93d5187d50..9dd1780867d 100644
--- a/gcc/tree-ssa-alias.cc
+++ b/gcc/tree-ssa-alias.cc
@@ -48,6 +48,7 @@ along with GCC; see the file COPYING3. If not see
#include "tree-ssa-alias-compare.h"
#include "builtins.h"
#include "internal-fn.h"
+#include "ipa-utils.h"
/* Broad overview of how alias analysis on gimple works:
@@ -4165,7 +4166,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. */
+ when compared with other types using same_type_for_tbaa. */
static bool
types_equal_for_same_type_for_tbaa_p (tree type1, tree type2,
@@ -4188,6 +4189,16 @@ types_equal_for_same_type_for_tbaa_p (tree type1, tree
type2,
return TYPE_CANONICAL (type1) == TYPE_CANONICAL (type2);
}
+/* Return ture if TYPE1 and TYPE2 will always give the same answer
+ when compared with other types using same_type_for_tbaa. */
+
+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 ());
+}
+
/* Compare REF1 and REF2 and return flags specifying their differences.
If LTO_STREAMING_SAFE is true do not use alias sets and canonical
types that are going to be recomputed.
--
2.48.1
