[committed] Re: [PATCH] openmp: Add support for the 'indirect' clause in C/C++

[Kwok Cheung Yeung <kcy@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 3047 bytes --]

On 09/11/2023 12:24 pm, Thomas Schwinge wrote:
>> --- a/gcc/tree-core.h
>> +++ b/gcc/tree-core.h
>> @@ -350,6 +350,9 @@ enum omp_clause_code {
>>     /* OpenMP clause: doacross ({source,sink}:vec).  */
>>     OMP_CLAUSE_DOACROSS,
>>
>> +  /* OpenMP clause: indirect [(constant-integer-expression)].  */
>> +  OMP_CLAUSE_INDIRECT,
>> +
>>     /* Internal structure to hold OpenACC cache directive's variable-list.
>>        #pragma acc cache (variable-list).  */
>>     OMP_CLAUSE__CACHE_,
> 
> In this position here, isn't 'OMP_CLAUSE_INDIRECT' applicable to the
> 'OMP_CLAUSE_RANGE_CHECK' in 'gcc/tree.h:OMP_CLAUSE_SIZE' and
> 'gcc/tree.h:OMP_CLAUSE_DECL':
> 
>      #define OMP_CLAUSE_SIZE(NODE)                                               \
>        OMP_CLAUSE_OPERAND (OMP_CLAUSE_RANGE_CHECK (OMP_CLAUSE_CHECK (NODE),      \
>                                                OMP_CLAUSE_FROM,          \
>                                                OMP_CLAUSE__CACHE_), 1)
> 
>      #define OMP_CLAUSE_DECL(NODE)                                       \
>        OMP_CLAUSE_OPERAND (OMP_CLAUSE_RANGE_CHECK (OMP_CLAUSE_CHECK (NODE),      \
>                                                OMP_CLAUSE_PRIVATE,       \
>                                                OMP_CLAUSE__SCANTEMP_), 0)
> 
> That's probably not intentional?  In that case, maybe simply move it at
> the end of the clause list?  (..., and generally then match that ordering
> in any 'switch'es, as applicable, and likewise position
> 'gcc/tree.h:OMP_CLAUSE_INDIRECT_EXPR' correspondingly.)

I have moved OMP_CLAUSE_INDIRECT to just before OMP_CLAUSE__SIMDUID_ so 
that it is outside the range checked by OMP_CLAUSE_SIZE and 
OMP_CLAUSE_DECL. I have also moved its handling in 
c(p)_parser_omp_clause_name so that the alphabetical ordering is 
preserved. Committed as trivial.

> I would've assumed handling for 'OMP_CLAUSE_INDIRECT' to also be
> necessary in the following places:
> 
>    - 'gcc/c-family/c-omp.cc:c_omp_split_clauses'
>    - 'gcc/cp/pt.cc:tsubst_omp_clauses',
>    - 'gcc/gimplify.cc:gimplify_scan_omp_clauses',
>      'gcc/gimplify.cc:gimplify_adjust_omp_clauses'
>    - 'gcc/omp-low.cc:scan_sharing_clauses' (twice)
>    - 'gcc/tree-nested.cc:convert_nonlocal_omp_clauses',
>      'gcc/tree-nested.cc:convert_local_omp_clauses'
>    - 'gcc/tree-pretty-print.cc:dump_omp_clause'
> 
> Please verify, and add handling as well as test cases as necessary, or,
> as applicable, put 'case OMP_CLAUSE_INDIRECT:' next to
> 'default: gcc_unreachable ();' etc., if indeed that clause is not
> expected there.

As Tobias noted, OMP_CLAUSE_INDIRECT never makes it into the middle-end. 
It may be generated by c(p)_parser_omp_all_clauses, and if present an 
attribute is applied to the function declaration, but at no point is it 
directly incorporated into the tree structure. I'm not sure whether it 
is best to explicitly list such cases as gcc_unreachable (it might imply 
that it can reach the ME, but just not at that particular point?) or not 
though.

Kwok

[-- Attachment #2: 0001-openmp-Adjust-position-of-OMP_CLAUSE_INDIRECT-in-Ope.patch --]
[-- Type: text/plain, Size: 4258 bytes --]

From a56a693a74dd3bee71b1266b09dbd753694ace94 Mon Sep 17 00:00:00 2001
From: Kwok Cheung Yeung <kcy@codesourcery.com>
Date: Wed, 3 Jan 2024 14:34:39 +0000
Subject: [PATCH] openmp: Adjust position of OMP_CLAUSE_INDIRECT in OpenMP
 clauses

Move OMP_CLAUSE_INDIRECT so that it is outside of the range checked by
OMP_CLAUSE_SIZE and OMP_CLAUSE_DECL.

2024-01-03  Kwok Cheung Yeung  <kcy@codesourcery.com>

	gcc/c/
	* c-parser.cc (c_parser_omp_clause_name): Move handling of indirect
	clause to correspond to alphabetical order.

	gcc/cp/
	* parser.cc (cp_parser_omp_clause_name): Move handling of indirect
	clause to correspond to alphabetical order.

	gcc/
	* tree-core.h (enum omp_clause_code): Move OMP_CLAUSE_INDIRECT to before
	OMP_CLAUSE__SIMDUID_.
	* tree.cc (omp_clause_num_ops): Update position of entry for
	OMP_CLAUSE_INDIRECT to correspond with omp_clause_code.
	(omp_clause_code_name): Likewise.
---
 gcc/c/c-parser.cc | 4 ++--
 gcc/cp/parser.cc  | 4 ++--
 gcc/tree-core.h   | 6 +++---
 gcc/tree.cc       | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/gcc/c/c-parser.cc b/gcc/c/c-parser.cc
index 64e436010d5..e7b74fb07f0 100644
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
@@ -14899,10 +14899,10 @@ c_parser_omp_clause_name (c_parser *parser)
 	    result = PRAGMA_OMP_CLAUSE_IN_REDUCTION;
 	  else if (!strcmp ("inbranch", p))
 	    result = PRAGMA_OMP_CLAUSE_INBRANCH;
-	  else if (!strcmp ("indirect", p))
-	    result = PRAGMA_OMP_CLAUSE_INDIRECT;
 	  else if (!strcmp ("independent", p))
 	    result = PRAGMA_OACC_CLAUSE_INDEPENDENT;
+	  else if (!strcmp ("indirect", p))
+	    result = PRAGMA_OMP_CLAUSE_INDIRECT;
 	  else if (!strcmp ("is_device_ptr", p))
 	    result = PRAGMA_OMP_CLAUSE_IS_DEVICE_PTR;
 	  break;
diff --git a/gcc/cp/parser.cc b/gcc/cp/parser.cc
index 1a6b53933a7..37536faf2cf 100644
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
@@ -37645,10 +37645,10 @@ cp_parser_omp_clause_name (cp_parser *parser)
 	    result = PRAGMA_OMP_CLAUSE_IN_REDUCTION;
 	  else if (!strcmp ("inbranch", p))
 	    result = PRAGMA_OMP_CLAUSE_INBRANCH;
-	  else if (!strcmp ("indirect", p))
-	    result = PRAGMA_OMP_CLAUSE_INDIRECT;
 	  else if (!strcmp ("independent", p))
 	    result = PRAGMA_OACC_CLAUSE_INDEPENDENT;
+	  else if (!strcmp ("indirect", p))
+	    result = PRAGMA_OMP_CLAUSE_INDIRECT;
 	  else if (!strcmp ("is_device_ptr", p))
 	    result = PRAGMA_OMP_CLAUSE_IS_DEVICE_PTR;
 	  break;
diff --git a/gcc/tree-core.h b/gcc/tree-core.h
index d1c7136c204..8a89462bd7e 100644
--- a/gcc/tree-core.h
+++ b/gcc/tree-core.h
@@ -350,9 +350,6 @@ enum omp_clause_code {
   /* OpenMP clause: doacross ({source,sink}:vec).  */
   OMP_CLAUSE_DOACROSS,
 
-  /* OpenMP clause: indirect [(constant-integer-expression)].  */
-  OMP_CLAUSE_INDIRECT,
-
   /* Internal structure to hold OpenACC cache directive's variable-list.
      #pragma acc cache (variable-list).  */
   OMP_CLAUSE__CACHE_,
@@ -497,6 +494,9 @@ enum omp_clause_code {
   /* OpenMP clause: filter (integer-expression).  */
   OMP_CLAUSE_FILTER,
 
+  /* OpenMP clause: indirect [(constant-integer-expression)].  */
+  OMP_CLAUSE_INDIRECT,
+
   /* Internally used only clause, holding SIMD uid.  */
   OMP_CLAUSE__SIMDUID_,
 
diff --git a/gcc/tree.cc b/gcc/tree.cc
index 82eff2bf34d..8aee3ef18d8 100644
--- a/gcc/tree.cc
+++ b/gcc/tree.cc
@@ -269,7 +269,6 @@ unsigned const char omp_clause_num_ops[] =
   2, /* OMP_CLAUSE_MAP  */
   1, /* OMP_CLAUSE_HAS_DEVICE_ADDR  */
   1, /* OMP_CLAUSE_DOACROSS  */
-  1, /* OMP_CLAUSE_INDIRECT  */
   2, /* OMP_CLAUSE__CACHE_  */
   2, /* OMP_CLAUSE_GANG  */
   1, /* OMP_CLAUSE_ASYNC  */
@@ -316,6 +315,7 @@ unsigned const char omp_clause_num_ops[] =
   0, /* OMP_CLAUSE_ORDER  */
   0, /* OMP_CLAUSE_BIND  */
   1, /* OMP_CLAUSE_FILTER  */
+  1, /* OMP_CLAUSE_INDIRECT  */
   1, /* OMP_CLAUSE__SIMDUID_  */
   0, /* OMP_CLAUSE__SIMT_  */
   0, /* OMP_CLAUSE_INDEPENDENT  */
@@ -362,7 +362,6 @@ const char * const omp_clause_code_name[] =
   "map",
   "has_device_addr",
   "doacross",
-  "indirect",
   "_cache_",
   "gang",
   "async",
@@ -409,6 +408,7 @@ const char * const omp_clause_code_name[] =
   "order",
   "bind",
   "filter",
+  "indirect",
   "_simduid_",
   "_simt_",
   "independent",
-- 
2.34.1