store-merging: Apply --param=store-merging-max-size= in more spots [PR117439]

Hi!

Store merging assumes a merged region won't be too large.  The assumption is
e.g. in using inappropriate types in various spots (e.g. int for bit sizes
and bit positions in a few spots, or unsigned for the total size in bytes of
the merged region), in doing XNEWVEC for the whole total size of the merged
region and preparing everything in there and even that XALLOCAVEC in two
spots.  The last case is what was breaking the test below in the patch,
64MB XALLOCAVEC is just too large, but even with that fixed I think we just
shouldn't be merging gigabyte large merge groups.

We already have --param=store-merging-max-size= parameter, right now with
65536 bytes maximum (if needed, we could raise that limit a little bit).
That parameter is currently used when merging two adjacent stores, if the
size of the already merged bitregion together with the new store's bitregion
is above that limit, we don't merge those.
I guess initially that was sufficient, at that time a store was always
limited to MAX_BITSIZE_MODE_ANY_INT bits.
But later on we've added support for empty ctors ({} and even later
{CLOBBER}) and also added another spot where we merge further stores into
the merge group, if there is some overlap, we can merge various other stores
in one coalesce_immediate_stores iteration.
And, we weren't applying the --param=store-merging-max-size= parameter
in either of those cases.  So a single store can be gigabytes long, and
if there is some overlap, we can extend the region again to gigabytes in
size.

The following patch attempts to apply that parameter even in those cases.
So, if testing if it should merge the merged group with info (we've already
punted if those together are above the parameter) and some other stores,
the first two hunks just punt if that would make the merge group too large.
And the third hunk doesn't even add stores which are over the limit.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2024-11-06  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/117439
	* gimple-ssa-store-merging.cc
	(imm_store_chain_info::coalesce_immediate_stores): Punt if merging of
	any of the additional overlapping stores would result in growing the
	bitregion size over param_store_merging_max_size.
	(pass_store_merging::process_store): Terminate all aliasing chains
	for stores with bitregion larger than param_store_merging_max_size.

	* g++.dg/opt/pr117439.C: New test.


	Jakub

On Wed, 6 Nov 2024, Jakub Jelinek wrote:

> Hi!
> 
> Store merging assumes a merged region won't be too large.  The assumption is
> e.g. in using inappropriate types in various spots (e.g. int for bit sizes
> and bit positions in a few spots, or unsigned for the total size in bytes of
> the merged region), in doing XNEWVEC for the whole total size of the merged
> region and preparing everything in there and even that XALLOCAVEC in two
> spots.  The last case is what was breaking the test below in the patch,
> 64MB XALLOCAVEC is just too large, but even with that fixed I think we just
> shouldn't be merging gigabyte large merge groups.
> 
> We already have --param=store-merging-max-size= parameter, right now with
> 65536 bytes maximum (if needed, we could raise that limit a little bit).
> That parameter is currently used when merging two adjacent stores, if the
> size of the already merged bitregion together with the new store's bitregion
> is above that limit, we don't merge those.
> I guess initially that was sufficient, at that time a store was always
> limited to MAX_BITSIZE_MODE_ANY_INT bits.
> But later on we've added support for empty ctors ({} and even later
> {CLOBBER}) and also added another spot where we merge further stores into
> the merge group, if there is some overlap, we can merge various other stores
> in one coalesce_immediate_stores iteration.
> And, we weren't applying the --param=store-merging-max-size= parameter
> in either of those cases.  So a single store can be gigabytes long, and
> if there is some overlap, we can extend the region again to gigabytes in
> size.
> 
> The following patch attempts to apply that parameter even in those cases.
> So, if testing if it should merge the merged group with info (we've already
> punted if those together are above the parameter) and some other stores,
> the first two hunks just punt if that would make the merge group too large.
> And the third hunk doesn't even add stores which are over the limit.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

OK.

Thanks,
Richard.

> 2024-11-06  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/117439
> 	* gimple-ssa-store-merging.cc
> 	(imm_store_chain_info::coalesce_immediate_stores): Punt if merging of
> 	any of the additional overlapping stores would result in growing the
> 	bitregion size over param_store_merging_max_size.
> 	(pass_store_merging::process_store): Terminate all aliasing chains
> 	for stores with bitregion larger than param_store_merging_max_size.
> 
> 	* g++.dg/opt/pr117439.C: New test.
> 
> --- gcc/gimple-ssa-store-merging.cc.jj	2024-11-05 08:55:34.000000000 +0100
> +++ gcc/gimple-ssa-store-merging.cc	2024-11-05 11:07:30.495980747 +0100
> @@ -3245,6 +3245,10 @@ imm_store_chain_info::coalesce_immediate
>  		      unsigned int min_order = first_order;
>  		      unsigned first_nonmergeable_int_order = ~0U;
>  		      unsigned HOST_WIDE_INT this_end = end;
> +		      unsigned HOST_WIDE_INT this_bitregion_start
> +			= new_bitregion_start;
> +		      unsigned HOST_WIDE_INT this_bitregion_end
> +			= new_bitregion_end;
>  		      k = i;
>  		      first_nonmergeable_order = ~0U;
>  		      for (unsigned int j = i + 1; j < len; ++j)
> @@ -3268,6 +3272,19 @@ imm_store_chain_info::coalesce_immediate
>  				  k = 0;
>  				  break;
>  				}
> +			      if (info2->bitregion_start
> +				  < this_bitregion_start)
> +				this_bitregion_start = info2->bitregion_start;
> +			      if (info2->bitregion_end
> +				  > this_bitregion_end)
> +				this_bitregion_end = info2->bitregion_end;
> +			      if (((this_bitregion_end - this_bitregion_start
> +				    + 1) / BITS_PER_UNIT)
> +				  > (unsigned) param_store_merging_max_size)
> +				{
> +				  k = 0;
> +				  break;
> +				}
>  			      k = j;
>  			      min_order = MIN (min_order, info2->order);
>  			      this_end = MAX (this_end,
> @@ -5335,7 +5352,9 @@ pass_store_merging::process_store (gimpl
>        || !bitsize.is_constant (&const_bitsize)
>        || !bitpos.is_constant (&const_bitpos)
>        || !bitregion_start.is_constant (&const_bitregion_start)
> -      || !bitregion_end.is_constant (&const_bitregion_end))
> +      || !bitregion_end.is_constant (&const_bitregion_end)
> +      || ((const_bitregion_end - const_bitregion_start + 1) / BITS_PER_UNIT
> +	  > (unsigned) param_store_merging_max_size))
>      return terminate_all_aliasing_chains (NULL, stmt);
>  
>    if (!ins_stmt)
> --- gcc/testsuite/g++.dg/opt/pr117439.C.jj	2024-11-05 11:00:41.871831185 +0100
> +++ gcc/testsuite/g++.dg/opt/pr117439.C	2024-11-05 11:01:08.311452638 +0100
> @@ -0,0 +1,16 @@
> +// PR tree-optimization/117439
> +// { dg-do compile }
> +// { dg-options "-O2" }
> +
> +struct A {
> +  A () : a (0), b (0) {}
> +  unsigned a, b;
> +};
> +struct B {
> +  A c, d[0x800000];
> +  B () {}
> +};
> +struct C {
> +  A e;
> +  B f;
> +} g;
> 
> 	Jakub
> 
>