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

[Sandra Loosemore]

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