Re: [PATCH 0/3] OpenMP: Improve data abstractions for context selectors

[Julian Brown <julian@codesourcery.com>]

On Sun, 19 Nov 2023 02:21:48 -0700
Sandra Loosemore <sandra@codesourcery.com> wrote:

> This series of patches adds a layer of data abstraction, using at
> least slightly more descriptive names, and then tries to address some
> of the representation and coding issues.
> 
> Part 1 introduces some macros (e.g., OMP_TSS_ID instead of
> TREE_PURPOSE to get the name of a selector) and renames a bunch of
> variables (e.g., tss for a trait-set selector, ts for a trait
> selector, tp for a trait property).  Those changes were relatively
> mechanical.  I also added some abstraction for the trait-score so that
> it need not be handled explicitly when processing property lists.
> 
> Part 2 changes the representation of name-list properties so that both
> the string and identifier forms store the name in the same place.
> 
> Part 3 is a more radical change: it replaces the string names of
> trait-set and trait selectors with enumerators, which allows clean-up
> of those funky switch statements.  I also made things more
> table-driven.  Alas, this part is still WIP; there's an ICE in one of
> the test cases I haven't been able to track down yet.
> 
> I can continue to work on this patch set in the next couple of weeks
> if the general direction is seen as a good thing.  I believe there is
> a little more latitude re the end of stage 1 with OpenMP (as there is
> with target-specific patches) since it is not enabled by default; in
> any case I'd like to get feedback on the general direction before
> continuing too much farther with this, and adapting the metadirective
> patches to match it.

FWIW, these changes look good to me (I've been working with these data
structures in the context of adding another OpenMP feature).

I wonder if this bit might be relevant to the ICE you see in the 3rd
patch (from my WIP patches):

@@ -1247,12 +1247,17 @@ omp_mark_declare_variant (location_t loc, tree variant, tree construct)
       DECL_ATTRIBUTES (variant) = attr;
       return;
     }
+#if 0
+  /* I think this might be bogus.  It compares an extracted "construct"
+     selector set (containing e.g. just "target") with a complete context
+     selector ("construct target", ...).  */
   if ((TREE_VALUE (attr) != NULL_TREE) != (construct != NULL_TREE)
       || (construct != NULL_TREE
          && omp_context_selector_set_compare ("construct", TREE_VALUE (attr),
                                               construct)))
     error_at (loc, "%qD used as a variant with incompatible %<construct%> "
                   "selector sets", variant);
+#endif
 }

Perhaps not (it could be a bug in my own code or understanding), but
maybe!

HTH,

Julian