Hi PA,
Paul-Antoine Arras wrote:
From: Julian Brown<[email protected]>
Following the similar support for C++, here is the C implementation for
the OpenMP 5.0 array-shaping operator, and for strided and rectangular
updates for "target update".
Doing the FE review in reverse, starting with the C FE review ...
I guess some comments also apply to the C++ FE.
* * *
Zeroth: Thanks for taking care of upstreaming Julian's patch and
doing some fixes and cleanup there!
First, can you check
https://github.com/OpenMP/Examples/blob/main/devices/sources/array_shaping.1.c
unless I screwed up, it fails here with the C compiler but not the C++
compiler:
devices/sources/array_shaping.1.c: In function ‘array_shaping’:
devices/sources/array_shaping.1.c:26:15: internal compiler error: in
c_set_type_bits, at c/c-typeck.cc:466
26 | #pragma omp target update from( (([nx][ny+2])a)[0:nx:2][1], \
| ^~~
0x2a6d69f internal_error(char const*, ...)
../../../repos/gcc/gcc/diagnostic-global-context.cc:787
0xa761f9 fancy_abort(char const*, int, char const*)
../../../repos/gcc/gcc/diagnostics/context.cc:1813
0x7bd7f8 c_set_type_bits
../../../repos/gcc/gcc/c/c-typeck.cc:466
0xab82a5 build_omp_array_section(unsigned long, tree_node*, tree_node*,
tree_node*, tree_node*)
../../../repos/gcc/gcc/c/c-typeck.cc:3776
* * *
Talking about ICE, the OpenMP_VV Fortran tests,
tests/5.1/target_update/test_target_update_iterator.F90
https://github.com/OpenMP-Validation-and-Verification/OpenMP_VV/blob/master/tests/5.1/target_update/test_target_update_iterator.F90
fails once the Fortran patch is applied - it works without the patch and does
not
use the new features. Namely:
internal compiler error: in gfc_conv_is_contiguous_expr, at
fortran/trans-intrinsic.cc:2334
0x2b0555f internal_error(char const*, ...)
../../../repos/gcc/gcc/diagnostic-global-context.cc:787
0xa6fe0b fancy_abort(char const*, int, char const*)
../../../repos/gcc/gcc/diagnostics/context.cc:1813
0x7e0eeb gfc_conv_is_contiguous_expr(gfc_se*, gfc_expr*)
../../../repos/gcc/gcc/fortran/trans-intrinsic.cc:2334
0xc609ac gfc_trans_omp_clauses
../../../repos/gcc/gcc/fortran/trans-openmp.cc:5911
0xc7126d gfc_trans_omp_target_update
../../../repos/gcc/gcc/fortran/trans-openmp.cc:10259
* * *
The following seems to be completely valid C:
void f() {
int A[10], B[10][10];
#pragma omp target update to(A[::], B[::][::1])
}
but it gets rejected with:
r.c:3:32: error: expected expression before ‘::’ token
3 | #pragma omp target update to(A[::], B[::][::1])
IMHO this should just work in C.
* * *
For C++, I think 'A[::]' is supposed to be likewise valid
but gets rejected:
r.c:4:34: error: expected id-expression before ‘]’ token
4 | #pragma omp target update to(A[::])
| ^
Talking about C++, the following works:
int n;
void f() {
int A[10], B[10][10];
#pragma omp target update to(A[::n])
}
as expected, but failing is:
r.c:4:34: error: expected id-expression before ‘]’ token
4 | #pragma omp target update to(A[::], B[0][: : ])
| ^
r.c:4:44: error: expected primary-expression before ‘:’ token
4 | #pragma omp target update to(A[::], B[0][: : ])
| ^
I think this should work by the same reasoning as for '[: :]'.
And I am pretty sure that '[::n :]' and '[:]' works with map
clauses with g++.
* * *
Also failing - but not really different:
r.c:4:49: error: expected primary-expression before ‘:’ token
4 | #pragma omp target update to(A[1:2:], B[:1:][1: : ])
I think some of them - esp. the last one - I already reported.
* * *
And failing but trickier due to the sequence of > 3 colons:
[Cf. OpenMP 6.x and discussions, cf. OpenMP spec issue #4740]:
r.c:4:34: error: expected id-expression before ‘:’ token
4 | #pragma omp target update to(A[:::n], B[:::n][: :: n : ])
| ^
r.c:4:43: error: expected id-expression before ‘:’ token
4 | #pragma omp target update to(A[:::n], B[:::n][: :: n : ])
| ^
where the A[::n] is an array element (this works) and
:::n is the length '::n' and ::::n is the stride '::n'.
* * *
2026-08-13 Paul-Antoine Arras<[email protected]>
gcc/c/
* c-tree.h (c_omp_array_section_p): Replace bool with...
(c_omp_array_section_kind): ...this 3-state unsigned char.
(OMP_ARRAY_SECTION_NONE, OMP_ARRAY_SECTION_UNSTRIDED,
OMP_ARRAY_SECTION_STRIDED): Define.
* c-parser.cc (c_parser_postfix_expression_after_primary): Only
accept a 2rd colon (stride) for to/from clauses, not map.
(c_parser_omp_variable_list): Only count OMP_ARRAY_SECTION
layers, not every ARRAY_REF layer, in a to/from clause's base
decl.
* c-typeck.cc (handle_omp_array_sections): Handle a genuinely
discontiguous update at any access-chain depth.
(c_finish_omp_clauses): Splice the GOMP_MAP_TO_GRID/FROM_GRID
replacement clause in at *PC when DISCONTIGUOUS == 2.
I think it would be good to have a testcase.
* * *
Additionally, I wonderwhether it wouldn't be cleaner to have an
'enum ... : unsigned char' instead of a bare 'unsigned char' – it
adds some type safety and also makes it easier to see what it is about.
The patch does in c-tree.h:
-extern bool c_omp_array_section_p;
+#define OMP_ARRAY_SECTION_NONE 0
+#define OMP_ARRAY_SECTION_UNSTRIDED 1
+#define OMP_ARRAY_SECTION_STRIDED 2
+extern unsigned char c_omp_array_section_kind;
* * *
Can you update the following comment in c-typeck.c?
'True' no longer makes sense:
/* True when parsing OpenMP map clause. */
-bool c_omp_array_section_p;
+unsigned char c_omp_array_section_kind;
Code wise, there is is still the assumption that the following
condition works:
c-typeck.cc: if (TREE_TYPE (ref) == error_mark_node &&
!c_omp_array_section_kind)
i.e. that '(bool) OMP_ARRAY_SECTION_NONE' evaluates to false.
I think it would be cleaner to compare against NONE, esp. if/once
changing to an enum.
* * *
+create_omp_arrayshape_type (tree expr, vec<tree> *omp_shape_dims)
+{
...
+ error ("OpenMP array shaping operator with non-pointer argument");
Can we have an 'error_at' here? I guess "EXPR_LOCATION (expr)" will work.
[This also applies to the C++ FE code.]
* * *
@@ -15846,7 +15892,7 @@ c_finish_omp_cancellation_point (location_t loc, tree
clauses)
static tree
handle_omp_array_sections_1 (tree c, tree t, vec<tree> &types,
bool &maybe_zero_len, unsigned int &first_non_one,
- enum c_omp_region_type ort)
+ enum c_omp_region_type ort, int *discontiguous)
{
...
+/* Handle array sections for clause C. On entry *DISCONTIGUOUS is 0 if array
+ section must be contiguous, 1 if it can be discontiguous, and in the latter
+ case it is set to 2 on exit if it is determined to be discontiguous during
+ the function's execution. */
static bool
-handle_omp_array_sections (tree &c, enum c_omp_region_type ort)
+handle_omp_array_sections (tree &c, enum c_omp_region_type ort,
+ int *discontiguous = NULL)
First and unrelated to this patch, I think it would be helpful to document
the return value of these two functions (handle_omp_array_sections_1 and
handle_omp_array_sections). Namely,
The aux _1 function returns a tree - the other a Boolean.
For the latter 'true' means an error, the former returns
error_mark_node in case of an error, for non OpenMP array sections
essentially 't' (except for a convert_lvalue_to_rvalue conversion
in a corner case) - and for array sections either build_array_ref or
build_omp_array_section.
* * *
I think the aux function needs also comment documenting the discontiguous
argument.
For the handle_omp_array_sections_1 aux function:
If known to be discontiguous:
* Print an error if discontiguous is a nullptr or *discontiguous == 0
* Otherwise, set *discontiguous = 2
If discontiguous && *discontiguous != 0 - build_omp_array_section.
For handle_omp_array_sections:
If discontiguous && *discontiguous
* When the array might be noncontigous, set *discontiguous = 2
* If at the end *discontiguous remains with value != 2, i.e. the
array section is known to be contiguous, convert the array section
back to an build_array_ref.
Hence:
For the aux function, '2' strictly means that it is known to
be noncontiguous.
For the other function, '2' means that it might be noncontigous,
'1' that it is known to be noncontigous, and '0' that semantic
requires noncontigous (with a compile time check, but still could
go wrong at runtime).
Noncontiguous can be either if stride != 1 [with some exceptions] but
also for multidimension variables for, e.g. 'arr[1:n][0:3]'
if 'int arr[...][size]' and size > 3 [except n == 1].
* * *
I wonder whether it wouldn't be cleaner to make DISCONTIGUOUS
required in the aux function. Using the current argument wording
for handle_omp_array_sections could be reused.
Otherwise, it should mention that nullptr and *discontigous == 0
both mean the same. - The nullptr bit could be removed, if the
pointer argument is alwas passed (like in the variant below):
* * *
Regarding the required part, that's because I wonder whether it is
cleaner to use ...
... in handle_omp_array_sections, the argument 'bool *discontiguous_p'
with:
int discontigous = discontiguous_p && *discontiguous_p ? 1 : 0;
...
// Discontiguous permitted but known to be contiguous.
if (discontigous == 1)
discontigous_p = false;
and passing this 'discontiguous' as argument to the aux function (i.e
it will never be a nullptr in the aux function).
Alternatively, if keeping it as integer, I wonder whether it would
be cleaner to set 'discontiguous' to '0' if known to be known to be
contiguous instead of keeping the old value.
If keeping it as integer: It should be made clear that '2' implies
that it might be discontigous (but it might be only known at runtime)
while the original value (or if set to zero, the value 0) means that
it is known to be contiguous.
And in in either case (bool* or int*), it should be made clear that
a nullptr acts like setting the value to 0 / false.
* * *
If we know that *discontiguous can only be 0 or two, the following
+ if (discontiguous && *discontiguous != 2)
+ first = omp_array_section_low_bound (OMP_CLAUSE_LOCATION (c), first);
could be replaced by '*discontigous == 1' - as 0 won't create an array section
and for 2 we want to retain it.
* * *
Actually, I think my claim that _1 only sets it to 2 when it is known to
be noncontiguous is not quite right:
/* If there is a pointer type anywhere but in the very first
array-section-subscript, the array section could be non-contiguous.
*/
if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_DEPEND
&& OMP_CLAUSE_CODE (c) != OMP_CLAUSE_AFFINITY
&& TREE_CODE (TREE_OPERAND (t, 0)) == OMP_ARRAY_SECTION)
{
/* If any prior dimension has a non-one length, then deem this
array section as non-contiguous. */
for (tree d = TREE_OPERAND (t, 0);
TREE_CODE (d) == OMP_ARRAY_SECTION;
d = TREE_OPERAND (d, 0))
{
tree d_length = TREE_OPERAND (d, 2);
- if (d_length == NULL_TREE || !integer_onep (d_length))
+ tree d_stride = TREE_OPERAND (d, 3);
+ if (d_length == NULL_TREE || !integer_onep (d_length)
+ || (d_stride && !integer_onep (d_stride)))
Assume:
to(arr[:1:m]) or to(arr[:1:2])
Those are the elements:
{ lower-bound, ... , lower-bound + ((length - 1) * stride) }
Namely, only '{ lower-bound }' remains - such that the array section
can still be contiguous.
On the other hand, for:
to(arr[:2]) or to(arr[:n])
the condition is already true - independent whether there is any stride or not.
(for n == 1 it would be still contiguous - but that's not handled in this very
special case.)
Thus, I think we can remove the 'd_stride' here - as it either leads either to
a false positive or is redundant.
* * *
Hence, I think for
+handle_omp_array_sections (tree &c, enum c_omp_region_type ort,
....
+ if (!integer_onep (stride)
+ || (higher_discontiguous
+ && (!integer_zerop (low_bound)
+ || !full_span)))
+ *discontiguous = 2;
+
+ if (!integer_onep (stride)
+ || !integer_zerop (low_bound)
+ || !full_span)
+ higher_discontiguous = true;
IMHO, it should be for both:
if ((!integer_onep (stride) && !integer_onep (length)
for the same reasons (if length is one, the stride >= 1
does not matter).
* * *
- if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_MAP)
+ if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_MAP
+ && !(discontiguous && *discontiguous == 2))
return false;
I wonder whether a comment would help:
// Done, except for MAP clauses and for array sections that may be
noncontiguous
At least I find it otherwise a bit hard to read with the ! (not) and more
complex
conditions - especially as at a glance, there is no real relation between one
and
the other. (Namely: Why aren't TO/FROM handled the same way as map, if
contiguous?
Answer: See longer comment below.)
Side note: c_omp_address_inspector (which is later called) actually turns the
TO/FROM clause to a MAP clause of map kind GOMP_MAP_{TO,FROM}_GRID; however,
that happens after this check (and is actually the reason for continuing).
* * *
Back to the aux function:
@@ -16198,14 +16277,42 @@ handle_omp_array_sections_1 (tree c, tree t, vec<tree>
&types,
- ret = build_array_ref (OMP_CLAUSE_LOCATION (c), ret, low_bound);
+ /* NOTE: Stride/length are discarded for affinity/depend here. */
+ if (discontiguous
+ && *discontiguous
+ && OMP_CLAUSE_CODE (c) != OMP_CLAUSE_AFFINITY
+ && OMP_CLAUSE_CODE (c) != OMP_CLAUSE_DEPEND)
+ ret = build_omp_array_section (OMP_CLAUSE_LOCATION (c), ret, low_bound,
+ length, stride);
+ else
+ ret = build_array_ref (OMP_CLAUSE_LOCATION (c), ret, low_bound);
return ret;
Can we remove the comment and clause-code checks here? Namely:
I have to admit that I find the comment and the check for
affinity/depend more confusing than helpful.
All callers either pass no discontiguous argument (→ NULL default),
except for one:
+ int discontiguous
+ = (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_TO
+ || OMP_CLAUSE_CODE (c) == OMP_CLAUSE_FROM);
+ if (handle_omp_array_sections (c, ort, &discontiguous))
And this one explicitly handles those two clauses.
The reason that I find it odd is that the aux function is also called
for a much of other clauses, e.f., OMP_CLAUSE_MAP or OMP_CLAUSE__CACHE_
for which the same applies.
* * *
Tobias
