Hi Jakub,
On 29.06.22 19:02, Jakub Jelinek wrote:
On Wed, Jun 29, 2022 at 04:33:02PM +0200, Tobias Burnus wrote:
+ if (flag_openmp
+ && lookup_attribute ("omp declare target",
+ DECL_ATTRIBUTES (current_function_decl)))
+ omp_requires_mask
+ = (enum omp_requires) (omp_requires_mask | OMP_REQUIRES_TARGET_USED);
I thought the above would be left out, at least until clarified what exactly
we mean with device routines in the restrictions.
Missed that – I thought mostly of the API calls. However, I concur that
for 'declare target', it is not really needed as no data transfers or
reverse offloads can happen. - Additionally, I took this part from
Chung-Lin's patch and did not really think about this part ...
--- a/gcc/cp/parser.cc
+++ b/gcc/cp/parser.cc
And the above too.
On the Fortran side I don't see it being done.
(Likewise.)
--- a/gcc/lto-cgraph.cc
+++ b/gcc/lto-cgraph.cc
...
+ if (output_requires)
+ {
+ HOST_WIDE_INT val = ((HOST_WIDE_INT) omp_requires_mask
+ & (OMP_REQUIRES_UNIFIED_ADDRESS
+ | OMP_REQUIRES_UNIFIED_SHARED_MEMORY
+ | OMP_REQUIRES_REVERSE_OFFLOAD
+ | OMP_REQUIRES_TARGET_USED));
Why is the OMP_REQUIRES_TARGET_USED bit saved there? It is always set
if output_requires...
If we want to make sure it is set in omp_requires_mask after streaming
in, we can just or it in back there.
It is there because it is later needed: With -flto, we otherwise
wouldn't generate the variable in omp-offload.cc. And as this value is
only used internally, I thought it could just stay there. However, it
could also be excluded here and ...
@@ -1811,6 +1844,24 @@ input_offload_tables (bool do_force_output)
if (do_force_output)
varpool_node::get (var_decl)->force_output = 1;
}
+ else if (tag == LTO_symtab_edge)
+ {
+ static bool error_emitted = false;
+ HOST_WIDE_INT val = streamer_read_hwi (ib);
+
+ if (omp_requires_mask == 0)
+ omp_requires_mask = (omp_requires) val;
... added here as:
I mean here: (omp_requires) (val | OMP_REQUIRES_TARGET_USED);
... but that also requires ...
+ else if (omp_requires_mask != val && !error_emitted)
... something like:
(omp_requires_mask & ~OMP_REQUIRES_TARGET_USED) != val
or something like that.
+ {
+ char buf[64], buf2[64];
+ omp_requires_to_name (buf, sizeof (buf), omp_requires_mask);
+ omp_requires_to_name (buf2, sizeof (buf2), val);
+ error ("OpenMP %<requires%> directive with non-identical "
+ "clauses in multiple compilation units: %qs vs. %qs",
+ buf, buf2);
I think the user should be told also where, so file_data->file_name
With -save-temps, the filename is indeed helpful, e.g.:
"a-requires-1.o". However, without, I get names like: "/tmp/ccIgRkOW.o".
As mentioned, I was thinking of storing after the value some location
data, e.g. DECL_SOURCE_FILE() or even DECL_SOURCE_LOCATION() – by
keeping track of the first 'omp requires' in a translation unit.
Those can be streamed with streamer_write_string and
lto_output_location. However, the both require as argument an "struct
output_block" and in lto-cgraph.cc, I only have a "struct
lto_simple_output_block".
And for reading, I additionally need a "class data_in" argument.
Thus, I think it is doable – however, I was not quite sure whether it
made sense to do all the effort or not. (And it was not immediately
clear to me how to create a "data_in" class and ...)
Can't we arrange for GOMP_offload_register_ver to see the bitmasks somewhere
instead (and force GOMP_offload_register_ver even if there are no vars or
funcs and just the requires mask)?
This probably works – but means some restructuring.
GOMP_offload_register_ver assumes that num_devices is already available
– and we need to exclude those for which the 'omp requires' cannot be
fulfilled.
I think this could be either be done in GOMP_offload_register_ver by
decrementing num_offload_images + shifting the offload_images[i] entries
(or have some table to match user-visible numbers to the original number) .
Or it could just be done by setting a flag – and num_offload_images
updated later. We probably need something which is run later to exclude
those devices for which no image exists (existing but not supported
devices) – and for OpenMP 6's OMP_AVAILABLE_DEVICES env, which permits
to sort the devices and filter out some of them.
Could be the weak __offload_requires_mask (but we probably e.g. can't assume
declare_weak will work),
I assume that it does work in practice, given that it is only needed on
the host – and given which systems effectively support offloading. –
With -flto, we even would know that there is only one variable, but
unfortunately, we cannot count on a host lto1 run.
Or we could ask for the offloading lto1 when it merges those from different
offloadng TUs to emit some magic symbol in what it emits and have mkoffload
search for it.
Or mkoffload could pass some option to the offloading lto compilation, say
filename of a temporary file, let lto1 save into that file the bitmask and
let mkoffload read it. Or env var.
(Can surely be done – having a constant in the GOMP_offload_register_ver
call would be surely useful.)
--- a/libgomp/libgomp-plugin.h
+++ b/libgomp/libgomp-plugin.h
...
-extern int GOMP_OFFLOAD_get_num_devices (void);
+extern int GOMP_OFFLOAD_get_num_devices (unsigned int);
This is an ABI change for plugins, don't we want to e.g. rename
the symbol as well, so that plugin mismatches are caught more easily?
Yes, I recall that we talked about it, but I obviously missed to
actually change it. If we go for GOMP_offload_register_ver + updating
the list later, this function can probably stay as is – and we need
another func to query whether the requirements are fulfillable or not.
Tobias
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
