Hi Thomas, Tobias,

On 2023/10/26 6:43 PM, Thomas Schwinge wrote:
>>>>> +++ b/gcc/tree.h
>>>>> @@ -1813,6 +1813,14 @@ class auto_suppress_location_wrappers
>>>>>   #define OMP_CLAUSE_MAP_DECL_MAKE_ADDRESSABLE(NODE) \
>>>>>     (OMP_CLAUSE_SUBCODE_CHECK (NODE, OMP_CLAUSE_MAP)->base.addressable_flag)
>>>>>
>>>>> +/* Nonzero if OpenACC 'readonly' modifier set, used for 'copyin'.  */
>>>>> +#define OMP_CLAUSE_MAP_READONLY(NODE) \
>>>>> +  TREE_READONLY (OMP_CLAUSE_SUBCODE_CHECK (NODE, OMP_CLAUSE_MAP))
>>>>> +
>>>>> +/* Same as above, for use in OpenACC cache directives.  */
>>>>> +#define OMP_CLAUSE__CACHE__READONLY(NODE) \
>>>>> +  TREE_READONLY (OMP_CLAUSE_SUBCODE_CHECK (NODE, OMP_CLAUSE__CACHE_))
>>>> I'm not sure if these special accessor functions are actually useful, or
>>>> we should just directly use 'TREE_READONLY' instead?  We're only using
>>>> them in contexts where it's clear that the 'OMP_CLAUSE_SUBCODE_CHECK' is
>>>> satisfied, for example.
>>> I find directly using TREE_READONLY confusing.
>>
>> FWIW, I've changed to use TREE_NOTHROW instead, if it can give a better sense of safety :P
> 
> I don't understand that, why not use 'TREE_READONLY'?
> 
>> I think there's a misunderstanding here anyways: we are not relying on a DECL marked
>> TREE_READONLY here. We merely need the OMP_CLAUSE_MAP to be marked as OMP_CLAUSE_MAP_READONLY == 1.
> 
> Yes, I understand that.  My question was why we don't just use
> 'TREE_READONLY (c)', where 'c' is the
> 'OMP_CLAUSE_MAP'/'OMP_CLAUSE__CACHE_' clause (not its decl), and avoid
> the indirection through
> '#define OMP_CLAUSE_MAP_READONLY'/'#define OMP_CLAUSE__CACHE__READONLY',
> given that we're only using them in contexts where it's clear that the
> 'OMP_CLAUSE_SUBCODE_CHECK' is satisfied.  I don't have a strong
> preference, though.

After further re-testing using TREE_NOTHROW, I have reverted to using TREE_READONLY, because TREE_NOTHROW clashes
with OMP_CLAUSE_RELEASE_DESCRIPTOR (which doesn't use the OMP_CLAUSE_MAP_* naming convention and is
not documented in gcc/tree-core.h either, hmmm...)

I have added the comment adjustments in gcc/tree-core.h for the new uses of TREE_READONLY/readonly_flag.

We basically all use OMP_CLAUSE_SUBCODE_CHECK macros for OpenMP clause expressions exclusively,
so I don't see a reason to diverge from that style (even when context is clear).

> Either way, you still need to document this:
> 
> | Also, for the new use for OMP clauses, update 'gcc/tree.h:TREE_READONLY',
> | and in 'gcc/tree-core.h' for 'readonly_flag' the
> | "table lists the uses of each of the above flags".

Okay, done as mentioned above.

> In addition to a few individual comments above and below, you've also not
> yet responded to my requests re test cases.

I have greatly expanded the test scan patterns to include parallel/kernels/serial/data/enter data,
as well as non-readonly copyin clause together with readonly.

Also added simple 'declare' tests, but there is not anything to scan in the 'tree-original' dump though.

