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 ED17B384D165 for ; Wed, 29 Jun 2022 18:10:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org ED17B384D165 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,231,1650960000"; d="scan'208";a="77942592" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa3.mentor.iphmx.com with ESMTP; 29 Jun 2022 10:10:15 -0800 IronPort-SDR: yQJyRTs8LaAhpW6yWSFBn9i8ApkrbMACfvkxIPPbikUcUOEYdIl/LikfTWW4apPNeBziTcZgoL hCzur6IRetdQbGfQolkICXAXedUewYpy6F9bpbNJ5lcDWX50zkDRqCODk2q8RxceMpbcg47rrF iqnh505zginCa/mKwcNTsIwFM7XYYE2yyW9d+b+nRuRdNRTxnS2KfVy07CywbMCoO9jjAbPC71 LQOAHFqsx6H3tD1mBakYHfevjCdWYTKMHbgsehMDiH7zlllHAYIOR75YnRyYv6jQaKKLg8ca5O pOo= Message-ID: <77331328-4961-9dab-db58-b5b03daf218c@codesourcery.com> Date: Wed, 29 Jun 2022 20:10:10 +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: [Patch][v4] OpenMP: Move omp requires checks to libgomp Content-Language: en-US To: Jakub Jelinek CC: gcc-patches References: <07fec82a-41cf-fdc5-6307-c068dd95ef1a@mentor.com> <7f9c91c1-a479-f94f-ac14-1d6827ce671b@codesourcery.com> <5576fa00-0ddd-8046-17c1-d1cea82bdcf5@codesourcery.com> From: Tobias Burnus In-Reply-To: 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-09.mgc.mentorg.com (139.181.222.9) To svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) X-Spam-Status: No, score=-5.4 required=5.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS, KAM_DMARC_STATUS, NICE_REPLY_A, SPF_HELO_PASS, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=no 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, 29 Jun 2022 18:10:18 -0000 Hi Jakub, On 29.06.22 19:02, Jakub Jelinek wrote: > On Wed, Jun 29, 2022 at 04:33:02PM +0200, Tobias Burnus wrote: >> + if (flag_openmp >> + && lookup_attribute ("omp declare target", >> + DECL_ATTRIBUTES (current_function_decl))) >> + omp_requires_mask >> + =3D (enum omp_requires) (omp_requires_mask | OMP_REQUIRES_TARGET_= USED); > I thought the above would be left out, at least until clarified what exac= tly > we mean with device routines in the restrictions. Missed that =E2=80=93 I thought mostly of the API calls. However, I concur = that for 'declare target', it is not really needed as no data transfers or reverse offloads can happen. - Additionally, I took this part from Chung-Lin's patch and did not really think about this part ... >> --- a/gcc/cp/parser.cc >> +++ b/gcc/cp/parser.cc > And the above too. > > On the Fortran side I don't see it being done. (Likewise.) >> --- a/gcc/lto-cgraph.cc >> +++ b/gcc/lto-cgraph.cc >> ... >> + if (output_requires) >> + { >> + HOST_WIDE_INT val =3D ((HOST_WIDE_INT) omp_requires_mask >> + & (OMP_REQUIRES_UNIFIED_ADDRESS >> + | OMP_REQUIRES_UNIFIED_SHARED_MEMORY >> + | OMP_REQUIRES_REVERSE_OFFLOAD >> + | OMP_REQUIRES_TARGET_USED)); > Why is the OMP_REQUIRES_TARGET_USED bit saved there? It is always set > if output_requires... > If we want to make sure it is set in omp_requires_mask after streaming > in, we can just or it in back there. It is there because it is later needed: With -flto, we otherwise wouldn't generate the variable in omp-offload.cc. And as this value is only used internally, I thought it could just stay there. However, it could also be excluded here and ... >> @@ -1811,6 +1844,24 @@ input_offload_tables (bool do_force_output) >> if (do_force_output) >> varpool_node::get (var_decl)->force_output =3D 1; >> } >> + else if (tag =3D=3D LTO_symtab_edge) >> + { >> + static bool error_emitted =3D false; >> + HOST_WIDE_INT val =3D streamer_read_hwi (ib); >> + >> + if (omp_requires_mask =3D=3D 0) >> + omp_requires_mask =3D (omp_requires) val; ... added here as: > I mean here: (omp_requires) (val | OMP_REQUIRES_TARGET_USED); ... but that also requires ... >> + else if (omp_requires_mask !=3D val && !error_emitted) ... something like: (omp_requires_mask & ~OMP_REQUIRES_TARGET_USED) !=3D val or something like that. >> + { >> + char buf[64], buf2[64]; >> + omp_requires_to_name (buf, sizeof (buf), omp_requires_mas= k); >> + omp_requires_to_name (buf2, sizeof (buf2), val); >> + error ("OpenMP % directive with non-identical = " >> + "clauses in multiple compilation units: %qs vs. %q= s", >> + buf, buf2); > I think the user should be told also where, so file_data->file_name With -save-temps, the filename is indeed helpful, e.g.: "a-requires-1.o". However, without, I get names like: "/tmp/ccIgRkOW.o". As mentioned, I was thinking of storing after the value some location data, e.g. DECL_SOURCE_FILE() or even DECL_SOURCE_LOCATION() =E2=80=93 by keeping track of the first 'omp requires' in a translation unit. Those can be streamed with streamer_write_string and lto_output_location. However, the both require as argument an "struct output_block" and in lto-cgraph.cc, I only have a "struct lto_simple_output_block". And for reading, I additionally need a "class data_in" argument. Thus, I think it is doable =E2=80=93 however, I was not quite sure whether = it made sense to do all the effort or not. (And it was not immediately clear to me how to create a "data_in" class and ...) > Can't we arrange for GOMP_offload_register_ver to see the bitmasks somewh= ere > instead (and force GOMP_offload_register_ver even if there are no vars or > funcs and just the requires mask)? This probably works =E2=80=93 but means some restructuring. GOMP_offload_register_ver assumes that num_devices is already available =E2=80=93 and we need to exclude those for which the 'omp requires' cannot = be fulfilled. I think this could be either be done in GOMP_offload_register_ver by decrementing num_offload_images + shifting the offload_images[i] entries (or have some table to match user-visible numbers to the original number) . Or it could just be done by setting a flag =E2=80=93 and num_offload_images updated later. We probably need something which is run later to exclude those devices for which no image exists (existing but not supported devices) =E2=80=93 and for OpenMP 6's OMP_AVAILABLE_DEVICES env, which perm= its to sort the devices and filter out some of them. > Could be the weak __offload_requires_mask (but we probably e.g. can't ass= ume > declare_weak will work), I assume that it does work in practice, given that it is only needed on the host =E2=80=93 and given which systems effectively support offloading. = =E2=80=93 With -flto, we even would know that there is only one variable, but unfortunately, we cannot count on a host lto1 run. > Or we could ask for the offloading lto1 when it merges those from differe= nt > offloadng TUs to emit some magic symbol in what it emits and have mkofflo= ad > search for it. > > Or mkoffload could pass some option to the offloading lto compilation, sa= y > filename of a temporary file, let lto1 save into that file the bitmask an= d > let mkoffload read it. Or env var. (Can surely be done =E2=80=93 having a constant in the GOMP_offload_registe= r_ver call would be surely useful.) >> --- a/libgomp/libgomp-plugin.h >> +++ b/libgomp/libgomp-plugin.h >> ... >> -extern int GOMP_OFFLOAD_get_num_devices (void); >> +extern int GOMP_OFFLOAD_get_num_devices (unsigned int); > This is an ABI change for plugins, don't we want to e.g. rename > the symbol as well, so that plugin mismatches are caught more easily? Yes, I recall that we talked about it, but I obviously missed to actually change it. If we go for GOMP_offload_register_ver + updating the list later, this function can probably stay as is =E2=80=93 and we need another func to query whether the requirements are fulfillable or not. 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