On Mon, Feb 13, 2023 at 10:45:05AM +0000, Richard Sandiford wrote:
> Ping^3 [https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606741.html]
>
> ----
>
> Various parts of the omp code checked whether the size of a decl
> was an INTEGER_CST in order to determine whether the decl was
> variable-sized or not. If it was variable-sized, it was expected
> to have a DECL_VALUE_EXPR replacement, as for VLAs.
>
> This patch uses poly_int_tree_p instead, so that variable-length
> SVE vectors are treated like constant-length vectors. This means
> that some structures become poly_int-sized, with some fields at
> poly_int offsets, but we already have code to handle that.
>
> An alternative would have been to handle the data via indirection
> instead. However, that's likely to be more complicated, and it
> would contradict is_variable_sized, which already uses a check
> for TREE_CONSTANT rather than INTEGER_CST.
>
> gimple_add_tmp_var should probably not add a safelen of 1
> for SVE vectors, but that's really a separate thing and might
> be hard to test.
Generally, OpenMP has lots of clauses on lots of different constructs
and for SVE we need to decide what to do with them, and it would be better
to cover them all in testsuite coverage, so something orders of magnitude
larger than this patch provides and then there is OpenACC too.
Can one add these non-constant poly_int sized types as members of
aggregates? If yes, they need to be tested in addition to the plain
vectors.
>From data sharing clauses and others:
1) shared (implicit or explicit) - I'd say the non-constant poly_int sized
stuff should be shared by passing around an address, rather than by
copying it around by value which can be large (so say similar to
aggregates rather than scalars), though feel free to argue otherwise
2) for the offloading stuff, I'd say we want to error or sorry at
gimplification time, both for explicit or implicit map clause on
target/target data/target {enter,exit} data and on explicit/implicit
private and firstprivate clauses on target; while it could work fine
with host fallback, generally the intention is to offload to a different
device and neither PTX nor AMDGCN have anything similar to SVE and even
for say potential ssh based offloading to aarch64 there is the
possibility that the two devices don't agree on the vector sizes
3) private clause just creates another private variable of the same type,
except for target I think it should generally work fine, but should be
nevertheless test covered, say on parallel, task and some worksharing
construct (e.g. for) and simd and also with allocate clause specifying
allocators
4) firstprivate clause is similar to private, but we need to copy the
initial value to it; e.g. in case of parallel, host teams or task
such values are copied through compiler generated artificial struct
that contains all the stuff needed to be propagated around (and except
for task/taskloop then propagated back). For the SVE stuff I think it
might be nice to put the non-constant sized stuff late in the artificial
structure so that one can access the constant sized stuff using constant
offsets
5) lastprivate similar to private with copying value back from one
particular thread/lane (e.g. should be tested also on simd)
6) reduction/in_reduction/task_reduction - reductions are partly
privatization clauses, for SVE only user defined reductions apply
(declare reduction), but need to be tested in lots of constructs,
parallel, for, simd, taskloop, task reductions and inscan reductions
(explicit/implicit)
7) copyin - can the SVE vars be threadprivate (thread_local in C++ etc.)?
If not, at least needs testing
8) linear clause hopefully will reject SVE stuff, but needs testing
9) affinity clause - we just parse/ignore, but still needs testing
10) aligned clause on simd - again, needs testing
11) allocate clause - as I said, for most of the data sharing clauses
coverage needs to be without and with corresponding allocate clauses
12) depend clause - this one again needs testing, it just under the hood
takes address of the passed in variable, so probably should just work
out of the box
13) nontemporal clause on simd - probably works fine, but needs testing
14) uniform clause - this is on declare simd, one needs to test declare
simd with the various cases (vector without clause, uniform, linear)
15) enter/link clauses - as I said in 2), these IMHO should be rejected
16) detach clause - the clause requires specific type, so I think should be
ok
17) use_device_ptr/use_device_addr/is_device_ptr - offloading stuff, so
like 2)
18) inclusive/exclusive clauses - see 6) above for inscan reductions
19) map/has_device_addr - see 2)
20) doacross - requires specific format with scalars, so just check it is
properly rejected
Rest of clauses don't have arguments or have integral/logical expression
operands, so those should be ok.
Now, if the above is too much work for GCC 13 (likely it is), I think
it would be best to just make sure explicit/implicit shared clause works
fine (i.e. 1) ), perhaps a few other easy ones like 12) and simply sorry
on everything else for now and then incrementally handle it up later.
Jakub
