Re: [PATCH V2 3/3] OpenMP: Use enumerators for names of trait-sets and traits

[Tobias Burnus <tobias@codesourcery.com>]

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[] =
> +  { "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[] =
> +  { "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_address"
are on the same level but in my understanding they have to be used as in:

  match(implementation = {requires(unified_address, atomic_default_mem_order_properties(release)})

while from the syntax, it looks as if this would permit:

  match(implementation = {unified_address, atomic_default_mem_order_properties(release))

Disclaimer: It might be that the code handles it correctly but I just misread 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] == 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 = {extension(ompx_myCompiler_abcd)])

should parse without error - but evaluate as false / not matching. Thus, it is not really
ignored but parsed – but still causing a not-matched.

(We can argue whether that should be silently accepted or still show a warning.)


Likewise for:
   match (implementation = { ompx_myCompiler_abcd(1) } )

albeit here a warning could make more sense than for 'extension', especially if a
typo fix would be available.

 From the comment, it looks like as it is completely ignored - such that there could be still a match.

Disclaimer: I might have misunderstood the code - or might have missed something in the spec.

Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955