typedef __SIZE_TYPE__ size_t; typedef __UINTPTR_TYPE__ uintptr_t; struct S { int a; unsigned short b; int c, d, e; long f, g, h; int i, j; }; static struct S *k; static size_t l = 0; int m; static int bar (void) { unsigned i; int j; if (k[0].c == 0) { ++m; size_t n = l * 2; struct S *o; o = (struct S *) __builtin_realloc (k, sizeof (struct S) * n); if (!o) __builtin_exit (0); k = o; for (i = l; i < n; i++) { void *p = (void *) &k[i]; int q = 0; size_t r = sizeof (struct S); if ((((uintptr_t) p) % __alignof__ (long)) == 0 && r % sizeof (long) == 0) { long __attribute__ ((may_alias)) *s = (long *) p; long *t = (long *) ((char *) s + r); while (s < t) *s++ = 0; } else __builtin_memset (p, q, r); k[i].c = i + 1; k[i].a = -1; } k[n - 1].c = 0; k[0].c = l; l = n; } j = k[0].c; k[0].c = k[j].c; return j; } int main () { k = (struct S *) __builtin_malloc (sizeof (struct S)); if (!k) __builtin_exit (0); __builtin_memset (k, '\0', sizeof (struct S)); k->a = -1; l = 1; for (int i = 0; i < 15; ++i) bar (); if (m != 4) __builtin_abort (); return 0; } is miscompiled with -O3 starting with r241789. The bug can be seen when diffing store_merging dump with the previous one: --- rh1547495.c.192t.widening_mul 2018-02-21 17:25:00.049972838 +0100 +++ rh1547495.c.193t.store-merging 2018-02-21 17:25:00.049972838 +0100 @@ -1,6 +1,10 @@ ;; Function main (main, funcdef_no=1, decl_uid=2691, cgraph_uid=1, symbol_order=4) (executed once) +Coalescing successful! +Merged into 2 stores +New sequence of 2 stmts to replace old one of 3 stmts +Merging successful! main () { unsigned int i; @@ -120,8 +124,6 @@ main () goto <bb 11>; [57.11%] <bb 10> [local count: 510973054]: - MEM[(long int *)p_28] = 0; - MEM[(long int *)p_28 + 8B] = 0; MEM[(long int *)p_28 + 16B] = 0; MEM[(long int *)p_28 + 24B] = 0; MEM[(long int *)p_28 + 32B] = 0; @@ -130,7 +132,8 @@ main () _14 = i_31 + 1; _119 = (int) _14; MEM[(struct S *)p_28].c = _119; - MEM[(struct S *)p_28].a = -1; + MEM[(void *)p_28] = 18446744069414584320; + MEM[(long int *)p_28 + 8B] = 0; _111 = (long unsigned int) _14; if (n_21 > _111) goto <bb 8>; [89.00%] MEM[(void *)p_28] = 18446744069414584320; is fine, that stores 0xffffffff00000000 into the first 8 bytes, but MEM[(struct S *)p_28].c = _119; stores 4 bytes at offset 8 and MEM[(long int *)p_28 + 8B] = 0; overwrites it.
Can be reproduced also on x86_64-linux with -O3 -fno-tree-vectorize -fno-ivopts. For the latter, I wonder what's the point in using TARGET_MEM_REF in: MEM[(long int *)p_28] = 0; MEM[(long int *)p_28 + 8B] = 0; MEM[(long int *)p_28 + 16B] = 0; MEM[(long int *)p_28 + 24B] = 0; MEM[(long int *)p_28 + 32B] = 0; MEM[(long int *)p_28 + 40B] = 0; MEM[(long int *)p_28 + 48B] = 0; , isn't that something that MEM_REF can express too? store-merging doesn't handle TARGET_MEM_REFs and only handles MEM_REFs. So, for stage1 shall it handle also TARGET_MEM_REFs that only have base and optionally constant disp and nothing else, or shall ivopts pass instead just generate MEM_REFs in those cases?
The bug is in the way we handle overlapping stores. The problem is that all we do if there is overlap is: if (IN_RANGE (info->bitpos, merged_store->start, merged_store->start + merged_store->width - 1)) { /* Only allow overlapping stores of constants. */ if (info->rhs_code == INTEGER_CST && merged_store->stores[0]->rhs_code == INTEGER_CST) { merged_store->merge_overlapping (info); continue; } } otherwise we: /* |---store 1---| <gap> |---store 2---|. Gap between stores or the rhs not compatible. Start a new group. */ /* Try to apply all the stores recorded for the group to determine the bitpattern they write and discard it if that fails. This will also reject single-store groups. */ if (!merged_store->apply_stores ()) delete merged_store; else m_merged_store_groups.safe_push (merged_store); merged_store = new merged_store_group (info); But the statements here are sorted primarily by bitpos and we emit statements for the group at the location of the last statement (highest order) in the merged store group. So I think we need to check before adding another store if it is not rhs_code INTEGER_CST, whether there is any overlap with following stores. Overlap is fine if the order of all the overlapping statements is higher than MAX (merged_store->last_order, info->order), because then we know we'll start a new group right after info and the merged stores of the current group will come before any overlapping stores (whether merged successfully with something or not), but otherwise we just shouldn't add info into the current group.
Created attachment 43485 [details] gcc8-pr84503.patch Untested fix.
*** Bug 84505 has been marked as a duplicate of this bug. ***
In 7.x this exact problem doesn't really exist, so the issue must be different there. I'd think it might be something fixable by PR82916 - like patch, but haven't actually tried that yet.
Author: jakub Date: Thu Feb 22 08:28:42 2018 New Revision: 257891 URL: https://gcc.gnu.org/viewcvs?rev=257891&root=gcc&view=rev Log: PR tree-optimization/84503 * gimple-ssa-store-merging.c (merged_store_group::merge_into): Compute width as info->bitpos + info->bitsize - start. (merged_store_group::merge_overlapping): Simplify width computation. (check_no_overlap): New function. (imm_store_chain_info::try_coalesce_bswap): Compute expected start + width and last_order of the group, fail if check_no_overlap fails. (imm_store_chain_info::coalesce_immediate_stores): Don't merge info to group if check_no_overlap fails. * gcc.dg/pr84503-1.c: New test. * gcc.dg/pr84503-2.c: New test. Added: trunk/gcc/testsuite/gcc.dg/pr84503-1.c trunk/gcc/testsuite/gcc.dg/pr84503-2.c Modified: trunk/gcc/ChangeLog trunk/gcc/gimple-ssa-store-merging.c trunk/gcc/testsuite/ChangeLog
Fixed on the trunk so far, 7.x fix will be very different.
Author: jakub Date: Sat Mar 3 13:38:48 2018 New Revision: 258201 URL: https://gcc.gnu.org/viewcvs?rev=258201&root=gcc&view=rev Log: Backported from mainline 2018-02-22 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/84503 * gcc.dg/pr84503-1.c: New test. * gcc.dg/pr84503-2.c: New test. 2017-11-10 Jakub Jelinek <jakub@redhat.com> PR bootstrap/82916 * gimple-ssa-store-merging.c (pass_store_merging::terminate_all_aliasing_chains): For gimple_store_p stmts also call refs_output_dependent_p. * gcc.dg/pr82916.c: New test. Added: branches/gcc-7-branch/gcc/testsuite/gcc.dg/pr82916.c branches/gcc-7-branch/gcc/testsuite/gcc.dg/pr84503-1.c branches/gcc-7-branch/gcc/testsuite/gcc.dg/pr84503-2.c Modified: branches/gcc-7-branch/gcc/ChangeLog branches/gcc-7-branch/gcc/gimple-ssa-store-merging.c branches/gcc-7-branch/gcc/testsuite/ChangeLog
Fixed.