From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from esa4.mentor.iphmx.com (esa4.mentor.iphmx.com [68.232.137.252]) by sourceware.org (Postfix) with ESMTPS id 1A06C385841F; Tue, 4 Oct 2022 12:26:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 1A06C385841F 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.93,157,1654588800"; d="diff'?scan'208,217";a="84081478" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa4.mentor.iphmx.com with ESMTP; 04 Oct 2022 04:26:21 -0800 IronPort-SDR: pSwtD0zjNL6LwVhRDDS5tsXG6D94DIBEu5ALywQZmOdBgiwh7Y96aGERLH2dXaOOXI4FVW3Mcr QpHoP+kmxFkj1yTCHZ2rokkgAgWqSwo8ccoykIRHNr09bBdVrz6YLANgsi8afy6p9pBZUvwawN VT9CQHrNayOsDtQpgFOJsfHWmwWOv9QTCXWo7xNIt7Tt+ajEzx2CzSX526pLts2HIcbG/yC3hH ydQAvjoDmdZBWrC/vVJ8xnNpT1ADenxGndyhq8thQCv+JL2extmBBLJfs/l32BZ46XWrxFkuo4 pmk= Content-Type: multipart/mixed; boundary="------------1FA9GsTkjkmhKpyQI9FJm9qH" Message-ID: Date: Tue, 4 Oct 2022 14:26:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.1 Subject: Re: [Patch] Fortran: Add OpenMP's assume(s) directives Content-Language: en-US To: Jakub Jelinek CC: gcc-patches , fortran References: From: Tobias Burnus In-Reply-To: X-Originating-IP: [137.202.0.90] X-ClientProxiedBy: svr-ies-mbx-10.mgc.mentorg.com (139.181.222.10) To svr-ies-mbx-12.mgc.mentorg.com (139.181.222.12) X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,KAM_DMARC_STATUS,NICE_REPLY_A,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: --------------1FA9GsTkjkmhKpyQI9FJm9qH Content-Type: multipart/alternative; boundary="------------opOut2EXIybYy5cUi1ey1OQW" --------------opOut2EXIybYy5cUi1ey1OQW Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: quoted-printable Hi Jakub, On 04.10.22 12:19, Jakub Jelinek wrote: On Sun, Oct 02, 2022 at 07:47:18PM +0200, Tobias Burnus wrote: --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -2749,9 +2749,9 @@ have support for @option{-pthread}. @option{-fopenmp}= implies @opindex fopenmp-simd @cindex OpenMP SIMD @cindex SIMD -Enable handling of OpenMP's SIMD directives with @code{#pragma omp} -in C/C++ and @code{!$omp} in Fortran. Other OpenMP directives -are ignored. +Enable handling of OpenMP's SIMD directives and OPENMP's @code{assume} dir= ective s/OPENMP/OpenMP/ We actually handle more directives, @code{declare reduction}, @code{ordered}, @code{scan}, @code{loop} and combined/composite directives with @code{simd} as constituent. ... And now in C++ we handle also the attribute syntax (guess we should update the text for that here as well as in -fopenmp entry). Updated suggestion attached =E2=80=93 I still need to update the main patch. (I also added 'declare simd' to the list. And I updated Fortran for scan + = loop.) OK? * * * Wouldn't this be better table driven (like c_omp_directives in c-family, though guess for Fortran you can just use spaces in the name, don't need 3 strings for the separate tokens)? Because I think absent/contains isn't the only spot where you need directive names, metadirective is another. Maybe =E2=80=93 I think there are already way to many string repetitions. O= ne problem is that metadirectives permit combined/composite constructs whil= e 'assume(s)' does not. I on purpose did not parse them, but probably in li= ght of Metadirectives, I should. I will take a look. + if (is_omp_declarative_stmt (st) || is_omp_informational_stmt (st)) + { + gfc_error ("Invalid %qs directive at %L in %s clause: declarative= , " + "informational and meta directives not permitted", + gfc_ascii_statement (st, true), &old_loc, + is_absent ? "ABSENT" : "CONTAINS"); Do you think we should do the same for C/C++? Right now it doesn't differentiate between invalid directive names and names of declarative, informational or meta directives. Maybe - it might help users to understand why something went wrong, on the = other hand, I do not really think that a user adds 'absent(declare variant)= ', but I might be wrong. + =3D (gfc_statement *) xrealloc ((*assume)->absent, + sizeof (gfc_statement) + * (*assume)->n_absent); XRESIZEVEC? Aha, that's the macro name! But also, resizing each time a single entry is added to the list isn't good for compile time, would be nice to grow the allocation size in powers of 2 or so. I only expect a very small number =E2=80=93 and did not want to keep track = of yet another number. However, the following should work: if (old_n_absent =3D 0) absent =3D ... sizeof() * 1 else if (popcount (old_n_absent) =3D=3D 1) absent =3D ... sizeof() * (old_n_absent) * 2) that allocates: 1, 2, 4, 8, 16, 32, ... without keeping track of the number. +gfc_match_omp_assumes (void) +{ + locus loc =3D gfc_current_locus; + gfc_omp_clauses *c =3D gfc_get_omp_clauses (); + c->assume =3D gfc_current_ns->omp_assumes; + if (!gfc_current_ns->proc_name + || (gfc_current_ns->proc_name->attr.flavor !=3D FL_MODULE + && !gfc_current_ns->proc_name->attr.subroutine + && !gfc_current_ns->proc_name->attr.function)) + { + gfc_error ("!OMP ASSUMES at %C must be in the specification part of = a " + "subprogram or module"); + return MATCH_ERROR; + } + if (gfc_match_omp_clauses (&c, omp_mask (OMP_CLAUSE_ASSUMPTIONS), true, = true, + false, false, false, false) !=3D MATCH_YES) + { + gfc_current_ns->omp_assumes =3D NULL; + return MATCH_ERROR; + } I don't understand the point of preallocation of gfc_omp_clauses here, can't it be allocated inside of gfc_match_omp_clauses like everywhere else? Because otherwise it e.g. leaks if the first error is reported. This is supposed to handle: subroutine foo() !$omp assumes absent(target) !$omp assumes absent(teams) end I did not spot anything which states that it is invalid. (I might have missed it, however.) And if it is valid, I assume it is equivalent to: subroutine foo() !$omp assumes absent(target, teams) end And the simplest way to do the merge seems to use gfc_match_omp_clauses, which already handles merging 'absent(target) absent(teams)'. Thus, I pre-populate the clause list with the assumption values from the previous directive. Additionally, there shouldn't be a leak as inside gfc_omp_match_clauses is: gfc_free_omp_clauses (c); return MATCH_ERROR; which frees the memory. To avoid double freeing, a possibly pre-existing 'gfc_current_ns->omp_assumes' has to be set to NULL. The other question is whether the spec is clear, e.g. is the following vali= d? !$omp assumes no_openmp !$omp assumes no_openmp In each directive, no_openmp is unique but the combination is not (but it should be fine, here). While for !$omp assumes absent(target) !$omp assumes contains(target) there is surely an issue. + for (int i =3D 0; i < assume->n_contains; i++) + for (int j =3D i + 1; j < assume->n_contains; j++) + if (assume->contains[i] =3D=3D assume->contains[j]) + gfc_error ("%qs directive mentioned multiple times in %s clause in = %s " + "directive at %L", + gfc_ascii_statement (assume->contains[i], true), + "CONTAINS", directive, loc); This is O(n^2)? Can't you use a bitmap or hash map instead? How about adding a 'break; after the all the gfc_error instead? This turns O(n^2) into O(n) and I am pretty sure in the common case, it is much faster than using a hash or bitmap. Reason: There 38 permitted directives of which 7 are rejected at parse time, hence 31 remain. The worst case is to have as input: dir_1, dir_2, ..., dir_31, dir_31,... dir_31 Thus, there are '(n-1) + (n-2) + ... + (n-30) + 1' iterations until the first error is found, which is O(n*3O) =3D O(n). In the real world, I assume n <=3D 5 and it seems to be faster to do 4+3+2+1 =3D 10 comparisons rather than starting to construct a hash or a bitmap. However, if you think it still makes sense to create a bitmap or hash, I can do it. 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 --------------opOut2EXIybYy5cUi1ey1OQW-- --------------1FA9GsTkjkmhKpyQI9FJm9qH Content-Type: text/x-patch; charset="UTF-8"; name="update-fopenmp-simd.diff" Content-Disposition: attachment; filename="update-fopenmp-simd.diff" Content-Transfer-Encoding: base64 T3Blbk1QOiBVcGRhdGUgaW52b2tlLnRleGkgYW5kIGZpeCBmb3J0cmFuL3Bh cnNlLmNjIGZvciAtZm9wZW5tcC1zaW1kCgpTcGxpdCBvZmYgZnJvbSB0aGUg J0ZvcnRyYW46IEFkZCBPcGVuTVAncyBhc3N1bWUocykgZGlyZWN0aXZlcycg cGF0Y2guCgpnY2MvCgkqIGRvYy9pbnZva2UudGV4aSAoLWZvcGVubXApOiBN ZW50aW9uIEMrKyBhdHRyaWJ1dCBzeW50YXguCgkoLWZvcGVubXAtc2ltZCk6 IExpa2V3aXNlOyB1cGRhdGUgcGVybWl0dGVkIGRpcmVjdGl2ZXMuCgpnY2Mv Zm9ydHJhbi8KCSogcGFyc2UuY2MgKGRlY29kZV9vbXBfZGlyZWN0aXZlKTog SGFuZGxlICcoZW5kKSBsb29wJyBhbmQgJ3NjYW4nCglhbHNvIHdpdGggLWZv cGVubXAtc2ltZC4KCmdjYy90ZXN0c3VpdGUvCgkqIGdmb3J0cmFuLmRnL2dv bXAvb3Blbm1wLXNpbWQtNy5mOTA6IE5ldyB0ZXN0LgoKIGdjYy9kb2MvaW52 b2tlLnRleGkgICAgICAgICAgICAgICAgICAgICAgICAgICAgICB8IDEyICsr KysrKysrLS0tLQogZ2NjL2ZvcnRyYW4vcGFyc2UuY2MgICAgICAgICAgICAg ICAgICAgICAgICAgICAgIHwgIDYgKysrLS0tCiBnY2MvdGVzdHN1aXRlL2dm b3J0cmFuLmRnL2dvbXAvb3Blbm1wLXNpbWQtNy5mOTAgfCAyMyArKysrKysr KysrKysrKysrKysrKysrKwogMyBmaWxlcyBjaGFuZ2VkLCAzNCBpbnNlcnRp b25zKCspLCA3IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL2djYy9kb2Mv aW52b2tlLnRleGkgYi9nY2MvZG9jL2ludm9rZS50ZXhpCmluZGV4IGE1ZGM2 Mzc3ODM1Li5lMGMyYzU3YzliMiAxMDA2NDQKLS0tIGEvZ2NjL2RvYy9pbnZv a2UudGV4aQorKysgYi9nY2MvZG9jL2ludm9rZS50ZXhpCkBAIC0yNzM3LDcg KzI3MzcsOCBAQCBjYW4gYmUgb21pdHRlZCwgdG8gdXNlIGEgdGFyZ2V0LXNw ZWNpZmljIGRlZmF1bHQgdmFsdWUuCiBAaXRlbSAtZm9wZW5tcAogQG9waW5k ZXggZm9wZW5tcAogQGNpbmRleCBPcGVuTVAgcGFyYWxsZWwKLUVuYWJsZSBo YW5kbGluZyBvZiBPcGVuTVAgZGlyZWN0aXZlcyBAY29kZXsjcHJhZ21hIG9t cH0gaW4gQy9DKysgYW5kCitFbmFibGUgaGFuZGxpbmcgb2YgT3Blbk1QIGRp cmVjdGl2ZXMgQGNvZGV7I3ByYWdtYSBvbXB9IGluIEMvQysrLAorQGNvZGV7 W1tvbXA6OmRpcmVjdGl2ZSguLi4pXV19IGFuZCBAY29kZXtbW29tcDo6c2Vx dWVuY2UoLi4uKV1dfSBpbiBDKysgYW5kCiBAY29kZXshJG9tcH0gaW4gRm9y dHJhbi4gIFdoZW4gQG9wdGlvbnstZm9wZW5tcH0gaXMgc3BlY2lmaWVkLCB0 aGUKIGNvbXBpbGVyIGdlbmVyYXRlcyBwYXJhbGxlbCBjb2RlIGFjY29yZGlu ZyB0byB0aGUgT3Blbk1QIEFwcGxpY2F0aW9uCiBQcm9ncmFtIEludGVyZmFj ZSB2NC41IEB3e0B1cmVme2h0dHBzOi8vd3d3Lm9wZW5tcC5vcmd9fS4gIFRo aXMgb3B0aW9uCkBAIC0yNzQ5LDkgKzI3NTAsMTIgQEAgaGF2ZSBzdXBwb3J0 IGZvciBAb3B0aW9uey1wdGhyZWFkfS4gQG9wdGlvbnstZm9wZW5tcH0gaW1w bGllcwogQG9waW5kZXggZm9wZW5tcC1zaW1kCiBAY2luZGV4IE9wZW5NUCBT SU1ECiBAY2luZGV4IFNJTUQKLUVuYWJsZSBoYW5kbGluZyBvZiBPcGVuTVAn cyBTSU1EIGRpcmVjdGl2ZXMgd2l0aCBAY29kZXsjcHJhZ21hIG9tcH0KLWlu IEMvQysrIGFuZCBAY29kZXshJG9tcH0gaW4gRm9ydHJhbi4gT3RoZXIgT3Bl bk1QIGRpcmVjdGl2ZXMKLWFyZSBpZ25vcmVkLgorRW5hYmxlIGhhbmRsaW5n IG9mIE9wZW5NUCdzIEBjb2Rle3NpbWR9LCBAY29kZXtkZWNsYXJlIHNpbWR9 LAorQGNvZGV7ZGVjbGFyZSByZWR1Y3Rpb259LCBAY29kZXthc3N1bWV9LCBA Y29kZXtvcmRlcmVkfSwgQGNvZGV7c2Nhbn0sCitAY29kZXtsb29wfSBkaXJl Y3RpdmVzIGFuZCBjb21iaW5lZCBvciBjb21wb3NpdGUgZGlyZWN0aXZlcyB3 aXRoCitAY29kZXtzaW1kfSBhcyBjb25zdGl0dWVudCB3aXRoIEBjb2RleyNw cmFnbWEgb21wfSBpbiBDL0MrKywKK0Bjb2Rle1tbb21wOjpkaXJlY3RpdmUo Li4uKV1dfSBhbmQgQGNvZGV7W1tvbXA6OnNlcXVlbmNlKC4uLildXX0gaW4g QysrCithbmQgQGNvZGV7ISRvbXB9IGluIEZvcnRyYW4uICBPdGhlciBPcGVu TVAgZGlyZWN0aXZlcyBhcmUgaWdub3JlZC4KIAogQGl0ZW0gLWZwZXJtaXR0 ZWQtZmx0LWV2YWwtbWV0aG9kcz1AdmFye3N0eWxlfQogQG9waW5kZXggZnBl cm1pdHRlZC1mbHQtZXZhbC1tZXRob2RzCmRpZmYgLS1naXQgYS9nY2MvZm9y dHJhbi9wYXJzZS5jYyBiL2djYy9mb3J0cmFuL3BhcnNlLmNjCmluZGV4IDVi MTM0NDE5MTJhLi4yZTJlOTc3MDUyMCAxMDA2NDQKLS0tIGEvZ2NjL2ZvcnRy YW4vcGFyc2UuY2MKKysrIGIvZ2NjL2ZvcnRyYW4vcGFyc2UuY2MKQEAgLTky NCw3ICs5MjQsNyBAQCBkZWNvZGVfb21wX2RpcmVjdGl2ZSAodm9pZCkKICAg ICAgIG1hdGNobyAoImVuZCBkaXN0cmlidXRlIiwgZ2ZjX21hdGNoX29tcF9l b3NfZXJyb3IsIFNUX09NUF9FTkRfRElTVFJJQlVURSk7CiAgICAgICBtYXRj aHMgKCJlbmQgZG8gc2ltZCIsIGdmY19tYXRjaF9vbXBfZW5kX25vd2FpdCwg U1RfT01QX0VORF9ET19TSU1EKTsKICAgICAgIG1hdGNobyAoImVuZCBkbyIs IGdmY19tYXRjaF9vbXBfZW5kX25vd2FpdCwgU1RfT01QX0VORF9ETyk7Ci0g ICAgICBtYXRjaG8gKCJlbmQgbG9vcCIsIGdmY19tYXRjaF9vbXBfZW9zX2Vy cm9yLCBTVF9PTVBfRU5EX0xPT1ApOworICAgICAgbWF0Y2hzICgiZW5kIGxv b3AiLCBnZmNfbWF0Y2hfb21wX2Vvc19lcnJvciwgU1RfT01QX0VORF9MT09Q KTsKICAgICAgIG1hdGNocyAoImVuZCBzaW1kIiwgZ2ZjX21hdGNoX29tcF9l b3NfZXJyb3IsIFNUX09NUF9FTkRfU0lNRCk7CiAgICAgICBtYXRjaG8gKCJl bmQgbWFza2VkIHRhc2tsb29wIHNpbWQiLCBnZmNfbWF0Y2hfb21wX2Vvc19l cnJvciwKIAkgICAgICBTVF9PTVBfRU5EX01BU0tFRF9UQVNLTE9PUF9TSU1E KTsKQEAgLTEwMjMsNyArMTAyMyw3IEBAIGRlY29kZV9vbXBfZGlyZWN0aXZl ICh2b2lkKQogICAgICAgbWF0Y2hvICgibm90aGluZyIsIGdmY19tYXRjaF9v bXBfbm90aGluZywgU1RfTk9ORSk7CiAgICAgICBicmVhazsKICAgICBjYXNl ICdsJzoKLSAgICAgIG1hdGNobyAoImxvb3AiLCBnZmNfbWF0Y2hfb21wX2xv b3AsIFNUX09NUF9MT09QKTsKKyAgICAgIG1hdGNocyAoImxvb3AiLCBnZmNf bWF0Y2hfb21wX2xvb3AsIFNUX09NUF9MT09QKTsKICAgICAgIGJyZWFrOwog ICAgIGNhc2UgJ28nOgogICAgICAgaWYgKGdmY19tYXRjaCAoIm9yZGVyZWQg ZGVwZW5kICgiKSA9PSBNQVRDSF9ZRVMKQEAgLTEwNzAsNyArMTA3MCw3IEBA IGRlY29kZV9vbXBfZGlyZWN0aXZlICh2b2lkKQogICAgICAgbWF0Y2hvICgi cmVxdWlyZXMiLCBnZmNfbWF0Y2hfb21wX3JlcXVpcmVzLCBTVF9PTVBfUkVR VUlSRVMpOwogICAgICAgYnJlYWs7CiAgICAgY2FzZSAncyc6Ci0gICAgICBt YXRjaG8gKCJzY2FuIiwgZ2ZjX21hdGNoX29tcF9zY2FuLCBTVF9PTVBfU0NB Tik7CisgICAgICBtYXRjaHMgKCJzY2FuIiwgZ2ZjX21hdGNoX29tcF9zY2Fu LCBTVF9PTVBfU0NBTik7CiAgICAgICBtYXRjaG8gKCJzY29wZSIsIGdmY19t YXRjaF9vbXBfc2NvcGUsIFNUX09NUF9TQ09QRSk7CiAgICAgICBtYXRjaG8g KCJzZWN0aW9ucyIsIGdmY19tYXRjaF9vbXBfc2VjdGlvbnMsIFNUX09NUF9T RUNUSU9OUyk7CiAgICAgICBtYXRjaG8gKCJzZWN0aW9uIiwgZ2ZjX21hdGNo X29tcF9lb3NfZXJyb3IsIFNUX09NUF9TRUNUSU9OKTsKZGlmZiAtLWdpdCBh L2djYy90ZXN0c3VpdGUvZ2ZvcnRyYW4uZGcvZ29tcC9vcGVubXAtc2ltZC03 LmY5MCBiL2djYy90ZXN0c3VpdGUvZ2ZvcnRyYW4uZGcvZ29tcC9vcGVubXAt c2ltZC03LmY5MApuZXcgZmlsZSBtb2RlIDEwMDY0NAppbmRleCAwMDAwMDAw MDAwMC4uZDcwMTBiYjQyODgKLS0tIC9kZXYvbnVsbAorKysgYi9nY2MvdGVz dHN1aXRlL2dmb3J0cmFuLmRnL2dvbXAvb3Blbm1wLXNpbWQtNy5mOTAKQEAg LTAsMCArMSwyMyBAQAorISB7IGRnLW9wdGlvbnMgIi1mbm8tb3Blbm1wIC1m b3Blbm1wLXNpbWQgLWZkdW1wLXRyZWUtb3JpZ2luYWwiIH0KKworc3Vicm91 dGluZSBmb28gKGEsIGIpCisgIGludGVnZXIsIGNvbnRpZ3VvdXMgOjogYSg6 KSwgYig6KQorICBpbnRlZ2VyIDo6IGkKKyAgISRvbXAgc2ltZCByZWR1Y3Rp b24gKGluc2NhbiwgKzpyKQorICBkbyBpID0gMSwgMTAyNAorICAgIHIgPSBy ICsgYShpKQorICAgICEkb21wIHNjYW4gaW5jbHVzaXZlKHIpCisgICAgYihp KSA9IHIKKyAgZW5kIGRvCisgICEkb21wIGVuZCBzaW1kCisKKyAgISRvbXAg bG9vcAorICBkbyBpID0gMSwgMTAyNAorICAgIGEoaSkgPSBhKGkpICsgaQor ICBlbmQgZG8KKyAgISRvbXAgZW5kIGxvb3AKK2VuZAorCishIHsgZGctZmlu YWwgeyBzY2FuLXRyZWUtZHVtcCAiI3ByYWdtYSBvbXAgc2ltZCBsaW5lYXJc XChpOjFcXCkgcmVkdWN0aW9uXFwoaW5zY2FuLFxcKzpyXFwpIiAib3JpZ2lu YWwiIH0gfQorISB7IGRnLWZpbmFsIHsgc2Nhbi10cmVlLWR1bXAgIiNwcmFn bWEgb21wIHNjYW4gaW5jbHVzaXZlXFwoclxcKSIgIm9yaWdpbmFsIiB9IH0K KyEgeyBkZy1maW5hbCB7IHNjYW4tdHJlZS1kdW1wICIjcHJhZ21hIG9tcCBs b29wIiAib3JpZ2luYWwiIH0gfQo= --------------1FA9GsTkjkmhKpyQI9FJm9qH--