On Wed, Jul 31, 2024 at 02:58:34PM +0000, Prathamesh Kulkarni wrote:
> There are a couple of issues in the patch:
> (1) The patch streams out NUM_POLY_INT_COEFFS at beginning of mode_table,
> which should work for bp_unpack_poly_value,
> (since AFAIK, it's only called by lto_input_mode_table). However, I am not
> sure if we will always call lto_input_mode_table
> before streaming in poly_int64 / poly_uint64 ? Or should we stream out host
> NUM_POLY_INT_COEFFS at a different place in LTO bytecode ?
The poly_ints unpacked in lto_input_mode_table obviously are done after
that.
If you use it for streaming in from other sections, you need to check if
they can't be read before the mode table.
And, you don't really need to stream out/in the number for non-offloading
LTO, that should use just NUM_POLY_INT_COEFFS.
> --- a/gcc/data-streamer-in.cc
> +++ b/gcc/data-streamer-in.cc
> @@ -183,9 +183,7 @@ poly_uint64
> streamer_read_poly_uint64 (class lto_input_block *ib)
> {
> poly_uint64 res;
> - for (unsigned int i = 0; i < NUM_POLY_INT_COEFFS; ++i)
> - res.coeffs[i] = streamer_read_uhwi (ib);
> - return res;
> + POLY_INT_READ_COMMON(res, streamer_read_uhwi (ib))
Why is this macro and not an inline function or inline function template
oor inline function calling a lambda?
Even if it has to be a macro (I don't see why), it should be defined such
that you need to add ; at the end, ideally not include the return res;
in there because it is just too weird if used like that (or make it return
what will be returned and use return POLY_INT_READ_COMMON...)
and there needs to be a space in between COMMON and (.
> @@ -194,9 +192,7 @@ poly_int64
> streamer_read_poly_int64 (class lto_input_block *ib)
> {
> poly_int64 res;
> - for (unsigned int i = 0; i < NUM_POLY_INT_COEFFS; ++i)
> - res.coeffs[i] = streamer_read_hwi (ib);
> - return res;
> + POLY_INT_READ_COMMON(res, streamer_read_hwi (ib))
> }
Ditto.
> + __typeof(x.coeffs[0]) val = streamer_read_coeff; \
You certainly can't use a GCC extension like __typeof here.
Plus missing space.
> + if (val != 0) \
> + fatal_error (input_location, \
> + "Degree of %<poly_int%> exceeds " \
Diagnostics shouldn't start with uppercase letter.
> + "%<NUM_POLY_INT_COEFFS%> (%d)", \
> + NUM_POLY_INT_COEFFS); \
> + } \
> + }
> \
> + \
> + return x; \
> +}
> +
> --- a/gcc/poly-int.h
> +++ b/gcc/poly-int.h
> @@ -354,6 +354,10 @@ struct poly_result<T1, T2, 2>
> ? (void) ((RES).coeffs[I] = VALUE) \
> : (void) ((RES).coeffs[I].~C (), new (&(RES).coeffs[I]) C (VALUE)))
>
> +/* Number of bits needed to represent maximum value of
> + NUM_POLY_INT_COEFFS defined by any target. */
> +#define MAX_NUM_POLY_INT_COEFFS_BITS (2)
Why (2) and not just 2?
There should be some static_assert to make sure it is a maximum for any
target.
> + if (!integer_zerop (val))
> + fatal_error (input_location,
> + "Degree of %<poly_int%> exceeds "
Again.
> + "%<NUM_POLY_INT_COEFFS%>");
> + }
> + }
> }
Jakub
