From 4d213bf6011ed2b30b9d0ca70069a5dbc294b5d7 Mon Sep 17 00:00:00 2001 From: Jakub Jelinek Date: Wed, 26 Feb 2020 09:33:48 +0100 Subject: [PATCH] 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 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. --- gcc/ChangeLog | 10 ++++++++++ gcc/gimple-ssa-store-merging.c | 27 +++++++++++++++++---------- gcc/testsuite/ChangeLog | 3 +++ gcc/testsuite/gcc.dg/pr93820.c | 26 ++++++++++++++++++++++++++ 4 files changed, 56 insertions(+), 10 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/pr93820.c diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 724df77c4d2..f3da501ef11 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,13 @@ +2020-02-26 Jakub Jelinek + + 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 PR other/93912 diff --git a/gcc/gimple-ssa-store-merging.c b/gcc/gimple-ssa-store-merging.c index 4bcafef4878..c5dc1a81deb 100644 --- a/gcc/gimple-ssa-store-merging.c +++ b/gcc/gimple-ssa-store-merging.c @@ -2370,8 +2370,9 @@ gather_bswap_load_refs (vec *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 *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 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 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 217c3600b1b..53a2a4ca976 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,5 +1,8 @@ 2020-02-26 Jakub Jelinek + 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 index 00000000000..98d8c20a178 --- /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; +} -- 2.30.2