[PATCH v2 01/12] OpenMP: metadirective tree data structures and front-end interfaces

[Sandra Loosemore]

On 5/31/24 06:22, Tobias Burnus wrote:

> I have to admit that I don't really see the use of metadirective_p as …
>>   int
>> -omp_context_selector_matches (tree ctx)
>> +omp_context_selector_matches (tree ctx, bool metadirective_p, bool 
>> delay_p)
> ...
>> +            if (metadirective_p && delay_p)
>> +              return -1;
> 
> I do see why the resolution of KIND/ARCH/ISA should be delayed – for 
> both variant/metadirective as long as the code is run by the host and 
> the device. Except that we could exclude, e.g., 'kind(FPGA)' early on as 
> we don't support it at all.
> 
> But once the device code is split off, I don't see why we can't expand 
> the DEVICE clause right away for both variant and metadirective – while 
> for 'target_device', we cannot do much until runtime – except of 
> excluding things like 'kind(fpga)' – or excluding all 'arch' known not 
> to be supported neither by the host nor by any enabled offload devices.
> 
> Thus, I see why there is a 'delay_p', but not why there is a 
> 'metadirective_p'.
> 
> But I might have missed something important ...

Yeah, omp_context_selector_matches() is pretty substantially revised in 
part 9 of the posted patch set -- among other things, to remove the 
metadirective_p parameter.  The current split between patches isn't 
ideal but this is such a huge patch set already (with more pieces in the 
works to support "begin declare variant") that refactoring them would be 
a lot of work and probably result in something even more challenging to 
review.  :-S

> 
>>              case OMP_TRAIT_USER_CONDITION:
>>                if (set == OMP_TRAIT_SET_USER)
>>                  for (tree p = OMP_TS_PROPERTIES (ts); p; p = 
>> TREE_CHAIN (p))
>>                    if (OMP_TP_NAME (p) == NULL_TREE)
>>                      {
>> +              /* OpenMP 5.1 allows non-constant conditions for
>> +             metadirectives.  */
>> +              if (metadirective_p
>> +              && !tree_fits_shwi_p (OMP_TP_VALUE (p)))
>> +            break;
>>                        if (integer_zerop (OMP_TP_VALUE (p)))
>>                          return 0;
>>                        if (integer_nonzerop (OMP_TP_VALUE (p)))
>>                          break;
>>                        ret = -1;
>>                      }
> 
> (BTW: I am happy to be enlightened as I likely have miss some fine print.)
> 
> Regarding the comment: True, but shouldn't this be handled before by 
> issuing an error when such a clause is used in 'declare variant', i.e. 
> only occur when metadirective_p is/can be true?

The error diagnostic is handled during parsing in the respective front 
ends (parts 4, 5, and 7 of the series).

> Besides, I have to admit that I do not understand the new code. The 
> current code has: constant zero → whole selector known to be false 
> ("return 0"); nonzero constant → keep current state, i.e. either 'true' 
> (1) or don't known ('-1') and continue; otherwise (not const) → set to 
> "don't know" (-1) and continue with the next item.
> 
> That seems to make also sense for metadirectives. But your patch changes 
> this to keep current state if a variable. In that case, '1' is used if 
> this is the only item or the previous condition is true. Or "-1" when 
> the previous item is "don't know" (-1). - I think that doesn't make 
> sense and it should always return -1 for a run time value.

-1 doesn't really mean "don't know".  It means "don't know YET".  For 
the purposes of omp_context_selector_matches, a dynamic selector always 
matches in the sense that they all need to be included in the list of 
replacement candidates.

> Additionally, I wonder why you use tree_fits_shwi_p instead of a simple 
> 'TREE_CODE (OMP_TP_VALUE (p)) != INTEGER_CST'. It does not seem to 
> matter here, but '(uint128_t)-1' looks like a valid condition and valid 
> constant, which integer_nonzerop should handled but if the hwi is 128bit 
> wide, it won't fit into a signed variable.
> 
> (As integer_nonzerop and the current code both do "break;" it won't 
> change the result of the current code.)

The existing code for parsing "declare variant" context selectors 
already uses tree_fits_shwi_p.  (See e.g. c_parser_omp_context_selector 
in gcc/c/c-parser.cc.)  If there's a better idiom for checking for a 
constant I'll certainly use it, but I was trying to be consistent with 
what I thought was standard practice already.  :-S

