Re: [PATCH] Handle gimple_clobber_p stmts in store-merging (PR target/92038)
On Thu, 7 Nov 2019 at 16:28, Jakub Jelinek wrote:
>
> Hi!
>
> The following patch adds handling of clobbers in store-merging. The intent
> is if we have a clobber followed by some stores into the clobbered area,
> even if don't store all the bytes in the area, we can avoid masking, because
> the non-stored bytes are undefined and in some cases we can even overwrite
> the whole area with the same or smaller number of stores compared to the
> original IL.
> Clobbers aren't removed from the IL, even if the following stores completely
> cover the whole area, as clobbers carry important additional information
> that the old value is gone, e.g. for tail call discovery if address taken
> before the clobber but not after it, removing the clobbers would disable
> tail call optimization.
> The patch right now treats the clobbered non-stored bytes as non-masked zero
> stores, except that we don't add stores to whole words etc. if there are no
> other overlapping stores; I have a separate patch that also computed
> defined_mask which contained whether some bytes are just undefined and we
> could in theory try different bit patterns in those bytes, but in the end
> decided it is too complicated and if needed, could be done as a follow-up.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
Hi Jakub,
I've noticed that the new test store-merging-2.C fails on arm:
FAIL: g++.dg/opt/store-merging-2.C -std=gnu++14 scan-tree-dump
store-merging "New sequence of 2 stores to replace old one of 3
stores"
Christophe
> 2019-11-07 Jakub Jelinek
>
> PR target/92038
> * gimple-ssa-store-merging.c (find_constituent_stores): For return
> value only, return non-NULL if there is a single non-clobber
> constituent store even if there are constituent clobbers and return
> one of clobber constituent stores if all constituent stores are
> clobbers.
> (split_group): Handle clobbers.
> (imm_store_chain_info::output_merged_store): When computing
> bzero_first, look after all clobbers at the start. Don't count
> clobber stmts in orig_num_stmts, except if the first orig store is
> a clobber covering the whole area and split_stores cover the whole
> area, consider equal number of stmts ok. Punt if split_stores
> contains only ->orig stores and their number plus number of original
> clobbers is equal to original number of stmts. For ->orig, look past
> clobbers in the constituent stores.
> (imm_store_chain_info::output_merged_stores): Don't remove clobber
> stmts.
> (rhs_valid_for_store_merging_p): Don't return false for clobber stmt
> rhs.
> (store_valid_for_store_merging_p): Allow clobber stmts.
> (verify_clear_bit_region_be): Fix up a thinko in function comment.
>
> * g++.dg/opt/store-merging-1.C: New test.
> * g++.dg/opt/store-merging-2.C: New test.
> * g++.dg/opt/store-merging-3.C: New test.
>
> --- gcc/gimple-ssa-store-merging.c.jj 2019-11-07 09:50:38.029447052 +0100
> +++ gcc/gimple-ssa-store-merging.c 2019-11-07 12:13:15.048531180 +0100
> @@ -3110,7 +3110,8 @@ split_store::split_store (unsigned HOST_
> /* Record all stores in GROUP that write to the region starting at BITPOS and
> is of size BITSIZE. Record infos for such statements in STORES if
> non-NULL. The stores in GROUP must be sorted by bitposition. Return INFO
> - if there is exactly one original store in the range. */
> + if there is exactly one original store in the range (in that case ignore
> + clobber stmts, unless there are only clobber stmts). */
>
> static store_immediate_info *
> find_constituent_stores (class merged_store_group *group,
> @@ -3146,16 +3147,24 @@ find_constituent_stores (class merged_st
>if (stmt_start >= end)
> return ret;
>
> + if (gimple_clobber_p (info->stmt))
> + {
> + if (stores)
> + stores->safe_push (info);
> + if (ret == NULL)
> + ret = info;
> + continue;
> + }
>if (stores)
> {
> stores->safe_push (info);
> - if (ret)
> + if (ret && !gimple_clobber_p (ret->stmt))
> {
> ret = NULL;
> second = true;
> }
> }
> - else if (ret)
> + else if (ret && !gimple_clobber_p (ret->stmt))
> return NULL;
>if (!second)
> ret = info;
> @@ -3347,13 +3356,17 @@ split_group (merged_store_group *group,
>
>if (bzero_first)
> {
> - first = 1;
> + store_immediate_info *gstore;
> + FOR_EACH_VEC_ELT (group->stores, first, gstore)
> + if (!gimple_clobber_p (gstore->stmt))
> + break;
> + ++first;
>ret = 1;
>if (split_stores)
> {
> split_store *store
> - = new split_store (bytepos, group->stores[0]->bitsize,
> align_base);
> -
Re: [PATCH] Handle gimple_clobber_p stmts in store-merging (PR target/92038)
On Thu, 7 Nov 2019, Jakub Jelinek wrote:
> Hi!
>
> The following patch adds handling of clobbers in store-merging. The intent
> is if we have a clobber followed by some stores into the clobbered area,
> even if don't store all the bytes in the area, we can avoid masking, because
> the non-stored bytes are undefined and in some cases we can even overwrite
> the whole area with the same or smaller number of stores compared to the
> original IL.
> Clobbers aren't removed from the IL, even if the following stores completely
> cover the whole area, as clobbers carry important additional information
> that the old value is gone, e.g. for tail call discovery if address taken
> before the clobber but not after it, removing the clobbers would disable
> tail call optimization.
> The patch right now treats the clobbered non-stored bytes as non-masked zero
> stores, except that we don't add stores to whole words etc. if there are no
> other overlapping stores; I have a separate patch that also computed
> defined_mask which contained whether some bytes are just undefined and we
> could in theory try different bit patterns in those bytes, but in the end
> decided it is too complicated and if needed, could be done as a follow-up.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Thanks,
Richard.
> 2019-11-07 Jakub Jelinek
>
> PR target/92038
> * gimple-ssa-store-merging.c (find_constituent_stores): For return
> value only, return non-NULL if there is a single non-clobber
> constituent store even if there are constituent clobbers and return
> one of clobber constituent stores if all constituent stores are
> clobbers.
> (split_group): Handle clobbers.
> (imm_store_chain_info::output_merged_store): When computing
> bzero_first, look after all clobbers at the start. Don't count
> clobber stmts in orig_num_stmts, except if the first orig store is
> a clobber covering the whole area and split_stores cover the whole
> area, consider equal number of stmts ok. Punt if split_stores
> contains only ->orig stores and their number plus number of original
> clobbers is equal to original number of stmts. For ->orig, look past
> clobbers in the constituent stores.
> (imm_store_chain_info::output_merged_stores): Don't remove clobber
> stmts.
> (rhs_valid_for_store_merging_p): Don't return false for clobber stmt
> rhs.
> (store_valid_for_store_merging_p): Allow clobber stmts.
> (verify_clear_bit_region_be): Fix up a thinko in function comment.
>
> * g++.dg/opt/store-merging-1.C: New test.
> * g++.dg/opt/store-merging-2.C: New test.
> * g++.dg/opt/store-merging-3.C: New test.
>
> --- gcc/gimple-ssa-store-merging.c.jj 2019-11-07 09:50:38.029447052 +0100
> +++ gcc/gimple-ssa-store-merging.c2019-11-07 12:13:15.048531180 +0100
> @@ -3110,7 +3110,8 @@ split_store::split_store (unsigned HOST_
> /* Record all stores in GROUP that write to the region starting at BITPOS and
> is of size BITSIZE. Record infos for such statements in STORES if
> non-NULL. The stores in GROUP must be sorted by bitposition. Return INFO
> - if there is exactly one original store in the range. */
> + if there is exactly one original store in the range (in that case ignore
> + clobber stmts, unless there are only clobber stmts). */
>
> static store_immediate_info *
> find_constituent_stores (class merged_store_group *group,
> @@ -3146,16 +3147,24 @@ find_constituent_stores (class merged_st
>if (stmt_start >= end)
> return ret;
>
> + if (gimple_clobber_p (info->stmt))
> + {
> + if (stores)
> + stores->safe_push (info);
> + if (ret == NULL)
> + ret = info;
> + continue;
> + }
>if (stores)
> {
> stores->safe_push (info);
> - if (ret)
> + if (ret && !gimple_clobber_p (ret->stmt))
> {
> ret = NULL;
> second = true;
> }
> }
> - else if (ret)
> + else if (ret && !gimple_clobber_p (ret->stmt))
> return NULL;
>if (!second)
> ret = info;
> @@ -3347,13 +3356,17 @@ split_group (merged_store_group *group,
>
>if (bzero_first)
> {
> - first = 1;
> + store_immediate_info *gstore;
> + FOR_EACH_VEC_ELT (group->stores, first, gstore)
> + if (!gimple_clobber_p (gstore->stmt))
> + break;
> + ++first;
>ret = 1;
>if (split_stores)
> {
> split_store *store
> - = new split_store (bytepos, group->stores[0]->bitsize, align_base);
> - store->orig_stores.safe_push (group->stores[0]);
> + = new split_store (bytepos, gstore->bitsize, align_base);
> + store->orig_stores.safe_push (gstore);
> store->orig = true;
> any_orig = true;
> split_stores->safe_push (store);
> @@ -3377,6 +3390,7 @@ split_group
