Richard Biener <rguent...@suse.de> writes: > On Tue, 30 Jul 2024, Prathamesh Kulkarni wrote: > >> >> >> > -----Original Message----- >> > From: Richard Sandiford <richard.sandif...@arm.com> >> > Sent: Monday, July 29, 2024 9:43 PM >> > To: Richard Biener <rguent...@suse.de> >> > Cc: Prathamesh Kulkarni <prathame...@nvidia.com>; gcc- >> > patc...@gcc.gnu.org >> > Subject: Re: Support streaming of poly_int for offloading when it's >> > degree <= accel's NUM_POLY_INT_COEFFS >> > >> > External email: Use caution opening links or attachments >> > >> > >> > 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 :) >> Hi Richard, >> The patch doesn't stream out NUM_POLY_INT_COEFFS, but the degree of poly_int >> (and streams-out coeffs only up to degree, ignoring the higher zero coeffs). >> During streaming-in, it reads back the degree (and streamed coeffs upto >> degree) and issues an error if degree > accel's NUM_POLY_INT_COEFFS, since >> we can't >> (as-is) represent a degree-N poly_int on accel with NUM_POLY_INT_COEFFS < N. >> If degree < accel's NUM_POLY_INT_COEFFS, the remaining coeffs are set to 0 >> (similar to zero-extension). I posted more details in RFC: >> https://gcc.gnu.org/pipermail/gcc/2024-July/244466.html
It's not clear to me what the plan is for VLA host + VLS offloading. Is the streamed data guaranteed to be "clean" of any host-only VLA stuff? E.g. if code does: #include <arm_sve.h> svint32_t *ptr: void foo(svint32_t); #pragma GCC target "+nosve" ...offloading... is there a guarantee that the offload target won't see the definition of ptr and foo? >> >> The attached patch defines MAX_NUM_POLY_INT_COEFFS_BITS in poly-int.h to >> represent number of bits needed for max value of NUM_POLY_INT_COEFFS defined >> by any target, >> and uses that for packing/unpacking degree of poly_int to/from bitstream, >> which should make it independent of the type used for representing >> NUM_POLY_INT_COEFFS by >> the target. > > Just as additional comment - maybe we can avoid the POLY_INT_CST tree > side if we'd consistently "canonicalize" a POLY_INT_CST with zero > second coeff as INTEGER_CST instead? This of course doesn't > generalize to NUM_POLY_INT_COEFFS == 3 vs NUM_POLY_INT_COEFFS == 2. That should already happen, via: tree wide_int_to_tree (tree type, const poly_wide_int_ref &value) { if (value.is_constant ()) return wide_int_to_tree_1 (type, value.coeffs[0]); return build_poly_int_cst (type, value); } etc. So if we see POLY_INT_CSTs that could be INTEGER_CSTs, I think that'd be a bug. Thanks, Richard