Prathamesh Kulkarni <prathame...@nvidia.com> writes: >> -----Original Message----- >> From: Richard Biener <rguent...@suse.de> >> Sent: Wednesday, August 21, 2024 5:09 PM >> To: Prathamesh Kulkarni <prathame...@nvidia.com> >> Cc: Richard Sandiford <richard.sandif...@arm.com>; Thomas Schwinge >> <tschwi...@baylibre.com>; gcc-patches@gcc.gnu.org >> Subject: RE: Re-compute TYPE_MODE and DECL_MODE while streaming in for >> accelerator >> >> External email: Use caution opening links or attachments >> >> >> On Wed, 21 Aug 2024, Prathamesh Kulkarni wrote: >> >> > >> > >> > > -----Original Message----- >> > > From: Richard Biener <rguent...@suse.de> >> > > Sent: Tuesday, August 20, 2024 10:36 AM >> > > To: Richard Sandiford <richard.sandif...@arm.com> >> > > Cc: Prathamesh Kulkarni <prathame...@nvidia.com>; Thomas Schwinge >> > > <tschwi...@baylibre.com>; gcc-patches@gcc.gnu.org >> > > Subject: Re: Re-compute TYPE_MODE and DECL_MODE while streaming in >> > > for accelerator >> > > >> > > External email: Use caution opening links or attachments >> > > >> > > >> > > > Am 19.08.2024 um 20:56 schrieb Richard Sandiford >> > > <richard.sandif...@arm.com>: >> > > > >> > > > Prathamesh Kulkarni <prathame...@nvidia.com> writes: >> > > >> diff --git a/gcc/lto-streamer-in.cc b/gcc/lto-streamer-in.cc >> > > >> index >> > > >> cbf6041fd68..0420183faf8 100644 >> > > >> --- a/gcc/lto-streamer-in.cc >> > > >> +++ b/gcc/lto-streamer-in.cc >> > > >> @@ -44,6 +44,7 @@ along with GCC; see the file COPYING3. If >> not >> > > see >> > > >> #include "debug.h" >> > > >> #include "alloc-pool.h" >> > > >> #include "toplev.h" >> > > >> +#include "stor-layout.h" >> > > >> >> > > >> /* Allocator used to hold string slot entries for line map >> > > streaming. >> > > >> */ static struct object_allocator<struct string_slot> >> > > >> *string_slot_allocator; @@ -1752,6 +1753,17 @@ lto_read_tree_1 >> > > (class lto_input_block *ib, class data_in *data_in, tree expr) >> > > >> with -g1, see for example PR113488. */ >> > > >> else if (DECL_P (expr) && DECL_ABSTRACT_ORIGIN (expr) == >> > > expr) >> > > >> DECL_ABSTRACT_ORIGIN (expr) = NULL_TREE; >> > > >> + >> > > >> +#ifdef ACCEL_COMPILER >> > > >> + /* For decl with aggregate type, host streams out >> VOIDmode. >> > > >> + Compute the correct DECL_MODE by calling relayout_decl. >> */ >> > > >> + if ((VAR_P (expr) >> > > >> + || TREE_CODE (expr) == PARM_DECL >> > > >> + || TREE_CODE (expr) == FIELD_DECL) >> > > >> + && AGGREGATE_TYPE_P (TREE_TYPE (expr)) >> > > >> + && DECL_MODE (expr) == VOIDmode) >> > > >> + relayout_decl (expr); >> > > >> +#endif >> > > > >> > > > Genuine question, but: is relayout_decl safe in this context? >> It >> > > does >> > > > a lot more than just reset the mode. It also applies the target >> > > ABI's >> > > > preferences wrt alignment, padding, and so on, rather than >> > > preserving >> > > > those of the host's. >> > > >> > > It would be better to just recompute the mode here. >> > Hi, >> > The attached patch sets DECL_MODE (expr) to TYPE_MODE (TREE_TYPE >> (expr)) in lto_read_tree_1 instead of calling relayout_decl (expr). >> > I checked layout_decl_type does the same thing for setting decl >> mode, >> > except for bit fields. Since bit-fields cannot have aggregate type, >> I am assuming setting DECL_MODE (expr) to TYPE_MODE (TREE_TYPE (expr)) >> would be OK in this case ? >> >> Yep, that should work. > Thanks, I have committed the patch in: > https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=792adb8d222d0d1d16b182871e105f47823b8e72 > > after verifying it passes bootstrap+test on aarch64-linux-gnu, > and libgomp testing (without GPU) for aarch64->nvptx and x86_64->nvptx. >> >> > Sorry if this sounds like a silly ques -- Why would it be unsafe to >> > call relayout_decl for variables that are mapped to accelerator even >> > if it'd not preserve host's properties ? I assumed we want to assign >> accel's ABI properties for mapped decls (mode being one of them), or >> am I misunderstanding ? >> >> Structure layout need not be compatible but we are preserving that of >> the host instead of re-layouting in target context. Likewise type <-> >> mode mapping doesn't have to agree. > Ah OK, thanks for clarifying. So IIUC, in future, we might need to change > that if > (in theory), host's structure layout for a decl is incompatible with a > particular accel's ABI > and will need to relayout in accel's context ?
If structures are ever used to communicate between the host and the accelerator, they would need to be laid out as the host expects, otherwise we'd get data corruption. But maybe structures are never used that way (it's not my area!). Richard