>> +  tree nl = list;
>> +  bool readonly = false;
>> +  matching_parens parens;
>> +  if (parens.require_open (parser))
>> +    {
>> +      /* Turn on readonly modifier parsing for copyin clause.  */
>> +      if (c_kind == PRAGMA_OACC_CLAUSE_COPYIN)
>> +     {
>> +       c_token *token = c_parser_peek_token (parser);
>> +       if (token->type == CPP_NAME
>> +           && !strcmp (IDENTIFIER_POINTER (token->value), "readonly")
>> +           && c_parser_peek_2nd_token (parser)->type == CPP_COLON)
>> +         {
>> +           c_parser_consume_token (parser);
>> +           c_parser_consume_token (parser);
>> +           readonly = true;
>> +         }
>> +     }
>> +      location_t loc = c_parser_peek_token (parser)->location;
> 
> I suppose 'loc' here now points to after the opening '(' or after the
> 'readonly :'?  This is different from what 'c_parser_omp_var_list_parens'
> does, and indeed, 'c_parser_omp_variable_list' states that "CLAUSE_LOC is
> the location of the clause", not the location of the variable-list?  As
> this, I suppose, may change diagnostics, please restore the original
> behavior.  (This appears to be different in the C++ front end, huh.)

Thanks for catching this! Fixed.

>> --- a/gcc/fortran/openmp.cc
>> +++ b/gcc/fortran/openmp.cc
>> @@ -1197,7 +1197,7 @@ omp_inv_mask::omp_inv_mask (const omp_mask &m) : omp_mask (m)
>>
>>  static bool
>>  gfc_match_omp_map_clause (gfc_omp_namelist **list, gfc_omp_map_op map_op,
>> -                       bool allow_common, bool allow_derived)
>> +                       bool allow_common, bool allow_derived, bool readonly = false)
>>  {
>>    gfc_omp_namelist **head = NULL;
>>    if (gfc_match_omp_variable_list ("", list, allow_common, NULL, &head, true,
>> @@ -1206,7 +1206,10 @@ gfc_match_omp_map_clause (gfc_omp_namelist **list, gfc_omp_map_op map_op,
>>      {
>>        gfc_omp_namelist *n;
>>        for (n = *head; n; n = n->next)
>> -     n->u.map_op = map_op;
>> +     {
>> +       n->u.map.op = map_op;
>> +       n->u.map.readonly = readonly;
>> +     }
>>        return true;
>>      }
> 
> Didn't we conclude that "not doing it here is cleaner" (Tobias' words),
> and instead do this "Similar to 'c_parser_omp_var_list_parens'" (my
> words)?  That is, not add the 'bool readonly' formal parameter to
> 'gfc_match_omp_map_clause'.

Fixed in this v3 patch.

Again, tested on x86_64-linux + nvptx offloading. Okay for mainline?

Thanks,
Chung-Lin

gcc/c/ChangeLog:

	* c-parser.cc (c_parser_oacc_data_clause): Add parsing support for
	'readonly' modifier, set OMP_CLAUSE_MAP_READONLY if readonly modifier
	found, update comments.
	(c_parser_oacc_cache): Add parsing support for 'readonly' modifier,
	set OMP_CLAUSE__CACHE__READONLY if readonly modifier found, update
	comments.

gcc/cp/ChangeLog:

	* parser.cc (cp_parser_oacc_data_clause): Add parsing support for
	'readonly' modifier, set OMP_CLAUSE_MAP_READONLY if readonly modifier
	found, update comments.
	(cp_parser_oacc_cache): Add parsing support for 'readonly' modifier,
	set OMP_CLAUSE__CACHE__READONLY if readonly modifier found, update
	comments.

gcc/fortran/ChangeLog:

	* dump-parse-tree.cc (show_omp_namelist): Print "readonly," for
	OMP_LIST_MAP and OMP_LIST_CACHE if n->u.map.readonly is set.
	Adjust 'n->u.map_op' to 'n->u.map.op'.
	* gfortran.h (typedef struct gfc_omp_namelist): Adjust map_op as
	'ENUM_BITFIELD (gfc_omp_map_op) op:8', add 'bool readonly' field,
	change to named struct field 'map'.

	* openmp.cc (gfc_match_omp_map_clause): Adjust 'n->u.map_op' to
	'n->u.map.op'.
	(gfc_match_omp_clause_reduction): Likewise.

	(gfc_match_omp_clauses): Add readonly modifier parsing for OpenACC
	copyin clause, set 'n->u.map.op' and 'n->u.map.readonly' for parsed
	clause. Adjust 'n->u.map_op' to 'n->u.map.op'.
	(gfc_match_oacc_declare): Adjust 'n->u.map_op' to 'n->u.map.op'.
	(gfc_match_oacc_cache): Add readonly modifier parsing for OpenACC
	cache directive.
	(resolve_omp_clauses): Adjust 'n->u.map_op' to 'n->u.map.op'.
	* trans-decl.cc (add_clause): Adjust 'n->u.map_op' to 'n->u.map.op'.
	(finish_oacc_declare): Likewise.
	* trans-openmp.cc (gfc_trans_omp_clauses): Set OMP_CLAUSE_MAP_READONLY,
	OMP_CLAUSE__CACHE__READONLY to 1 when readonly is set. Adjust
	'n->u.map_op' to 'n->u.map.op'.
	(gfc_add_clause_implicitly): Adjust 'n->u.map_op' to 'n->u.map.op'.

gcc/ChangeLog:
	* tree.h (OMP_CLAUSE_MAP_READONLY): New macro.
	(OMP_CLAUSE__CACHE__READONLY): New macro.
	* tree-core.h (struct GTY(()) tree_base): Adjust comments for new
	uses of readonly_flag bit in OMP_CLAUSE_MAP_READONLY and
	OMP_CLAUSE__CACHE__READONLY.
	* tree-pretty-print.cc (dump_omp_clause): Add support for printing
	OMP_CLAUSE_MAP_READONLY and OMP_CLAUSE__CACHE__READONLY.

gcc/testsuite/ChangeLog:

	* c-c++-common/goacc/readonly-1.c: New test.
	* gfortran.dg/goacc/readonly-1.f90: New test.