On Sun, Nov 17, 2024 at 4:28 AM Lewis Hyatt <[email protected]> wrote:
>
> Currently, when we allocate a gphi object, we round up the capacity for the
> trailing arguments array such that it will make full use of the page size
> that ggc will allocate. While there is also an explicit minimum of 2
> arguments, in practice after rounding to the ggc page size there is always
> room for at least 4.
>
> It seems we have some code that has come to depend on there being this much
> room before reallocation of a PHI is required. For example, the function
> loop_version () used during loop optimization will make sure there is room
> for an additional edge on each PHI that it processes. But there are call
> sites which cache a PHI pointer prior to calling loop_version () and assume
> it remains valid afterward, thus implicitly assuming that the PHI will have
> spare capacity. Examples include split_loop () and gen_parallel_loop ().
>
> This works fine now, but if the size of a gphi becomes larger, e.g. due to
> configuring location_t to be a 64-bit type, then on 32-bit platforms it ends
> up being possible to get a gphi with only 2 arguments of capacity, causing
> the above call sites of loop_version () to fail. (They store a pointer to a
> gphi object that no longer has the same meaning it did before it got
> reallocated.) The testcases gcc.dg/torture/pr113707-2.c and
> gcc.dg/graphite/pr81945.c exhibit that failure mode.
>
> It may be necessary to adjust those call sites to make this more robust, but
> in the meantime, changing the minimum from 2 to 4 does no harm given the
> minimum is practically 4 anyway, and it resolves the issue for 32-bit
> platforms.
We need to fix the users. Note ideal_phi_node_len rounds up to a power of two
but extra_order_size_table also has MAX_ALIGNMENT * n with n from 1 to 16
buckets, so such extensive rounding up is not needed.
The cache is also quite useless this way (I didn't fix this when last working
there).
Richard.
> gcc/ChangeLog:
>
> * tree-phinodes.cc (MIN_PHI_ARGS): New constant.
> (allocate_phi_node): Change from hard-coded value 2 to MIN_PHI_ARGS,
> which is now 4.
> (ideal_phi_node_len): Likewise.
> (release_phi_node): Likewise.
> ---
> gcc/tree-phinodes.cc | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/gcc/tree-phinodes.cc b/gcc/tree-phinodes.cc
> index 5a7e4a94e57..9d8e16ac200 100644
> --- a/gcc/tree-phinodes.cc
> +++ b/gcc/tree-phinodes.cc
> @@ -63,11 +63,12 @@ along with GCC; see the file COPYING3. If not see
> walking the elements of the last array entry would result in finding less
> than .1% additional reusable PHI nodes.
>
> - Note that we can never have less than two PHI argument slots. Thus,
> - the -2 on all the calculations below. */
> + Note that we can never have less than MIN_PHI_ARGS argument slots. Thus,
> + the subtraction of MIN_PHI_ARGS on all the calculations below. */
>
> #define NUM_BUCKETS 10
> -static GTY ((deletable (""))) vec<gimple *, va_gc>
> *free_phinodes[NUM_BUCKETS - 2];
> +#define MIN_PHI_ARGS 4
> +static GTY ((deletable (""))) vec<gimple *, va_gc>
> *free_phinodes[NUM_BUCKETS - MIN_PHI_ARGS];
> static unsigned long free_phinode_count;
>
> static int ideal_phi_node_len (int);
> @@ -94,17 +95,18 @@ static inline gphi *
> allocate_phi_node (size_t len)
> {
> gphi *phi;
> - size_t bucket = NUM_BUCKETS - 2;
> + size_t bucket = NUM_BUCKETS - MIN_PHI_ARGS;
> size_t size = sizeof (struct gphi)
> + (len - 1) * sizeof (struct phi_arg_d);
>
> if (free_phinode_count)
> - for (bucket = len - 2; bucket < NUM_BUCKETS - 2; bucket++)
> + for (bucket = len - MIN_PHI_ARGS; bucket < NUM_BUCKETS - MIN_PHI_ARGS;
> + bucket++)
> if (free_phinodes[bucket])
> break;
>
> /* If our free list has an element, then use it. */
> - if (bucket < NUM_BUCKETS - 2
> + if (bucket < NUM_BUCKETS - MIN_PHI_ARGS
> && gimple_phi_capacity ((*free_phinodes[bucket])[0]) >= len)
> {
> free_phinode_count--;
> @@ -145,9 +147,8 @@ ideal_phi_node_len (int len)
> size_t size, new_size;
> int log2, new_len;
>
> - /* We do not support allocations of less than two PHI argument slots. */
> - if (len < 2)
> - len = 2;
> + /* We do not support allocations of less than MIN_PHI_ARGS argument slots.
> */
> + len = MAX (len, MIN_PHI_ARGS);
>
> /* Compute the number of bytes of the original request. */
> size = sizeof (struct gphi)
> @@ -225,14 +226,13 @@ release_phi_node (gimple *phi)
>
> /* Immediately return the memory to the allocator when we would
> only ever re-use it for a smaller size allocation. */
> - if (len - 2 >= NUM_BUCKETS - 2)
> + if (len >= NUM_BUCKETS)
> {
> ggc_free (phi);
> return;
> }
>
> - bucket = len > NUM_BUCKETS - 1 ? NUM_BUCKETS - 1 : len;
> - bucket -= 2;
> + bucket = len - MIN_PHI_ARGS;
> vec_safe_push (free_phinodes[bucket], phi);
> free_phinode_count++;
> }