> * * *
>> +static tree
>> +omp_dynamic_cond (tree ctx)
>> +{
> ...
>> +      /* The user condition is not dynamic if it is constant.  */
>> +      if (!tree_fits_shwi_p (TREE_VALUE (expr_list)))
> 
> Any reason for using tree_fits_shwi_p instead of INTEGER_CST? Here, 
> (uint128_t)-1 could make a difference …

Same here.

> 
>> +    /* omp_initial_device is -1, omp_invalid_device is -4; choose
>> +       a value that isn't otherwise defined to indicate the default
>> +       device.  */
>> +    device_num = build_int_cst (integer_type_node, -2);
> 
> Don't do this - we do it differently for 'target' and it should do the 
> same. Some value usage history:

Wait, in your January review comments on an earlier version of this 
patch you told me I *should* do this -- use a negative value that is 
neither omp_initial_device nor omp_invalid_device.

> For target / target data etc, GCC historically used -1 to denote for 
> that no device clause was specified (→ default device) and >= 0 for a 
> user-specified device, i.e. "GOMP_target_ext(device_num,..." gets a "-1" 
> if nothing was specified (= default device).
> 
> OpenMP 5.2 then introduced omp_{initial,invalid}_device with 
> omp_initial_device == -1 and the other one < -1 but implementation defined.
> 
> Thus, we have 0...num_devices + -4 (omp_invalid_device) and "-1" for 
> both default device (GOMP_target_ext etc.) and as host/initial device 
> (API routines omp_...).
> 
> Solution was: For the GOMP_... functions, keep using -1 = default device 
> and use -2 for omp_initial_device. (And for the API routines, -1 == 
> initial device).
> 
> If you look at the device number handling in libgomp, functions which 
> can be called in both context have a "remap" boolean to handle the two 
> usages for -1.
> 
> 
> I strongly suggest to use the same semantic here to avoid confusion, 
> i.e. -1 = nothing specified, use default device. And map a 
> user-specified omp_initial_device (-1) to a -2.
> 
> And it would help to use GOMP_DEVICE_HOST_FALLBACK (-2, host/initial 
> device) and GOMP_DEVICE_ICV (-1, default device) where appropriate as 
> they are a tiny bit more readable and more greppable.
> 
> For the conversion, have a look at omp-expand.cc's expand_omp_target and 
> search for both OMP_CLAUSE_DEVICE (2nd hit) and need_device_adjustment; 
> if the latter is true: device = (cond ? device : 
> GOMP_DEVICE_HOST_FALLBACK)  [where cond is 'd == -1' or rather 
> '(unsigned) d > (unsigned)-2']
> 
> It might make sense to move (part of) this code out of expand_omp_target 
> and share it by both; at least the gimple code is rather complex. And I 
> guess your code also runs in later phase of the compiler and, hence, 
> also cannot use simple code.

I'm totally lost here.  Why is this necessary or a good idea?  The 
device number gets passed directly to a generated call to the runtime 
function GOMP_evaluate_target_device, which specifically tests for -2 as 
a magic number (part 3 of the patch series).  It doesn't have anything 
to do with the omp target construct expansion.  If -2, specifically, was 
a bad choice, I can change it to a different magic number that is used 
only for this purpose and nothing else.

> * * *
>> @@ -4019,6 +4019,40 @@ dump_generic_node (pretty_printer *pp, tree 
>> node, int spc, dump_flags_t flags,
> ...
>> +        if (selector == NULL_TREE)
>> +          pp_string (pp, "default:");
> 
> I wonder whether we should be forward looking (OpenMP >= 5.2) and dump 
> 'otherwise:'. (OpenMP <= 5.1 + [deprecated] 5.2 have 'default'.)

I can fix this one easily enough now, but I was envisioning some future 
task to clean up all the deprecated syntaxes.  Among other things, GCC 
still implements only the OpenMP 5.0 syntax for requires selectors 
(where the clauses of the requires directive are recognized as traits by 
themselves in the "implementation" set) instead of the 5.1+ syntax (the 
requires clauses are properties of a "requires" trait in the 
"implementation" set).  In addition to the backlog of 
unreviewed/uncommitted patches relating to variant directives, there's 
also a backlog of incomplete or broken features, and doing something 
about different versions of the syntax is one of them.

-Sandra