Tobias Burnus wrote:
while I still need to continue looking through the patches
(1/3 + this follow up), I thought I could already start by
sending you three ICE examples and two please-add wish for
an existing diagnostic and for missing diagnostic.
* * *
I start first with comments done in 2/3 - the actual example code
parts are handled by this follow-up patch, i.e. [::] / [n::] now
actually work. (I think that was the only C++ applicable part of
those examples.)
* * *
First an RFC:
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:
... and likewise in gcc/cp/parser.h:
+#define OMP_ARRAY_SECTION_NONE 0
+#define OMP_ARRAY_SECTION_UNSTRIDED 1
+#define OMP_ARRAY_SECTION_STRIDED 2
+ unsigned char omp_array_section_kind;
which then gets used as:
@@ -9889 +9960,2 @@ cp_parser_parenthesized_expression_list (cp_parser* parser,
- bool saved_omp_array_section_p;
+ unsigned char saved_omp_array_section_kind;
besides some 'auto' + 'make_temp_override' use.
In C++, it comes into existence via ggc_cleared_alloc in
cp_parser_new - instead of being a global var as in C.
Albeit thanks to 'make_temp_override' it seems to be less
important. (Although, whatever is chosen, it probably makes
sense to use either an enum or not in both C and C++.)
* * *
* * *
+cp_omp_create_arrayshape_type (location_t loc, tree expr,
...
+ error ("OpenMP array shaping operator with non-pointer argument");
As written in the 2/3 patch:
Can we have an 'error_at' here? I guess "EXPR_LOCATION (expr)" will
work. [This also applies to the C++ FE code.]
* * *
* * *
This patch changes:
can if MAYBE_ZERO_LEN is false. MAYBE_ZERO_LEN will be true in the above
case though, as some lengths could be zero. */
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)
and
+/* 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)
However, I think the same applies here as what I wrote to patch 2/3
This is mostly asking for adding argument documentation - possibly
with changing the internal representation as that might be clearer
and makes also writing the documentation easier.
I also would like to see one or two minor changes of code which
is more confusing than helpful and the like.
Full verbatim quote from 2/3, which seems to be fully applicable.
[Sorry, probably should be copy-edited it - as it is partially
backward as I discovered more.]
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.
* * *
* * *
And finally:
+static tree
+omp_noncontig_descriptor_type (location_t loc)
+{
+ static tree cached = NULL_TREE;
+
+ if (cached)
+ return cached;
...
+ tree desc_type = omp_noncontig_descriptor_type (UNKNOWN_LOCATION);
I think it is cleaner to have no argument to the function
and use
location_t loc = UNKNOWN_LOCATION;
in the function. It seems to be unlikely that we ever want to have a location
and that this location makes sense, given that the type-declaration is cached.
* * *
Otherwise, I did not spot anything (besides the ICEs report before in this
thread),
i.e. except for the reported issues both the C++ and C patches should fine -
such
that hopefully the next version can just land.
Thanks again to PA for taking care of Julian's patches!
Tobias
PS: Next is the Fortran patch that modifies the used descriptor [ABI] (and
implements
the feature for Fortran, obviously]. There is also a follow up patch that
handles
multi-segment noncontiguous array sections →
https://gcc.gnu.org/pipermail/gcc-patches/2026-August/726503.html
