Hi Chung-Lin, hi Thomas, hello world,
some thoughts glancing at the patch.
Chung-Lin Tang wrote:
There is still some shortcomings in the current state, mainly that only explicit-shaped
arrays can be used (like its C counterpart). Anything else is currently a bit more
complicated in the middle-end, since the existing reduction code creates an
"init-op" (literal of initial values) which can't be done when say
TYPE_MAX_VALUE (TYPE_DOMAIN (array_type)) is not a tree constant. I think we'll be on the
hook to solve this later, but I think the current state is okay to submit.
I think having some initial support is fine, but it needs an
understandable and somewhat complete error diagnostic and testcases.
More to this below.
+ if (!TREE_CONSTANT (min_tree) || !TREE_CONSTANT (max_tree))
+ {
+ error_at (loc, "array in reduction must be of constant size");
+ return error_mark_node;
+ }
Shouldn't this use a sorry_at instead?
+ /* OpenACC current only supports array reductions on explicit-shape
+ arrays. */
+ if ((n->sym->as && n->sym->as->type != AS_EXPLICIT)
+ || n->sym->attr.codimension)
gfc_error ("Array %qs is not permitted in reduction at %L",
n->sym->name, &n->where);
[Coarray excursion. I am in favor of allowing it for the reasons above,
but it could be also rejected but I would prefer to have a proper error
message in that case.]
While coarrays are unspecified, I do not see a reason why a corray
shouldn't be permitted here – as long as it is not coindexed. At the
end, it is just a normal array with some additional properties, which
make it possible to remotely access it.
Note: For coarray scalars, we have 'sym->as', thus the check should be
'(n->sym->as && n->sym->as->rank)' to permit scalar coarrays.
* * *
Coarray excursion: A coarray variables exists in multiple processes
("images", e.g. MPI processes). If 'caf' and 'caf2' are coarrays, then
'caf = 5' and 'i = caf2' refer to the local variable.
On the other hand, 'caf[n] = 5' or 'i = caf[3,m]' refers to the 'caf'
variable on image 'n' or [3,m]', respectively, which implies in general
some function call to read or set the remote data, unless the memory is
directly accessible (→ e.g. some offset calculation) and the compiler
already knows how to handle this.
While a coarrary might be allocated in some special memory, as long as
one uses the local version (i.e. not coindexed / without the image index
in brackets).
Assume for the example above, e.g., integer :: caf[*], caf2[3:6, 7:*].
* * *
Thus, in terms of OpenACC or OpenMP, there is no reason to fret a
coarray as long as it is not coindexed and as long as OpenMP/OpenACC
does not interfere with the memory allocation – either directly ('!$omp
allocators') or indirectly by placing it into special memory (pinned,
pseudo-unified-shared memory → OG13's -foffload-memory=pinned/unified).
In the meanwhile, OpenMP actually explicitly allows coarrays with few
exceptions while OpenACC talks about unspecified behavior.
* * *
Back to generic comments:
If I look at the existing code, I see at gfc_match_omp_clause_reduction:
if (gfc_match_omp_variable_list (" :", &c->lists[list_idx], false, NULL,
&head, openacc, allow_derived) !=
MATCH_YES)
If 'openacc' is true, array sections are permitted - but the code added
(see quote above) does not handle n->expr at all and only n->sym.
I think there needs to be at least a "gfc_error ("Sorry, subarrays/array
sections not yet handled" [subarray is the OpenACC wording, 'array
section' is the Fortran one, which might be clearer.
But you could consider to handle at least array elements, i.e.
n->expr->rank == 0.
Additionally, I think the current error message is completely unhelpful
given that some arrays are supported but most are not.
I think there should be also some testcases for the not-yet-supported
case. I think the following will trigger the omp-low.cc 'sorry_at' (or
currently 'error' - but I think it should be a sorry):
subroutine foo(n)
integer :: n, A(n)
... reduction(+:A)
And most others will trigger in openmp.cc; for those, you should have an
allocatable/pointer and assumed-shape arrays for the diagnostic testcase
as well.
* * *
I have not really experimented with the code, but does it handle
multi-dimensional constant arrays like 'integer :: a(3:6,10,-1:1)' ? — I
bet it does, at least after handling my example [2] for the C patch [1].
Thanks,
Tobias
[1] https://gcc.gnu.org/pipermail/gcc-patches/2024-January/641669.html
[2] https://gcc.gnu.org/pipermail/gcc-patches/2024-March/647704.html
