store-merging: Fix coalesce_immediate_stores [PR93820]

The following testcase is miscompiled in 8+.
The problem is that check_no_overlap has a special case for INTEGER_CST
marked stores (i.e. stores of constants), if both all currenly merged stores
and the one under consideration for merging with them are marked that way,
it anticipates that other INTEGER_CST marked stores that overlap with those
and precede those (have smaller info->order) could be merged with those and
doesn't punt for them.
In PR86844 and PR87859 fixes I've then added quite large code that is
performed after check_no_overlap and tries to find out if we need and can
merge further INTEGER_CST marked stores, or need to punt.
Unfortunately, that code is there only in the overlapping case code and
the testcase below shows that we really need it even in the adjacent store
case.  After sort_by_bitpos we have:
bitpos	width	order	rhs_code
96	32	3	INTEGER_CST
128	32	1	INTEGER_CST
128	128	2	INTEGER_CST
192	32	0	MEM_REF
Because of the missing PR86844/PR87859-ish code in the adjacent store
case, we merge the adjacent (memory wise) stores 96/32/3 and 128/32/1,
and then we consider the 128-bit store which is in program-order in between
them, but in this case we punt, because the merging would extend the
merged store region from bitpos 96 and 64-bits to bitpos 96 and 160-bits
and that has an overlap with an incompatible store (the MEM_REF one).
The problem is that we can't really punt this way, because the 128-bit
store is in between those two we've merged already, so either we manage
to merge even that one together with the others, or would need to avoid
already merging the 96/32/3 and 128/32/1 stores together.
Now, rather than copying around the PR86844/PR87859 code to the other spot,
we can actually just use the overlapping code, merge_overlapping is really
a superset of merge_into, so that is what the patch does.  If doing
adjacent store merge for rhs_code other than INTEGER_CST, I believe the
current code is already fine, check_no_overlap in that case doesn't make
the exception and will punt if there is some earlier (smaller order)
non-mergeable overlapping store.  There is just one case that could be
problematic, if the merged_store has BIT_INSERT_EXPRs in them and the
new store is a constant store (INTEGER_CST rhs_code), then check_no_overlap
would do the exception and still would allow the special case.  But we
really shouldn't have the special case in that case, so this patch also
changes check_no_overlap to just have a bool whether we should have the
special case or not.

Note, as I said in the PR, for GCC11 we could consider performing some kind
of cheap DSE during the store merging (perhaps guarded with flag_tree_dse).
And another thing to consider is only consider as problematic non-mergeable
stores that not only have order smaller than last_order as currently, but
also have order larger than first_order, as in this testcase if we actually
ignored (not merged with anything at all) the 192/32/0 store, because it is
not in between the other stores we'd merge, it would be fine to merge the
other 3 stores, though of course the testcase can be easily adjusted by
putting the 192/32 store after the 128/32 store and then this patch would be
still needed.  Though, I think I'd need more time thinking this over.

2020-02-26  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/93820
	* gimple-ssa-store-merging.c (check_no_overlap): Change RHS_CODE
	argument to ALL_INTEGER_CST_P boolean.
	(imm_store_chain_info::try_coalesce_bswap): Adjust caller.
	(imm_store_chain_info::coalesce_immediate_stores): Likewise.  Handle
	adjacent INTEGER_CST store into merged_store->only_constants like
	overlapping one.

	* gcc.dg/pr93820.c: New test.

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 724df77c4d2335e1af9099756e5d06de9be06b5c..f3da501ef110f172bcc1aaa775deda170baaf1e6 100644 (file)
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,13 @@
+2020-02-26  Jakub Jelinek  <jakub@redhat.com>
+
+       PR tree-optimization/93820
+       * gimple-ssa-store-merging.c (check_no_overlap): Change RHS_CODE
+       argument to ALL_INTEGER_CST_P boolean.
+       (imm_store_chain_info::try_coalesce_bswap): Adjust caller.
+       (imm_store_chain_info::coalesce_immediate_stores): Likewise.  Handle
+       adjacent INTEGER_CST store into merged_store->only_constants like
+       overlapping one.
+
 2020-02-25  Jakub Jelinek  <jakub@redhat.com>
 
        PR other/93912

