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 56AB838582A7 for ; Mon, 27 Nov 2023 17:19:32 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 56AB838582A7 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=codesourcery.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=mentor.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 56AB838582A7 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=68.232.137.180 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701105574; cv=none; b=hTpTMWMBUZpUaLGoHEPtTYobdj+qLhnVKXXWMxvQc1dipatm5/zh9VbeuytIbz8Vr2YdknJxWNZw/klPaVnmmzudUjrz1CgRhVat3Cd4ne5i+0itU2P8ordUAjbER/W++xAdZm+7KKiDINJ51rHT4ZchJ295+fTNEZgE5nFPh+s= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701105574; c=relaxed/simple; bh=cVnG5F8ztEiCmrQsjcd9fzTcqd81hfx8PznZ0D0KfaQ=; h=Message-ID:Date:MIME-Version:Subject:To:From; b=vWGZM0oqPxHl0IvpGTXREaL13ZP7utXRJWgXv2oye4H4MUHdnEpxbFqmQEAOpFTO2z6hym3th+dXInaFlfjIy3I/HywwlLzCv1itD7AKVsf/DniEaMjxJVZwbX3NLAgnmjpAsqIRSVlnE5uFrk2L26mHcDeQ9RjHp2YvYYYeco4= ARC-Authentication-Results: i=1; server2.sourceware.org X-CSE-ConnectionGUID: FlQYXgu3SxefE5QrJ3FX6Q== X-CSE-MsgGUID: M01dUzuTTgOFdHR1R1FTJg== X-IronPort-AV: E=Sophos;i="6.04,231,1695715200"; d="scan'208";a="23812739" Received: from orw-gwy-02-in.mentorg.com ([192.94.38.167]) by esa3.mentor.iphmx.com with ESMTP; 27 Nov 2023 09:19:31 -0800 IronPort-SDR: qGpUX5NvWGe1u5db6naDFcuOUHDuFJ0S+CCSvRQlHxuaPhe1vEMR998Xoz3Ti0so6mDiKAEYYy W0WKsCB0dpGCjV361ma223JckGEZhOMCaDAZr9AKhZiwo5CtX8AmdV535mrrAPV2yujaIr4qZX 6zp+xztZ0/xSVrFw1dTD2UKvomTvowoxEDYP/H6JMVgM2/stdVN2NliE22uV654JLUB+9NOUI/ WHoPP8zU+gwXtyM3bmK4GUVvBgWrKya/vayb5UoDWcDo2957JlCdNSWhlPkbTPN88V2zo6Ojr+ dQw= Message-ID: <7913e015-6eb6-44d5-bea1-39282c4039ed@codesourcery.com> Date: Mon, 27 Nov 2023 18:19:26 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH V2 3/3] OpenMP: Use enumerators for names of trait-sets and traits Content-Language: en-US To: Sandra Loosemore , CC: , References: <20231119092151.1690294-1-sandra@codesourcery.com> <20231122162233.1721224-1-sandra@codesourcery.com> From: Tobias Burnus In-Reply-To: <20231122162233.1721224-1-sandra@codesourcery.com> 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-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=-5.3 required=5.0 tests=BAYES_00,HEADER_FROM_DIFFERENT_DOMAINS,KAM_DMARC_STATUS,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 List-Id: Hi Sandra, {BTW: 1/3 needs to be eventually rebased as it no longer applies cleanly; I have not checked 2/3 or 3/3 yet.] 1/3+2/3 look good to me, unless Jakub has some comments, I think they can go it. Regarding 3/3, some first comments. I still want to read it a bit more careful and play with it. On 22.11.23 17:22, Sandra Loosemore wrote: > +static const char *const vendor_properties[] =3D > + { "amd", "arm", "bsc", "cray", "fujitsu", "gnu", "ibm", "intel", > + "llvm", "nvidia", "pgi", "ti", "unknown", NULL }; Can you add "hpe"? Cf. "OpenMP API 5.2 Supplementary Source Code" at https://www.openmp.org/specifications/ > +static const char *const atomic_default_mem_order_properties[] =3D > + { "seq_cst", "relaxed", "acq_rel", NULL }; Can you add "acquire" and "release"? Those have been added in OpenMP 5.1 for 'omp atomic', supported since GCC 12; albeit, for requires, that's new since 5.2. > + { "atomic_default_mem_order", > + (1 << OMP_TRAIT_SET_IMPLEMENTATION), > + OMP_TRAIT_PROPERTY_ID, true, > + atomic_default_mem_order_properties, > + }, > + { "requires", > + (1 << OMP_TRAIT_SET_IMPLEMENTATION), > + OMP_TRAIT_PROPERTY_CLAUSE_LIST, true, > + NULL > + }, > + { "unified_address", > + (1 << OMP_TRAIT_SET_IMPLEMENTATION), > + OMP_TRAIT_PROPERTY_NONE, true, > + NULL > + }, I don't understand this code. This looks as if "requires" and "unified_addr= ess" are on the same level but in my understanding they have to be used as in: match(implementation =3D {requires(unified_address, atomic_default_mem_or= der_properties(release)}) while from the syntax, it looks as if this would permit: match(implementation =3D {unified_address, atomic_default_mem_order_prope= rties(release)) Disclaimer: It might be that the code handles it correctly but I just misre= ad it. Or that I misread the spec. * * * > + warning_at (loc, 0, > + "unknown property %qE of %qs selector", All '0' OpenMP warnings should now use 'OPT_Wopenmp' instead. * * * > - if (selectors[i] =3D=3D NULL) > + /* Some trait sets permit extension traits which are supposed > + to be ignored if the implementation doesn't support them. > + GCC does not support any extension traits, and if it did, they > + would have their own identifiers. */ I am not sure whether I get this correctly. In my understanding match(implementation =3D {extension(ompx_myCompiler_abcd)]) should parse without error - but evaluate as false / not matching. Thus, it= is not really ignored but parsed =E2=80=93 but still causing a not-matched. (We can argue whether that should be silently accepted or still show a warn= ing.) Likewise for: match (implementation =3D { ompx_myCompiler_abcd(1) } ) albeit here a warning could make more sense than for 'extension', especiall= y if a typo fix would be available. From the comment, it looks like as it is completely ignored - such that th= ere could be still a match. Disclaimer: I might have misunderstood the code - or might have missed some= thing in the spec. 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