Hi Thomas, Tobias,
here's the updated v2 of the readonly modifier front-end patch.

On 2023/7/20 11:08 PM, Tobias Burnus wrote:
>>> +++ b/gcc/c/c-parser.cc
>>> @@ -14059,7 +14059,8 @@ c_parser_omp_variable_list (c_parser *parser,
>>>
>>>   static tree
>>>   c_parser_omp_var_list_parens (c_parser *parser, enum omp_clause_code kind,
>>> -                           tree list, bool allow_deref = false)
>>> +                           tree list, bool allow_deref = false,
>>> +                           bool *readonly = NULL)
>>> ...
>> Instead of doing this in 'c_parser_omp_var_list_parens', I think it's
>> clearer to have this special 'readonly :' parsing logic in the two places
>> where it's used.
> I concur. The same issue also occurred for OpenMP's
> c_parser_omp_clause_to, and c_parser_omp_clause_from and the 'present'
> modifier. For it, I created a combined function but the main reason for
> that is that OpenMP also permits more modifiers (like 'iterators'),
> which would cause more duplication of code ('iterator' is not yet
> supported).
> 
> For something as simple to parse as this modifier, I would just do it at
> the two places – as Thomas suggested.

Okay, I've changed the C/C++ parser parts to have the parsing logic directly
added.

>>> +++ b/gcc/fortran/gfortran.h
>>> @@ -1360,7 +1360,11 @@ typedef struct gfc_omp_namelist
>>>       {
>>>         gfc_omp_reduction_op reduction_op;
>>>         gfc_omp_depend_doacross_op depend_doacross_op;
>>> -      gfc_omp_map_op map_op;
>>> +      struct
>>> +        {
>>> +       ENUM_BITFIELD (gfc_omp_map_op) map_op:8;
>>> +       bool readonly;
>>> +        };
>>>         gfc_expr *align;
>>>         struct
>>>        {
>> [...] Thus, the above looks good to me.
> I concur but I wonder whether it would be cleaner to name the struct;
> this makes it also more obvious what belongs together in the union.
> 
> Namely, naming the struct 'map' and then changing the 45 users from
> 'u.map_op' to 'u.map.op' and the new 'u.readonly' to 'u.map.readonly'. –
> this seems to be cleaner.

I've adjusted 'u.map' to be a named struct now, and updated the references.

>> + if (gfc_match ("readonly :") == MATCH_YES)
>> I note this one does not have a space after ':' in 'gfc_match', but the
>> one above in 'gfc_match_omp_clauses' does.  I don't know off-hand if that
>> makes a difference in parsing -- probably not, as all of
>> 'gcc/fortran/openmp.cc' generally doesn't seem to be very consistent
>> about these two variants?
> It *does* make a difference. And for obvious reasons. You don't want to permit:
> 
>    !$acc kernels asnyccopy(a)
> 
> but require at least one space (or comma) between "async" and "copy"..
> (In fixed form Fortran, it would be fine - as would be "!$acc k e nelsasy nc co p y(a)".)
> 
> A " " matches zero or more whitespaces, but with gfc_match_space you can find out
> whether there was whitespace or not.

Okay, made sure both are 'gfc_match ("readonly : ")'. Thanks for catching that, didn't
realize that space was significant.

>>> +++ 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 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.

The other points-to patch then (also in front-ends) take the OMP_CLAUSE_MAP_READONLY
to mark the clauses of "base-pointers of array-sections" as OMP_CLAUSE_MAP_POINTS_TO_READONLY,
and later this gradually gets relayed to alias oracle routines in tree-ssa-alias.cc

Re-tested this v2 patch on powerpc64le-linux/nvptx. Okay for trunk?

Thanks,
Chung-Lin

2023-08-07  Chung-Lin Tang  <cltang@codesourcery.com>

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): Add 'bool readonly = false'
	parameter, set n->u.map.readonly field. Adjust 'n->u.map_op' to
	'n->u.map.op'.
	(gfc_match_omp_clause_reduction): Adjust 'n->u.map_op' to 'n->u.map.op'.
	(gfc_match_omp_clauses): Add readonly modifier parsing for OpenACC
	copyin clause, adjust call to gfc_match_omp_map_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-pretty-print.cc (dump_omp_clause): Add support for printing
	OMP_CLAUSE_MAP_READONLY and OMP_CLAUSE__CACHE__READONLY.
	* tree.h (OMP_CLAUSE_MAP_READONLY): New macro.
	(OMP_CLAUSE__CACHE__READONLY): New macro.

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