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 <ja...@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
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
