Paul-Antoine Arras wrote:
On 27/08/2026 13:25, Tobias Burnus wrote:
[…]
[…]
Adjusted as suggested.
Thanks for the updated patch (or rather: follow-up fixing patch)!
* * *
Regarding the following of the new patch:
@@ -6523,9 +6527,10 @@ handle_omp_array_sections_1 (tree c, tree t, vec<tree>
&types,
...
- /* NOTE: Stride/length are discarded for affinity/depend here. */
- if (discontiguous
- && *discontiguous
+ /* AFFINITY/DEPEND only need an address, not real array-section semantics;
+ stride/length are discarded for them here even when a discontiguous
+ section is permitted. */
+ if (*discontiguous
&& OMP_CLAUSE_CODE (c) != OMP_CLAUSE_AFFINITY
&& OMP_CLAUSE_CODE (c) != OMP_CLAUSE_DEPEND)
ret = grok_omp_array_section (OMP_CLAUSE_LOCATION (c), ret, low_bound,
Or likely more helpful the full diff against mainline:
+ /* AFFINITY/DEPEND only need an address, not real array-section semantics;
+ stride/length are discarded for them here even when a discontiguous
+ section is permitted. */
+ if (*discontiguous
+ && OMP_CLAUSE_CODE (c) != OMP_CLAUSE_AFFINITY
+ && OMP_CLAUSE_CODE (c) != OMP_CLAUSE_DEPEND)
+ ret = grok_omp_array_section (OMP_CLAUSE_LOCATION (c), ret, low_bound,
+ length, stride);
+ else
+ ret = grok_array_decl (OMP_CLAUSE_LOCATION (c), ret, low_bound, NULL,
+ tf_warning_or_error);
As mentioned before: I think the condition should just be:
if (*discontiguous)
and the comment removed. The reason is that OpenMP only permits strides
when explicitly permitted (→ 'Array Sections' section) and the only clauses
that permit them are the to/from clauses to 'target update' (→ 'Data-Motion
Control'
section).
It seems to be very odd to single out 'affinity' and 'depend' here. The
same applies to 'reduction' or '__CACHE__' or 'map' or ...
None of the clauses (but to/from) will ever have a user-specified stride and,
hence, they all refer to a contiguous block of memory (at least in C/C++) -
there
is nothing special about affinity/depend in this regard.
[Side note: While for affinity/depend, only the address matters,¹ the others
need
an address and a size, but this distinction does not matter here as all
non-strided code goes through build_array_ref.
¹the only-address matters is not according to the spec, which has a
*block* of memory ('storage location' [SIC]); however, all implementations
seem to only use the address.]
* * *
Code wise: in the parser, strides are only accepted with
OMP_ARRAY_SECTION_STRIDED,
which is only set for the TO and FROM clauses.
And the 'handle_omp_array_sections' function is called as followed:
handle_omp_array_sections's discontiguous argument is NULL for:
case OMP_CLAUSE_REDUCTION:
case OMP_CLAUSE_IN_REDUCTION:
case OMP_CLAUSE_TASK_REDUCTION:
case OMP_CLAUSE_DEPEND:
case OMP_CLAUSE_AFFINITY:
And passing the following variable on for
case OMP_CLAUSE_MAP:
case OMP_CLAUSE_TO:
case OMP_CLAUSE_FROM:
case OMP_CLAUSE__CACHE_:
bool discontiguous
= (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_TO
|| OMP_CLAUSE_CODE (c) == OMP_CLAUSE_FROM);
Then, value passed to handle_omp_array_sections_1 is:
int discontiguous = discontiguous_p && *discontiguous_p ? 1 : 0;
i.e. that variable is only != 0 for the TO/FROM clause.
And inside handle_omp_array_sections_1, 'discontiguous' is only
touched if != 0.
Thus, when the first '&&' is reached (i.e. discontiguous != 0), the
conditions
+ && OMP_CLAUSE_CODE (c) != OMP_CLAUSE_AFFINITY
+ && OMP_CLAUSE_CODE (c) != OMP_CLAUSE_DEPEND)
will always hold.
* * *
Thus, just checking for 'if (*discontiguous)' is simplest and just works.
* * *
Note: That's for the code in cp/semantics.cc.
The code in c/c-typeck.cc's handle_omp_array_sections_1 already is just:
- ret = build_array_ref (OMP_CLAUSE_LOCATION (c), ret, low_bound);
+ if (*discontiguous)
+ 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);
Thus, this is a C++-only issue.
* * *
Except for this 'if' condition + comment issue: LGTM.
Note: The patches 1/3 and 2/3 contain one patch but two change logs,
which is not permitted for a single commit. Thus, the commit log needs
to be updated. - Regarding the two follow up patches for 1/3 and the
one for 2/3: I think you could either merge those changes into the
main 1/3 and 2/3, respectively, commit - or commit them separately as
follow up.
I don't think that need to see the final version of the patch - the
'if' condition + comment change is trivial and I think you will sort
out the git commit log part yourself. - You could post the final
commit hashes for completeness, though.
* * *
* * *
Next on the review side:
* Fortran patch of this series (also ABI changing):
"[PATCH 3/3] OpenMP: Noncontiguous "target update" for Fortran"
https://gcc.gnu.org/pipermail/gcc-patches/2026-August/727574.html
* Important feature, but only relevant for C/C++:
"[PATCH 0/5] openmp: Support array-of-pointers (multi-segment)
noncontiguous array sections and array-shaping casts"
https://gcc.gnu.org/pipermail/gcc-patches/2026-August/726503.html
* * *
On the documentation side:
The implementation status of
Array shaping
Array sections with non-unit strides in C and C++
is then 'Y' - once the 1/3 + 2/3 patches are committed.
For
Discontiguous array section with target update construct
it is 'P'aritially ('Initial support for C and C++') until the
other above-mentioned patches have landed.
(→ https://gcc.gnu.org/onlinedocs/libgomp/OpenMP-5_002e0.html
and libgomp/libgomp.texi)
The doc can be updated as part of the 2/3 patch, as separate
patch right now - or deferred until after more patches have
landed - hopefully then with a full 'Y'.
* * *
Thanks!
Tobias
