Re: [PATCH V3 3/4] OpenMP: Use enumerators for names of trait-sets and traits
On 12/12/23 05:05, Tobias Burnus wrote:
Hi Sandra,
On 07.12.23 16:52, Sandra Loosemore wrote:
This patch introduces enumerators to represent trait-set names and
trait names, which makes it easier to use tables to control other
behavior and for switch statements to dispatch on the tags. The tags
are stored in the same place in the TREE_LIST structure (OMP_TSS_ID or
OMP_TS_ID) and are encoded there as integer constants.
Thanks - that looks like a huge improvement.
* * *
I think it is useful to prepare for 'target_device'. However, it is currently
not yet implemented
on mainline - contrary to OG13.
Can you add some kind of error diagnostic for it? On mainline, the current
result is:
error: expected ‘construct’, ‘device’, ‘implementation’ or ‘user’ before
‘target_device’
13 | #pragma omp declare variant (f05) match (target_device={kind(gpu)})
| ^
But with your patch, it is silently accepted, which is bad.
(That's a modified version of
gcc/testsuite/c-c++-common/gomp/declare-variant-10.c:13)
I think you have two options:
* Either fail with the same error message as above
* Or update the error message to list 'target_device' (for C/C++/Fortran)
and handle 'target_device' separately with a sorry.
To whatever you think makes more sense for know, knowing that we do want to add
'target_device'
in the not to far future.
(I am slightly preferring the updated-error message + sorry variant as it
avoids touching
the messages later again, but either is fine.)
OK. I had a FIXME in the code noting that listing all the valid selector-set
keywords in the error message was prone to bit-rot anyway, so I have replaced
it with something more generic, and added a sorry for the missing
"target_device" support.
Also in V4 of the patch I have added a sorry for the missing "requires"
selector support so it does that rather than ICE, as Julian discovered.
Finally, I improved the error handling for including a trait-score on selectors
that don't permit it -- it now says explicitly that a score isn't permitted
there, instead of a cascade of more more obscure errors. I have a test case
for that coming later with the metadirectives patches, which are not ready for
GCC 14.
And I have a new part 5 for this series coming along too, with
prettyprinter support for the new selector representation. I realize we're
long past the end of stage 1 but I think this is still reasonable to consider
for GCC 14. It's only for internal debugging purposes, and I think it'll be
useful for Julian's work and implementing missing 5.1/5.2/TR11 selector
features for declare variant as well as my current hackery on finishing
metadirectives. There aren't any "declare variant" test cases that examine the
attribute on the base function in dump files, BTW, but I did inspect the output
by hand and also do some further testing with metadirective.
Otherwise, the patch LGTM.
As written before, 1/4, 2/4 and 4/4 are LGTM as posted.
Thanks. I'll push parts 1-4 when part 3 is approved, and part 5 too if/when
that's approved.
-Sandra
Re: [PATCH V3 3/4] OpenMP: Use enumerators for names of trait-sets and traits
Hi Sandra,
On 07.12.23 16:52, Sandra Loosemore wrote:
This patch introduces enumerators to represent trait-set names and
trait names, which makes it easier to use tables to control other
behavior and for switch statements to dispatch on the tags. The tags
are stored in the same place in the TREE_LIST structure (OMP_TSS_ID or
OMP_TS_ID) and are encoded there as integer constants.
Thanks - that looks like a huge improvement.
* * *
I think it is useful to prepare for 'target_device'. However, it is currently
not yet implemented
on mainline - contrary to OG13.
Can you add some kind of error diagnostic for it? On mainline, the current
result is:
error: expected ‘construct’, ‘device’, ‘implementation’ or ‘user’ before
‘target_device’
13 | #pragma omp declare variant (f05) match (target_device={kind(gpu)})
| ^
But with your patch, it is silently accepted, which is bad.
(That's a modified version of
gcc/testsuite/c-c++-common/gomp/declare-variant-10.c:13)
I think you have two options:
* Either fail with the same error message as above
* Or update the error message to list 'target_device' (for C/C++/Fortran)
and handle 'target_device' separately with a sorry.
To whatever you think makes more sense for know, knowing that we do want to add
'target_device'
in the not to far future.
(I am slightly preferring the updated-error message + sorry variant as it
avoids touching
the messages later again, but either is fine.)
* * *
Otherwise, the patch LGTM.
As written before, 1/4, 2/4 and 4/4 are LGTM as posted.
Thanks,
Tobias
gcc/ChangeLog
* omp-selectors.h: New file.
* omp-general.h: Include omp-selectors.h.
(OMP_TSS_CODE, OMP_TSS_NAME): New.
(OMP_TS_CODE, OMP_TS_NAME): New.
(make_trait_set_selector, make_trait_selector): Adjust declarations.
(omp_construct_traits_to_codes): Likewise.
(omp_context_selector_set_compare): Likewise.
(omp_get_context_selector): Likewise.
(omp_get_context_selector_list): New.
* omp-general.cc (omp_construct_traits_to_codes): Pass length in
as argument instead of returning it. Make it table-driven.
(omp_tss_map): New.
(kind_properties, vendor_properties, extension_properties): New.
(atomic_default_mem_order_properties): New.
(omp_ts_map): New.
(omp_check_context_selector): Simplify lookup and dispatch logic.
(omp_mark_declare_variant): Ignore variants with unknown construct
selectors. Adjust for new representation.
(make_trait_set_selector, make_trait_selector): Adjust for new
representations.
(omp_context_selector_matches): Simplify dispatch logic. Avoid
fixed-sized buffers and adjust call to omp_construct_traits_to_codes.
(omp_context_selector_props_compare): Adjust for new representations
and simplify dispatch logic.
(omp_context_selector_set_compare): Likewise.
(omp_context_selector_compare): Likewise.
(omp_get_context_selector): Adjust for new representations, and split
out...
(omp_get_context_selector_list): New function.
(omp_lookup_tss_code): New.
(omp_lookup_ts_code): New.
(omp_context_compute_score): Adjust for new representations. Avoid
fixed-sized buffers and magic numbers. Adjust call to
omp_construct_traits_to_codes.
* gimplify.cc (omp_construct_selector_matches): Avoid use of
fixed-size buffer. Adjust call to omp_construct_traits_to_codes.
gcc/c/ChangeLog
* c-parser.cc (omp_construct_selectors): Delete.
(omp_device_selectors): Delete.
(omp_implementation_selectors): Delete.
(omp_user_selectors): Delete.
(c_parser_omp_context_selector): Adjust for new representations
and simplify dispatch logic. Uniformly warn instead of sometimes
error when an unknown selector is found.
(c_parser_omp_context_selector_specification): Likewise.
(c_finish_omp_declare_variant): Adjust for new representations.
gcc/cp/ChangeLog
* decl.cc (omp_declare_variant_finalize_one): Adjust for new
representations.
* parser.cc (omp_construct_selectors): Delete.
(omp_device_selectors): Delete.
(omp_implementation_selectors): Delete.
(omp_user_selectors): Delete.
(cp_parser_omp_context_selector): Adjust for new representations
and simplify dispatch logic. Uniformly warn instead of sometimes
error when an unknown selector is found.
(cp_parser_omp_context_selector_specification): Likewise.
* pt.cc (tsubst_attribute): Adjust for new representations.
gcc/fortran/ChangeLog
* gfortran.h: Include omp-selectors.h.
(enum gfc_omp_trait_property_kind): Delete, and replace all
references with equivalent omp_tp_type enumerators.
(struct gfc_omp_trait_property): Update for omp_tp_type.
(struct gfc_omp_selector): Replace string name with new enumerator.
(struct
[PATCH V3 3/4] OpenMP: Use enumerators for names of trait-sets and traits
This patch introduces enumerators to represent trait-set names and trait names, which makes it easier to use tables to control other behavior and for switch statements to dispatch on the tags. The tags are stored in the same place in the TREE_LIST structure (OMP_TSS_ID or OMP_TS_ID) and are encoded there as integer constants. gcc/ChangeLog * omp-selectors.h: New file. * omp-general.h: Include omp-selectors.h. (OMP_TSS_CODE, OMP_TSS_NAME): New. (OMP_TS_CODE, OMP_TS_NAME): New. (make_trait_set_selector, make_trait_selector): Adjust declarations. (omp_construct_traits_to_codes): Likewise. (omp_context_selector_set_compare): Likewise. (omp_get_context_selector): Likewise. (omp_get_context_selector_list): New. * omp-general.cc (omp_construct_traits_to_codes): Pass length in as argument instead of returning it. Make it table-driven. (omp_tss_map): New. (kind_properties, vendor_properties, extension_properties): New. (atomic_default_mem_order_properties): New. (omp_ts_map): New. (omp_check_context_selector): Simplify lookup and dispatch logic. (omp_mark_declare_variant): Ignore variants with unknown construct selectors. Adjust for new representation. (make_trait_set_selector, make_trait_selector): Adjust for new representations. (omp_context_selector_matches): Simplify dispatch logic. Avoid fixed-sized buffers and adjust call to omp_construct_traits_to_codes. (omp_context_selector_props_compare): Adjust for new representations and simplify dispatch logic. (omp_context_selector_set_compare): Likewise. (omp_context_selector_compare): Likewise. (omp_get_context_selector): Adjust for new representations, and split out... (omp_get_context_selector_list): New function. (omp_lookup_tss_code): New. (omp_lookup_ts_code): New. (omp_context_compute_score): Adjust for new representations. Avoid fixed-sized buffers and magic numbers. Adjust call to omp_construct_traits_to_codes. * gimplify.cc (omp_construct_selector_matches): Avoid use of fixed-size buffer. Adjust call to omp_construct_traits_to_codes. gcc/c/ChangeLog * c-parser.cc (omp_construct_selectors): Delete. (omp_device_selectors): Delete. (omp_implementation_selectors): Delete. (omp_user_selectors): Delete. (c_parser_omp_context_selector): Adjust for new representations and simplify dispatch logic. Uniformly warn instead of sometimes error when an unknown selector is found. (c_parser_omp_context_selector_specification): Likewise. (c_finish_omp_declare_variant): Adjust for new representations. gcc/cp/ChangeLog * decl.cc (omp_declare_variant_finalize_one): Adjust for new representations. * parser.cc (omp_construct_selectors): Delete. (omp_device_selectors): Delete. (omp_implementation_selectors): Delete. (omp_user_selectors): Delete. (cp_parser_omp_context_selector): Adjust for new representations and simplify dispatch logic. Uniformly warn instead of sometimes error when an unknown selector is found. (cp_parser_omp_context_selector_specification): Likewise. * pt.cc (tsubst_attribute): Adjust for new representations. gcc/fortran/ChangeLog * gfortran.h: Include omp-selectors.h. (enum gfc_omp_trait_property_kind): Delete, and replace all references with equivalent omp_tp_type enumerators. (struct gfc_omp_trait_property): Update for omp_tp_type. (struct gfc_omp_selector): Replace string name with new enumerator. (struct gfc_omp_set_selector): Likewise. * openmp.cc (gfc_free_omp_trait_property_list): Update for omp_tp_type. (omp_construct_selectors): Delete. (omp_device_selectors): Delete. (omp_implementation_selectors): Delete. (omp_user_selectors): Delete. (gfc_ignore_trait_property_extension): New. (gfc_ignore_trait_property_extension_list): New. (gfc_match_omp_selector): Adjust for new representations and simplify dispatch logic. Uniformly warn instead of sometimes error when an unknown selector is found. (gfc_match_omp_context_selector): Adjust for new representations. (gfc_match_omp_context_selector_specification): Likewise. * trans-openmp.cc (gfc_trans_omp_declare_variant): Adjust for new representations. gcc/testsuite/ * c-c++-common/gomp/declare-variant-1.c: Expect warning on unknown selectors. * c-c++-common/gomp/declare-variant-2.c: Likewise. * gfortran.dg/gomp/declare-variant-1.f90: Likewise. * gfortran.dg/gomp/declare-variant-2.f90: Likewise. --- gcc/c/c-parser.cc | 185
