On 08/07/2022 10:00, Tobias Burnus wrote:
On 08.07.22 00:18, Andrew Stubbs wrote:
Likewise, the 'requires' mechanism could then also be used in '[PATCH
16/17] amdgcn, openmp: Auto-detect USM mode and set HSA_XNACK'.
No, I don't think so; that environment variable needs to be set before
the libraries are loaded or it's too late. There are other ways to
achieve the same thing, by leaving messages for the libgomp plugin to
pick up, perhaps, but it's all extra complexity for no real gain.
I think we talk about two different things:
(a) where (and when) to check/set the environment variable. I think this
part is fine. You could consider moving the generated code for
'configure_xnack' code into the existing 'init' constructor function,
but it does not really matter. (Nor does the order in which the
constructor function runs.)
(I also do not see any benefit of moving it to libgomp. The message
could then be suppressed if no device available or similar tricky, but I
do not see any real advantage of moving it.)
Longer side note: I think the message "error: HSA_XNACK=%%s is
incompatible; please unset" could be clearer. Both in terms who issues
it and that it talks about an environment variable. Maybe:
"|libgomp: fatal error: Environment variable HSA_XNACK=%s is
incompatible with GCN offloading; please unset"|
|or something like that. (I did misuse 'libgomp:' for this; I am not
sure that makes sense or is even more misleading.) – I am also not sure
GCN fits that well, given that CDNA is not GCN. But that is a general
problem. But in any case, adding "fatal", "environment variable" and ...
offloading makes surely sense, IMHO.
It's not incompatible with GCN offloading, only with the XNACK mode in
which the binary was compiled (i.e. USM on or off).
The message could be less terse, indeed. I went through a variety of
messages for this and couldn't find one that I liked. How about...
fatal error: HSA_XNACK=%s is set but this program was compiled for
HSA_XNACK=%s; please unset your environment variable.
(b) How the value is made available inside both gcc/config/gcn/gcn.cc
and in mkoffload.cc.
I was talking about (b). Namely:
omp_requires_mask is already available in gcc/config/gcn/gcn.cc and
mkoffload.cc. Thus, there is no reason to reinvent the wheel and coming
up with another means to pass the same kind of data to the very same files.
(You still might want to add another flag to it (assuming 'omp requires
unified_shared_memory' alias OMP_REQUIRES_UNIFIED_SHARED_MEMORY is
insufficient.)
OK, this is a new feature that I probably should investigate.
Thanks
Andrew
