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
