On Thu, Jun 09, 2022 at 02:46:34PM +0200, Tobias Burnus wrote:
> On 09.06.22 13:40, Jakub Jelinek via Gcc-patches wrote:
> > On Wed, Jun 08, 2022 at 05:56:02AM +0200, Tobias Burnus wrote:
> > > + && lookup_attribute ("omp declare target",
> > > + DECL_ATTRIBUTES (current_function_decl)))
> > > + omp_requires_mask
> > > + = (enum omp_requires) (omp_requires_mask |
> > > OMP_REQUIRES_TARGET_USED);
> > I must admit it is unclear what the
> > "must appear lexically before any device constructs or device routines."
> > restriction actually means for device routines.
> > Is that lexically before definition of such device routines, or even their
> > declarations?
> I have similar issues – also for Fortran (and C++) module use. Hence, I
> had filled https://github.com/OpenMP/spec/issues/3240 (not publicly
> accessible); I added your issues to the list.
> > The above patch snippet is I believe for function definitions that were
> > arked as declare target before the definition somehow (another decl for
> > it merged with the new one or in between the begin/end). And is true
> > even for device_type (host), to rule that out you'd need to check for
> > "omp declare target host" attribute not being present.
> > I'm not against the above snippet perhaps adjusted for device_type(host),
> > but IMHO we want clarifications from omp-lang
> How to proceed for now? And does 'omp_is_initial_device()' on the host a
> device function or not? It can be hard-coded to 'true' ...
If it is from me, bet it was because of that (mis)understanding that
device routines are device related runtime API calls.
I'd suggest to only mark in the patch what is clear (which is device
constructs) and defer the rest until it is clarified.
> > For Fortran, is the above mostly not needed because requires need to be in
> > the specification part and device constructs are executable and appear in
> > the part after it? Do we allow requires in BLOCK's specification part?
> We don't allow it in BLOCK – but there are issues related to USE-ing
> modules, cf. OpenMP issue.
Ack.
> In terms of parsing, it makes no difference – contrary to
> 'unified_shared_memory', where the parser could decide not to add
> implicit mapping, the compiler part is not affected by API calls.
Yeah. So perhaps on the standard side we should just keep the
lexically before device constructs (and metadirective/declare variant
device related resolution) in the restriction, but say that TUs
that have device constructs and device runtime APIs (or whatever is agreed)
imply that requires mask must be the same in all of them.
> > Shouldn't the vars in that section be const, so that it is a read-only
> > section?
> >
> > Is unsigned_type_node what we want (say wouldn't be just unsigned_char_node
> > be enough, currently we just need 3 bits).
>
> Probably -that would be 8 bits, leaving 5 spare. I have not checked what
> Andrew et al. do with the pinned-memory support by -f<some-flag>, but
> that will likely use only 1 to 3 bits, if any.
If it is SHF_MERGE, even 16-bit or 32-bit wouldn't be the end of the world,
or if it is in LTO streamed out stuff, we can use a bitpack for it...
> > Also, wonder if for HAVE_GAS_SHF_MERGE && flag_merge_constants
> > we shouldn't try to make that section mergeable. If it goes away during
> > linking and is replaced by something, then it doesn't matter, but otherwise,
> > as we don't record which TU had what flags, all we care about is that
> > there were some TUs which used device construct/routines (and device APIs?)
> > and used bitmask 7, other TUs that used bitmask 3 and others that used
> > bitmask 4.
> (maybe – I am not sure about this, either.)
> > @@ -442,6 +463,14 @@ omp_finish_file (void)
> > }
> > else
> > {
> > +#ifndef ACCEL_COMPILER
> > + if (flag_openmp
> > + && (omp_requires_mask & OMP_REQUIRES_TARGET_USED)
> > + && (omp_requires_mask & (OMP_REQUIRES_UNIFIED_ADDRESS
> > + | OMP_REQUIRES_UNIFIED_SHARED_MEMORY
> > + | OMP_REQUIRES_REVERSE_OFFLOAD)))
> > + sorry ("OpenMP device offloading is not supported for this target");
> > +#endif
> > I don't understand this snippet. Without named sections on the host,
> > I bet we simply don't support offloading at all,
> > the record_offload_symbol target hook is only non-trivially defined
> > for nvptx and nvptx isn't typical host for OpenMP offloading,
> > because we don't remember it anywhere.
>
> I thought that would address your: "This probably needs to sorry if the
> target doesn't support named sections. We probably don't support LTO in
> that case either though."
But sorry means we will fail to compile it. Perhaps
inform would be better, but then we don't complain (warn/inform)
if no offloading targets are configured. And, presence of requires
unified*/reverse_offload as the reason for the diagnostics rather than
say presence of declare target functions would be weird.
I think best would be a fatal error if people try to configure
offloading targets for a compiler that doesn't support named sections,
or perhaps that and presence of anything that should be offloaded.
Jakub
