Re: [PATCH 1/8] OpenMP: metadirective tree data structures and front-end interfaces

[Tobias Burnus <burnus@net-b.de>]

Hi Sandra,

given that it is "just" a revised patch of previously posted patch and 
(code wise not file wise) very localized to OpenMP code - and even there 
touching only 'declare ...' code in a simple way:

I would be still fine to have it in GCC 14, but I want to give Jakub a 
chance to shout...


Glancing at the code, it looks okayish, but I have two quick comments.
(I have now an errant to do - and will continue later with the review.)

Sandra Loosemore wrote:
> +static tree
> +omp_dynamic_cond (tree ctx)
> +{
> +  tree expr = NULL_TREE;
> +
> +  tree user = omp_get_context_selector (ctx, OMP_TRAIT_SET_USER,
> +					OMP_TRAIT_USER_CONDITION);
> +  if (user)
> +    {
> +      tree expr_list = OMP_TS_PROPERTIES (user);
> +
> +      gcc_assert (OMP_TP_NAME (expr_list) == NULL_TREE);
> +
> +      /* The user condition is not dynamic if it is constant.  */
> +      if (!tree_fits_shwi_p (TREE_VALUE (expr_list)))
> +	expr = TREE_VALUE (expr_list);
> +    }
> +
> +  tree target_device
> +    = omp_get_context_selector_list (ctx, OMP_TRAIT_SET_TARGET_DEVICE);
> +  if (target_device)
> +    {
> +      tree device_num = null_pointer_node;
> +      tree kind = null_pointer_node;
> +      tree arch = null_pointer_node;
> +      tree isa = null_pointer_node;
> +
> +      tree device_num_sel
> +	= omp_get_context_selector (ctx, OMP_TRAIT_SET_TARGET_DEVICE,
> +				    OMP_TRAIT_DEVICE_NUM);
> +      if (device_num_sel)
> +	device_num = OMP_TP_VALUE (OMP_TS_PROPERTIES (device_num_sel));
> +      else
> +	device_num = build_int_cst (integer_type_node, -1);

I think this won't correctly distinguish 'omp_initial_device' (= -1) 
from unset. OpenMP has 'omp_invalid_device', 'omp_initial_device' and 
then the values 0 to omp_get_num_devices()  [where the latter denotes 
the host, 0 ... < omp_get_num_device() are non-host devices].


>   /* Compute *SCORE for context selector CTX.  Return true if the score
>      would be different depending on whether it is a declare simd clone or
> @@ -2194,12 +2308,21 @@ omp_context_compute_score (tree ctx, score_wide_int *score, bool declare_simd)
>   {
>     tree selectors
>       = omp_get_context_selector_list (ctx, OMP_TRAIT_SET_CONSTRUCT);
> -  bool has_kind = omp_get_context_selector (ctx, OMP_TRAIT_SET_DEVICE,
> -					    OMP_TRAIT_DEVICE_KIND);
> -  bool has_arch = omp_get_context_selector (ctx, OMP_TRAIT_SET_DEVICE,
> -					    OMP_TRAIT_DEVICE_ARCH);
> -  bool has_isa = omp_get_context_selector (ctx, OMP_TRAIT_SET_DEVICE,
> -					   OMP_TRAIT_DEVICE_ISA);
> +  bool has_kind
> +    = (omp_get_context_selector (ctx, OMP_TRAIT_SET_DEVICE,
> +				 OMP_TRAIT_DEVICE_KIND)
> +       || omp_get_context_selector (ctx, OMP_TRAIT_SET_TARGET_DEVICE,
> +				    OMP_TRAIT_DEVICE_KIND));

I do not completely understand how that's used, but I have the feeling 
that having a match that contains both 'device' and 'target_device' is 
not correctly handled here.

>       int *scores
>         = (int *) alloca ((2 * nconstructs + 2) * sizeof (int));

That's not new but I have the feeling it should be '+ 3' and not '+ 2'
for device or target_device + and having both device and target device,
it might even need to be + 6.

I also wonder whether 'alloca' will really work - or only when inlined 
at all call sites. (Currenlty, it is called 6 times - and a 7th time 
with this patch.)

That's how far I came and before having to leave for now. (And having no 
time to think about what I wrote about 'score'.)

Tobias