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 @@
[PATCH] Handle gimple_clobber_p stmts in store-merging (PR target/92038)
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?
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);
- 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 (merged_store_group *group,
unsigned HOST_WIDE_INT align_bitpos
= (try_bitpos - align_base) & (group_align - 1);
unsigned HOST_WIDE_INT align =