diff --git a/gcc/gimple-ssa-store-merging.c b/gcc/gimple-ssa-store-merging.c
index 4bcafef4878988d6b0896c6e3a860ccd6b957878..c5dc1a81debf90d742202266723e7ca52f481984 100644 (file)
--- a/gcc/gimple-ssa-store-merging.c
+++ b/gcc/gimple-ssa-store-merging.c
@@ -2370,8 +2370,9 @@ gather_bswap_load_refs (vec<tree> *refs, tree val)
 /* Check if there are any stores in M_STORE_INFO after index I
    (where M_STORE_INFO must be sorted by sort_by_bitpos) that overlap
    a potential group ending with END that have their order
-   smaller than LAST_ORDER.  RHS_CODE is the kind of store in the
-   group.  Return true if there are no such stores.
+   smaller than LAST_ORDER.  ALL_INTEGER_CST_P is true if
+   all the stores already merged and the one under consideration
+   have rhs_code of INTEGER_CST.  Return true if there are no such stores.
    Consider:
      MEM[(long long int *)p_28] = 0;
      MEM[(long long int *)p_28 + 8B] = 0;
@@ -2394,13 +2395,13 @@ gather_bswap_load_refs (vec<tree> *refs, tree val)
    the MEM[(long long int *)p_28 + 8B] = 0; would now be before it,
    so we need to refuse merging MEM[(long long int *)p_28 + 8B] = 0;
    into the group.  That way it will be its own store group and will
-   not be touched.  If RHS_CODE is INTEGER_CST and there are overlapping
+   not be touched.  If ALL_INTEGER_CST_P and there are overlapping
    INTEGER_CST stores, those are mergeable using merge_overlapping,
    so don't return false for those.  */
 
 static bool
 check_no_overlap (vec<store_immediate_info *> m_store_info, unsigned int i,
-                 enum tree_code rhs_code, unsigned int last_order,
+                 bool all_integer_cst_p, unsigned int last_order,
                  unsigned HOST_WIDE_INT end)
 {
   unsigned int len = m_store_info.length ();
@@ -2410,7 +2411,7 @@ check_no_overlap (vec<store_immediate_info *> m_store_info, unsigned int i,
       if (info->bitpos >= end)
        break;
       if (info->order < last_order
-         && (rhs_code != INTEGER_CST || info->rhs_code != INTEGER_CST))
+         && (!all_integer_cst_p || info->rhs_code != INTEGER_CST))
        return false;
     }
   return true;
@@ -2563,7 +2564,7 @@ imm_store_chain_info::try_coalesce_bswap (merged_store_group *merged_store,
   if (n.base_addr == NULL_TREE && !is_gimple_val (n.src))
     return false;
 
-  if (!check_no_overlap (m_store_info, last, LROTATE_EXPR, last_order, end))
+  if (!check_no_overlap (m_store_info, last, false, last_order, end))
     return false;
 
   /* Don't handle memory copy this way if normal non-bswap processing
@@ -2713,7 +2714,14 @@ imm_store_chain_info::coalesce_immediate_stores ()
               |---store 2---|
         Overlapping stores.  */
       else if (IN_RANGE (info->bitpos, merged_store->start,
-                        merged_store->start + merged_store->width - 1))
+                        merged_store->start + merged_store->width - 1)
+              /* |---store 1---||---store 2---|
+                 Handle also the consecutive INTEGER_CST stores case here,
+                 as we have here the code to deal with overlaps.  */
+              || (info->bitregion_start <= merged_store->bitregion_end
+                  && info->rhs_code == INTEGER_CST
+                  && merged_store->only_constants
+                  && merged_store->can_be_merged_into (info)))
        {
          /* Only allow overlapping stores of constants.  */
          if (info->rhs_code == INTEGER_CST
@@ -2725,8 +2733,7 @@ imm_store_chain_info::coalesce_immediate_stores ()
              unsigned HOST_WIDE_INT end
                = MAX (merged_store->start + merged_store->width,
                       info->bitpos + info->bitsize);
-             if (check_no_overlap (m_store_info, i, INTEGER_CST,
-                                   last_order, end))
+             if (check_no_overlap (m_store_info, i, true, last_order, end))
                {
                  /* check_no_overlap call above made sure there are no
                     overlapping stores with non-INTEGER_CST rhs_code
@@ -2879,7 +2886,7 @@ imm_store_chain_info::coalesce_immediate_stores ()
              std::swap (info->ops[0], info->ops[1]);
              info->ops_swapped_p = true;
            }
-         if (check_no_overlap (m_store_info, i, info->rhs_code,
+         if (check_no_overlap (m_store_info, i, false,
                                MAX (merged_store->last_order, info->order),
                                MAX (merged_store->start + merged_store->width,
                                     info->bitpos + info->bitsize)))

diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 217c3600b1b8df7f2c155a78b46b61383e0b7979..53a2a4ca9762075c208398e782fbaf06fb88c212 100644 (file)
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,5 +1,8 @@
 2020-02-26  Jakub Jelinek  <jakub@redhat.com>
 
+       PR tree-optimization/93820
+       * gcc.dg/pr93820.c: New test.
+
        PR c++/93905
        * g++.dg/cpp0x/pr93905.C: New test.
 

diff --git a/gcc/testsuite/gcc.dg/pr93820.c b/gcc/testsuite/gcc.dg/pr93820.c
new file mode 100644 (file)
index 0000000..98d8c20
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr93820.c
@@ -0,0 +1,26 @@
+/* PR tree-optimization/93820 */
+/* { dg-do run } */
+/* { dg-options "-O2 -fno-tree-dse" } */
+
+typedef int v4si __attribute__((vector_size(4 * sizeof (int))));
+int a[10];
+
+__attribute__((noipa)) int
+foo (int *p)
+{
+  a[6] = *p;
+  a[4] = 1;
+  *(((v4si *)&a[0]) + 1) = (v4si) { 0, 0, 0, 0 };
+  a[3] = 0;
+}
+
+int
+main ()
+{
+  int i = 0;
+  foo (&i);
+  for (i = 0; i < 10; i++)
+    if (a[i])
+      __builtin_abort ();
+  return 0;
+}