Hi Julian,
first round of comments - I think I need a second pass as the patch is
long and complex. The depth of review also was decreasing, hence, I
assume I will spot things in later parts of the compiler.
In any case, I think the patch is a huge leap forward and very useful!
Contrary to previous iterations of the patch series, it looks as if
{1,2}/5 could be sensibly applied before {3,4,5}/5. However, those are
smaller or tiny, it should be way quicker to review them!
* * *
CROSS REFS
This patch is part of the patch set
"[PATCH v7 0/5] OpenMP/OpenACC: map clause and OMP gimplify rework"
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/627895.html
This patch set in turn is required for the follow-up patch set
https://gcc.gnu.org/pipermail/gcc-patches/2023-September/629363.html
'[PATCH 0/8] OpenMP: lvalue parsing and "declare mapper" support'
and it looks as if it would be very useful for some other WIP patches
like map+iterator.
* * *
The patch 1/5 is just an indentation patch to make the following patch
smaller - and it has been approved multiple times.
This patch can be found at
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/627897.html
in case someone wants to read the full patch or Julian's patch-intro
comments.
* * *
I do note that all 5 patches of this patch set apply cleanly with some
fuzzy line matching, but:
WARNING: at least for this patch (2/5), code gets wrongly applied
in (c-)parser.cc - see below.
On 19.08.23 00:47, Julian Brown wrote:
This patch reworks clause expansion in the C, C++ and (to a lesser
extent) Fortran front ends for OpenMP and OpenACC mapping nodes used in
GPU offloading support.
At present a single clause may be turned into several mapping nodes,
or have its mapping type changed, in several places scattered through
the front- and middle-end. The analysis relating to which particular
transformations are needed for some given expression has become quite hard
to follow. Briefly, we manipulate clause types in the following places:
[...]
[As the patch is very large, I copied the interesting bits out of the patch
and comment on them - I hope that preserves enough context to be useful.]
tree c_omp_address_inspector::get_address ()
This class member function is unused. It returns 'orig' and that member variable
is used a couple of times in the member functions - but not this function.
I think it can be removed. (I also find the name slightly misleading.)
* * *
+c_omp_address_inspector::maybe_zero_length_array_section (tree clause)
+{
+ switch (OMP_CLAUSE_MAP_KIND (clause))
+{
+case GOMP_MAP_ALLOC:
+case GOMP_MAP_IF_PRESENT:
+case GOMP_MAP_TO:
+case GOMP_MAP_FROM:
+case GOMP_MAP_TOFROM:
+case GOMP_MAP_ALWAYS_TO:
+case GOMP_MAP_ALWAYS_FROM:
+case GOMP_MAP_ALWAYS_TOFROM:
+case GOMP_MAP_RELEASE:
+case GOMP_MAP_DELETE:
+case GOMP_MAP_FORCE_TO:
+case GOMP_MAP_FORCE_FROM:
+case GOMP_MAP_FORCE_TOFROM:
+case GOMP_MAP_FORCE_PRESENT:
Shouldn't this also include:
GOMP_MAP_ALWAYS_PRESENT_TO:
GOMP_MAP_ALWAYS_PRESENT_FROM:
GOMP_MAP_ALWAYS_PRESENT_TOFROM:
GOMP_MAP_PRESENT_ALLOC:
GOMP_MAP_PRESENT_TO:
?
* * *
+omp_expand_access_chain (tree c, tree expr, vec _tokens,
+unsigned *idx)
+{
+ using namespace omp_addr_tokenizer;
+ location_t loc = OMP_CLAUSE_LOCATION (c);
+ unsigned i = *idx;
+ tree c2 = NULL_TREE;
+ gomp_map_kind kind
+= (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_FROM
+ || (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP
+ && (OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FROM
+ || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_DELETE
+ || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_RELEASE
+ || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_ALWAYS_FROM
+ || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_FORCE_FROM
+ || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_PRESENT_FROM)))
+ ? GOMP_MAP_DETACH : GOMP_MAP_ATTACH;
GOMP_MAP_ALWAYS_PRESENT_FROM missing?
And I personally find it more readable to have
if (...
...
...)
kind = GOMP_MAP_DETACH;
else
kind = GOMP_MAP_ATTACH;
as in the assignment above, the lhs is in this case really far from the rhs.
* * *
I do see in omp_expand_access_chain, which is shared by C and C++, that the
following
accesses are handled:
ACCESS_POINTER,
ACCESS_POINTER_OFFSET,
ACCESS_INDEXED_ARRAY,
But not:
ACCESS_REF_TO_POINTER,
ACCESS_REF_TO_POINTER_OFFSET,
ACCESS_INDEXED_REF_TO_ARRAY
Do those need to be handled as well?
SIDENOTE: As Fortran allocatable components, reference-type variables in a C++
class ('class', 'struct')
are automatically be copied when doing 'map(var)' - contrary to pointer members
that need to be explicitly
be mapped (either at mapping side or via a mapper). (And contrary to Fortran
allocatables, reference-type members
are always "there" - not like with allocatables can change from unallocated to
allocated - with all compilations
of
