ACATS-4 ca11d02 exposed an error in the logic for recognizing and
identifying the inner object in decode_field_ref: a view-converting
load, inserted in a previous successful field merging operation, was
recognized by gimple_convert_def_p within decode_field_reference, and
as a result we took its operand as the expression, and failed to take
note of the load location.
Without that load, we couldn't compare vuses, and then we ended up
inserting a wider load before relevant parts of the object were
initialized.
This patch makes gimple_convert_def_p recognize loads only when
requested, and requires that either both or neither parts of a
potentially merged operand have associated loads.
As a bonus, it enables additional optimizations by swapping the
operands of the second compare when that makes left-hand operands
of both compares match.
Regstrapped on x86_64-linux-gnu and on ppc64-linux-gnu, along with 3
other ifcombine patches. Ok to install?
for gcc/ChangeLog
* gimple-fold.cc (gimple_convert_def_p): Reject load stmts
unless requested.
(decode_field_reference): Accept a converting load at the last
conversion matcher, subsuming the load identification.
(fold_truth_andor_for_ifcombine): Refuse to merge operands
when only one of them has an associated load stmt. Swap
operands of one of the compares if that helps them match.
for gcc/testsuite/ChangeLog
* gcc.dg/field-merge-13.c: New.
---
gcc/gimple-fold.cc | 93 ++++++++++++++++++++++++---------
gcc/testsuite/gcc.dg/field-merge-13.c | 93 +++++++++++++++++++++++++++++++++
2 files changed, 162 insertions(+), 24 deletions(-)
create mode 100644 gcc/testsuite/gcc.dg/field-merge-13.c
diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
index 6c11654a2c65b..92f02ddd77408 100644
--- a/gcc/gimple-fold.cc
+++ b/gcc/gimple-fold.cc
@@ -7440,26 +7440,42 @@ maybe_fold_comparisons_from_match_pd (tree type, enum
tree_code code,
}
/* Return TRUE and set op[0] if T, following all SSA edges, is a type
- conversion. */
+ conversion. Reject loads if LOAD is NULL, otherwise set *LOAD if a
+ converting load is found. */
static bool
-gimple_convert_def_p (tree t, tree op[1])
+gimple_convert_def_p (tree t, tree op[1], gimple **load = NULL)
{
+ bool ret = false;
+
if (TREE_CODE (t) == SSA_NAME
&& !SSA_NAME_IS_DEFAULT_DEF (t))
if (gassign *def = dyn_cast <gassign *> (SSA_NAME_DEF_STMT (t)))
- switch (gimple_assign_rhs_code (def))
- {
- CASE_CONVERT:
- op[0] = gimple_assign_rhs1 (def);
- return true;
- case VIEW_CONVERT_EXPR:
- op[0] = TREE_OPERAND (gimple_assign_rhs1 (def), 0);
- return true;
- default:
- break;
- }
- return false;
+ {
+ bool load_p = gimple_assign_load_p (def);
+ if (load_p && !load)
+ return false;
+ switch (gimple_assign_rhs_code (def))
+ {
+ CASE_CONVERT:
+ op[0] = gimple_assign_rhs1 (def);
+ ret = true;
+ break;
+
+ case VIEW_CONVERT_EXPR:
+ op[0] = TREE_OPERAND (gimple_assign_rhs1 (def), 0);
+ ret = true;
+ break;
+
+ default:
+ break;
+ }
+
+ if (ret && load_p)
+ *load = def;
+ }
+
+ return ret;
}
/* Return TRUE and set op[*] if T, following all SSA edges, resolves to a
@@ -7604,19 +7620,23 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
*pbitsize,
return NULL_TREE;
}
- /* Yet another chance to drop conversions. */
- if (gimple_convert_def_p (exp, res_ops))
+ /* Yet another chance to drop conversions. This one is allowed to
+ match a converting load, subsuming the load identification block
+ below. */
+ if (gimple_convert_def_p (exp, res_ops, load))
{
if (!outer_type)
{
outer_type = TREE_TYPE (exp);
loc[0] = gimple_location (SSA_NAME_DEF_STMT (exp));
}
+ if (*load)
+ loc[3] = gimple_location (*load);
exp = res_ops[0];
}
/* Identify the load, if there is one. */
- if (TREE_CODE (exp) == SSA_NAME
+ if (!(*load) && TREE_CODE (exp) == SSA_NAME
&& !SSA_NAME_IS_DEFAULT_DEF (exp))
{
gimple *def = SSA_NAME_DEF_STMT (exp);
@@ -8056,13 +8076,37 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
tree truth_type,
/* It must be true that the inner operation on the lhs of each
comparison must be the same if we are to be able to do anything.
Then see if we have constants. If not, the same must be true for
- the rhs's. */
+ the rhs's. If one is a load and the other isn't, we have to be
+ conservative and avoid the optimization, otherwise we could get
+ SRAed fields wrong. */
if (volatilep
|| ll_reversep != rl_reversep
- || ll_inner == 0 || rl_inner == 0
- || ! operand_equal_p (ll_inner, rl_inner, 0)
- || (ll_load && rl_load
- && gimple_vuse (ll_load) != gimple_vuse (rl_load)))
+ || ll_inner == 0 || rl_inner == 0)
+ return 0;
+
+ if (! operand_equal_p (ll_inner, rl_inner, 0))
+ {
+ /* Try swapping the operands. */
+ if (ll_reversep != rr_reversep
+ || !rr_inner
+ || !operand_equal_p (ll_inner, rr_inner, 0))
+ return 0;
+
+ rcode = swap_tree_comparison (rcode);
+ std::swap (rl_arg, rr_arg);
+ std::swap (rl_inner, rr_inner);
+ std::swap (rl_bitsize, rr_bitsize);
+ std::swap (rl_bitpos, rr_bitpos);
+ std::swap (rl_unsignedp, rr_unsignedp);
+ std::swap (rl_reversep, rr_reversep);
+ std::swap (rl_and_mask, rr_and_mask);
+ std::swap (rl_load, rr_load);
+ std::swap (rl_loc, rr_loc);
+ }
+
+ if ((ll_load && rl_load)
+ ? gimple_vuse (ll_load) != gimple_vuse (rl_load)
+ : (!ll_load != !rl_load))
return 0;
if (TREE_CODE (lr_arg) == INTEGER_CST
@@ -8083,8 +8127,9 @@ fold_truth_andor_for_ifcombine (enum tree_code code, tree
truth_type,
else if (lr_reversep != rr_reversep
|| lr_inner == 0 || rr_inner == 0
|| ! operand_equal_p (lr_inner, rr_inner, 0)
- || (lr_load && rr_load
- && gimple_vuse (lr_load) != gimple_vuse (rr_load)))
+ || ((lr_load && rr_load)
+ ? gimple_vuse (lr_load) != gimple_vuse (rr_load)
+ : (!lr_load != !rr_load)))
return 0;
/* If we found sign tests, finish turning them into bit tests. */
diff --git a/gcc/testsuite/gcc.dg/field-merge-13.c
b/gcc/testsuite/gcc.dg/field-merge-13.c
new file mode 100644
index 0000000000000..7e4f4c499347f
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/field-merge-13.c
@@ -0,0 +1,93 @@
+/* { dg-do run } */
+/* { dg-options "-O -fdump-tree-ifcombine-details" } */
+
+/* Check that we optimize swapped compares, and that we don't load from objects
+ before they're fully initialized. */
+
+struct pair {
+ signed char a;
+ signed char b;
+} __attribute__ ((aligned (2)));
+
+#define make_pair(a,b) { a, b }
+
+struct s {
+ struct pair c;
+ struct pair d;
+} __attribute__ ((aligned (4)));
+
+struct pair cp = make_pair (127, -1);
+struct pair dp = make_pair (42, 0xf1);
+
+struct pair cq = make_pair (127, -1);
+struct pair dq = make_pair (42, 0xf1);
+
+struct pair cr = make_pair (-127, -1);
+struct pair dr = make_pair (42, 0xff);
+
+static __attribute__ ((noinline, noclone, noipa))
+struct pair copy_pair (struct pair c)
+{
+ return c;
+}
+
+static inline
+struct s
+make_s (struct pair c, struct pair d)
+{
+ struct s r;
+ r.c = copy_pair (c);
+ r.d = copy_pair (d);
+ return r;
+}
+
+void f (void) {
+ struct s p = make_s (cp, dp);
+ struct s q = make_s (cq, dr);
+
+ if (0
+ || p.c.a != q.c.a
+ || q.c.b != p.c.b
+ || p.d.b != q.d.b
+ || q.d.a != p.d.a
+ )
+ return;
+ __builtin_abort ();
+}
+
+void g (void) {
+ struct s p = make_s (cp, dp);
+ struct s q = make_s (cr, dq);
+
+ if (0
+ || p.c.a != q.c.a
+ || q.c.b != p.c.b
+ || p.d.b != q.d.b
+ || q.d.a != p.d.a
+ )
+ return;
+ __builtin_abort ();
+}
+
+void h (void) {
+ struct s p = make_s (cp, dp);
+ struct s q = make_s (cq, dq);
+
+ if (0
+ || p.c.a != q.c.a
+ || q.c.b != p.c.b
+ || p.d.b != q.d.b
+ || q.d.a != p.d.a
+ )
+ __builtin_abort ();
+ return;
+}
+
+int main () {
+ f ();
+ g ();
+ h ();
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump-times "optimizing" 9 "ifcombine" } } */
--
Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
Free Software Activist GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity
Excluding neuro-others for not behaving ""normal"" is *not* inclusive
