From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa3.mentor.iphmx.com (esa3.mentor.iphmx.com [68.232.137.180]) by sourceware.org (Postfix) with ESMTPS id 5B9DE3858C20 for ; Wed, 6 Jul 2022 14:00:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 5B9DE3858C20 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com X-IronPort-AV: E=Sophos;i="5.92,250,1650960000"; d="scan'208";a="78268784" Received: from orw-gwy-01-in.mentorg.com ([192.94.38.165]) by esa3.mentor.iphmx.com with ESMTP; 06 Jul 2022 06:00:11 -0800 IronPort-SDR: 1FgWWqlGR37HanMFoU5OxW919VMR/5WcaEROfWuKVb0hipyt1TVO9AncHx1KBrfzptXE0aEDaV HVwXXfCPBycBGJ4GBMjcJ/oqdrXGx6Mz1HGR3CuKSv7JKMI7wtaFdeSayh9ZW8KAQA4tBuzzw4 t85QgYM0XIgukdqpDqxH13Qv71TA4oO3Z7jBtTkrpAXeMBAYV8RS7mED58hFX6PZgfMihIP3Ps D7G0ns96VYWZ57eOLqeSA0jiyB05GJKZXy4ADOfpag8FZti6GBjN1bHv9hgE+0LMeA0BZ6PRPP +0g= Message-ID: <1f1aea0a-dbe8-7b95-6bee-9d157faa320e@codesourcery.com> Date: Wed, 6 Jul 2022 15:59:59 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: Restore 'GOMP_offload_unregister_ver' functionality (was: [Patch][v5] OpenMP: Move omp requires checks to libgomp) Content-Language: en-US To: Thomas Schwinge , CC: Jakub Jelinek References: <07fec82a-41cf-fdc5-6307-c068dd95ef1a@mentor.com> <7f9c91c1-a479-f94f-ac14-1d6827ce671b@codesourcery.com> <5576fa00-0ddd-8046-17c1-d1cea82bdcf5@codesourcery.com> <77331328-4961-9dab-db58-b5b03daf218c@codesourcery.com> <16ca2aa4-7e73-cf9d-9482-dd59f5b0cdae@codesourcery.com> <87tu7u4j4r.fsf@euler.schwinge.homeip.net> From: Tobias Burnus In-Reply-To: <87tu7u4j4r.fsf@euler.schwinge.homeip.net> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: quoted-printable X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-02.mgc.mentorg.com (139.181.222.2) To svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 06 Jul 2022 14:00:18 -0000 Hi Thomas, hello all, On 06.07.22 12:42, Thomas Schwinge wrote: > On 2022-07-01T15:06:05+0200, Tobias Burnus > wrote: >> Attached is the updated patch. Main changes: [...] > This is now a great implementation of cross-component communication > (host/offloading compilers, runtime), thanks! I'm sure this will be > usable (or at least instructing) for further purposes, too. I also see potential use for other tools. >> - Uses GOMP_register_var to pass the mask to libgomp > Like 'GOMP_offload_register_ver', also 'GOMP_offload_unregister_ver' > needs to be adjusted correspondingly. Ups! Thanks for catching it and the patch. > OK to push the attached Fixing the data handling in GOMP_offload_unregister_ver, i.e. the 'target_data =3D &((void **) data)[1];' bit, is obvious and a good bug fix. Regarding the renaming of (..._)'target_data' to (..._)'data', I do not have a real opinion as I also regard the requires part as being part of the target(-related) data. Thus, I don't think it harms but I also do not think that there is a large benefit. Regarding the assert ... (continued below) > gcc/ > * config/gcn/mkoffload.cc (process_obj): Clarify 'target_data' -> > '[...]_data'. > * config/nvptx/mkoffload.cc (process): Likewise. > libgomp/ > * target.c (GOMP_offload_register_ver): Clarify 'target_data' -> > 'data'. > (GOMP_offload_unregister_ver): Likewise. Fix up 'target_data', > and add 'assert'. ... > diff --git a/gcc/config/gcn/mkoffload.cc b/gcc/config/gcn/mkoffload.cc > index b8b3fecfcb4..d2464332275 100644 > --- a/gcc/config/gcn/mkoffload.cc > +++ b/gcc/config/gcn/mkoffload.cc > @@ -692,13 +692,13 @@ process_obj (FILE *in, FILE *cfile, uint32_t omp_re= quires) > len); > > fprintf (cfile, > - "static const struct gcn_image_desc {\n" > + "static const struct gcn_data {\n" > " uintptr_t omp_requires_mask;\n" > " const struct gcn_image *gcn_image;\n" > " unsigned kernel_count;\n" > " const struct hsa_kernel_description *kernel_infos;\n" > " unsigned global_variable_count;\n" > - "} target_data =3D {\n" > + "} gcn_data =3D {\n" > " %d,\n" > " &gcn_image,\n" > " sizeof (gcn_kernels) / sizeof (gcn_kernels[0]),\n" > @@ -723,7 +723,7 @@ process_obj (FILE *in, FILE *cfile, uint32_t omp_requ= ires) > fprintf (cfile, "static __attribute__((constructor)) void init (void)= \n" > "{\n" > " GOMP_offload_register_ver (%#x, __OFFLOAD_TABLE__," > - " %d/*GCN*/, &target_data);\n" > + " %d/*GCN*/, &gcn_data);\n" > "};\n", > GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_GCN), > GOMP_DEVICE_GCN); > @@ -731,7 +731,7 @@ process_obj (FILE *in, FILE *cfile, uint32_t omp_requ= ires) > fprintf (cfile, "static __attribute__((destructor)) void fini (void)\= n" > "{\n" > " GOMP_offload_unregister_ver (%#x, __OFFLOAD_TABLE__," > - " %d/*GCN*/, &target_data);\n" > + " %d/*GCN*/, &gcn_data);\n" > "};\n", > GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_GCN), > GOMP_DEVICE_GCN); > diff --git a/gcc/config/nvptx/mkoffload.cc b/gcc/config/nvptx/mkoffload.c= c > index d8c81eb0547..0fa5f4423bf 100644 > --- a/gcc/config/nvptx/mkoffload.cc > +++ b/gcc/config/nvptx/mkoffload.cc > @@ -310,7 +310,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires) > fprintf (out, "\n};\n\n"); > > fprintf (out, > - "static const struct nvptx_tdata {\n" > + "static const struct nvptx_data {\n" > " uintptr_t omp_requires_mask;\n" > " const struct ptx_obj *ptx_objs;\n" > " unsigned ptx_num;\n" > @@ -318,7 +318,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires) > " unsigned var_num;\n" > " const struct nvptx_fn *fn_names;\n" > " unsigned fn_num;\n" > - "} target_data =3D {\n" > + "} nvptx_data =3D {\n" > " %d, ptx_objs, sizeof (ptx_objs) / sizeof (ptx_objs[0]),\n" > " var_mappings," > " sizeof (var_mappings) / sizeof (var_mappings[0]),\n" > @@ -344,7 +344,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires) > fprintf (out, "static __attribute__((constructor)) void init (void)\n= " > "{\n" > " GOMP_offload_register_ver (%#x, __OFFLOAD_TABLE__," > - " %d/*NVIDIA_PTX*/, &target_data);\n" > + " %d/*NVIDIA_PTX*/, &nvptx_data);\n" > "};\n", > GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_NVIDIA_PTX), > GOMP_DEVICE_NVIDIA_PTX); > @@ -352,7 +352,7 @@ process (FILE *in, FILE *out, uint32_t omp_requires) > fprintf (out, "static __attribute__((destructor)) void fini (void)\n" > "{\n" > " GOMP_offload_unregister_ver (%#x, __OFFLOAD_TABLE__," > - " %d/*NVIDIA_PTX*/, &target_data);\n" > + " %d/*NVIDIA_PTX*/, &nvptx_data);\n" > "};\n", > GOMP_VERSION_PACK (GOMP_VERSION, GOMP_VERSION_NVIDIA_PTX), > GOMP_DEVICE_NVIDIA_PTX); > diff --git a/libgomp/target.c b/libgomp/target.c > index 4dac81862d7..288b748b9e8 100644 > --- a/libgomp/target.c > +++ b/libgomp/target.c > @@ -2334,23 +2334,29 @@ gomp_requires_to_name (char *buf, size_t size, in= t requires_mask) > > /* This function should be called from every offload image while loadin= g. > It gets the descriptor of the host func and var tables HOST_TABLE, T= YPE of > - the target, and TARGET_DATA needed by target plugin. */ > + the target, and DATA. */ > > void > GOMP_offload_register_ver (unsigned version, const void *host_table, > - int target_type, const void *target_data) > + int target_type, const void *data) > { > int i; > - int omp_req =3D 0; > > if (GOMP_VERSION_LIB (version) > GOMP_VERSION) > gomp_fatal ("Library too old for offload (version %u < %u)", > GOMP_VERSION, GOMP_VERSION_LIB (version)); > > + int omp_req; > + const void *target_data; > if (GOMP_VERSION_LIB (version) > 1) > { > - omp_req =3D (int) (size_t) ((void **) target_data)[0]; > - target_data =3D &((void **) target_data)[1]; > + omp_req =3D (int) (size_t) ((void **) data)[0]; > + target_data =3D &((void **) data)[1]; > + } > + else > + { > + omp_req =3D 0; > + target_data =3D data; > } > > gomp_mutex_lock (®ister_lock); > @@ -2413,14 +2419,24 @@ GOMP_offload_register (const void *host_table, in= t target_type, > > /* This function should be called from every offload image while unload= ing. > It gets the descriptor of the host func and var tables HOST_TABLE, T= YPE of > - the target, and TARGET_DATA needed by target plugin. */ > + the target, and DATA. */ > > void > GOMP_offload_unregister_ver (unsigned version, const void *host_table, > - int target_type, const void *target_data) > + int target_type, const void *data) > { > int i; > > + if (GOMP_VERSION_LIB (version) > GOMP_VERSION) > + gomp_fatal ("Library too old for offload (version %u < %u)", > + GOMP_VERSION, GOMP_VERSION_LIB (version)); > + > + const void *target_data; > + if (GOMP_VERSION_LIB (version) > 1) > + target_data =3D &((void **) data)[1]; > + else > + target_data =3D data; > + > gomp_mutex_lock (®ister_lock); > > /* Unload image from all initialized devices. */ > @@ -2436,12 +2452,15 @@ GOMP_offload_unregister_ver (unsigned version, co= nst void *host_table, > } > > /* Remove image from array of pending images. */ > + bool found =3D false; > for (i =3D 0; i < num_offload_images; i++) > if (offload_images[i].target_data =3D=3D target_data) > { > offload_images[i] =3D offload_images[--num_offload_images]; > + found =3D true; > break; > } > + assert (found); > > gomp_mutex_unlock (®ister_lock); > } ... I don't like that libgomp crashes without any helpful message in that c= ase. In my opinion: * Either we assume that it is unlikely to occur - ignore it. (Matches the current implementation: do nothing.) * Or we want to have some diagnostic in case it occurs. But in that case, it should be some explicit diagnostic printed by gomp_error or gomp_fata= l. IMHO, gomp_error is better than gomp_fatal as libgomp then continues cle= aning up after this error, which IMHO makes more sense that just aborting. To conclude, I think the '&((void **) data)[1]' shall go in - it is an obvi= ous bugfix! While the 'data' change is IMHO fine (but not doing it is also fine= . And, for the assert change, I think it shouldn't be done - either keep it a= s is or use a gomp_error instead. Thanks, Tobias ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstra=C3=9Fe 201= , 80634 M=C3=BCnchen; Gesellschaft mit beschr=C3=A4nkter Haftung; Gesch=C3= =A4ftsf=C3=BChrer: Thomas Heurung, Frank Th=C3=BCrauf; Sitz der Gesellschaf= t: M=C3=BCnchen; Registergericht M=C3=BCnchen, HRB 106955