Richard Biener <rguent...@suse.de> writes:
> On Mon, 29 Jul 2024, Prathamesh Kulkarni wrote:
>
>> Hi Richard,
>> Thanks for your suggestions on RFC email, the attached patch adds support 
>> for streaming of poly_int when it's degree <= accel's NUM_POLY_INT_COEFFS.
>> The patch changes streaming of poly_int as follows:
>> 
>> Streaming out poly_int:
>> 
>> degree = poly_int.degree();
>> stream out degree;
>> for (i = 0; i < degree; i++)
>>   stream out poly_int.coeffs[i];
>> 
>> Streaming in poly_int:
>> 
>> stream in degree;
>> if (degree > NUM_POLY_INT_COEFFS)
>>   fatal_error();
>> stream in coeffs;
>> // Set remaining coeffs to zero in case degree < accel's NUM_POLY_INT_COEFFS
>> for (i = degree; i < NUM_POLY_INT_COEFFS; i++)
>>   poly_int.coeffs[i] = 0;
>> 
>> Patch passes bootstrap+test and LTO bootstrap+test on aarch64-linux-gnu.
>> LTO bootstrap+test on x86_64-linux-gnu in progress.
>> 
>> I am not quite sure how to test it for offloading since currently it's 
>> (entirely) broken for aarch64->nvptx.
>> I can give a try with x86_64->nvptx offloading if required (altho I guess 
>> LTO bootstrap should test streaming changes ?)
>
> +  unsigned degree
> +    = bp_unpack_value (bp, BITS_PER_UNIT * sizeof (unsigned
> HOST_WIDE_INT));
>
> The NUM_POLY_INT_COEFFS target define doesn't seem to be constrained
> to any type it needs to fit into, using HOST_WIDE_INT is arbitrary.
> I'd say we should constrain it to a reasonable upper bound,
> like 2?  Maybe even have MAX_NUM_POLY_INT_COEFFS or 
> NUM_POLY_INT_COEFFS_BITS in poly-int.h and constrain NUM_POLY_INT_COEFFS.
>
> The patch looks reasonable over all, but Richard S. should have a say
> about the abstraction you chose and the poly-int adjustment.

Sorry if this has been discussed already, but could we instead stream
NUM_POLY_INT_COEFFS once per file, rather than once per poly_int?
It's a target invariant, and poly_int has wormed its way into lots
of things by now :)

Thanks,
Richard

Reply via email to