ifcombine field merge: stricten loads tests, swap compare to match

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.

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.

diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
index b90ad8f090060b9eaf32ba12d30c5fbb6faa9421..46b4874c4885cb94bbd69cb00b00df61e3b779ab 100644 (file)
--- 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,
 }
 \f
 /* 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 (file)
index 0000000..7e4f4c4
--- /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" } } */