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.

Thanks,
Richard.


> Signed-off-by: Prathamesh Kulkarni <prathame...@nvidia.com>
> 
> Thanks,
> Prathamesh
> 

-- 
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)

Reply via email to