store-merging: Fix coalesce_immediate_stores [PR93820]
authorJakub Jelinek <jakub@redhat.com>
Wed, 26 Feb 2020 08:33:48 +0000 (09:33 +0100)
committerJakub Jelinek <jakub@redhat.com>
Wed, 26 Feb 2020 08:33:48 +0000 (09:33 +0100)
commit4d213bf6011ed2b30b9d0ca70069a5dbc294b5d7
treed1870db352caf46793e7ad8562c4f329b1d92cdd
parent5de338f001eb2a821f40e599bd55119d43e0ae7e
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.
gcc/ChangeLog
gcc/gimple-ssa-store-merging.c
gcc/testsuite/ChangeLog
gcc/testsuite/gcc.dg/pr93820.c [new file with mode: 0644]