Re: [PATCH] libgomp: Add Controls For Runtime USM
Tobias Burnus <[email protected]>
| Newsgroups | gmane.comp.gcc.patches |
|---|---|
| Message-ID | <[email protected]> |
Gio T wrote:
> This patch serves to introduce a new environment variable for OpenMP,
> 'GOMP_RUNTIME_USM', which allows the runtime to control the use of
> Unified Shared Memory. The options are 'disabled', which prevents
> the runtime from automatically applying USM (the requires directive will
> still function), 'auto', which attempts to apply USM on some AMD APUs,
> and 'enabled', which attempts to apply USM on all devices that report
> support for it. The 'requires' directive will still take precedence
> in all cases.
In particular, 'requires unified_shared_memory/self_maps' will hide
all devices that do not support host memory access for the device.
The environment value will keep such devices, but just not enable
self mapping for them. (self mapping is the OpenMP 6 term for not
actually mapping data - such that the non-host device always
accesses the host memory for (self) mapped variables.)
For systems where the memory transfer between GPU and CPU is fast,¹
this environment variable might improve the performance. This is
in particular to be expected when CPU and GPU share the same memory
controller - which AMD calls APU and the most prominent example is
the MI300A.
(¹ be it by having the same memory controller, a fast interconnect
between GPU and CPU or by migrating the memory in a fast way.)
It should also help in cases where the GPU has less memory than
expected by the code – making it possible to run such code.
* * *
> Since this happens at device intialization, there is a carveout to elide
> OpenACC interaction.
* * *
...
> * target.c (resolve_device): Update function call.
> (gomp_load_image_to_device): Add device capability evaluation.
> (gomp_init_device): Update function contract to now care about
> whether we are in an OpenMP or OpenACC context.
Can we use a 'bool' and 'true'/'false' in the call? – Especially the
latter (which also works with 'int') makes it easier to see that the
0 and 1 are boolean flags and not some integral value.
(Actually, the datatype is already 'bool'.)
* * *
> @@ -2455,6 +2516,13 @@ initialize_env (void)
> if (gomp_throttled_spin_count_var > gomp_spin_count_var)
> gomp_throttled_spin_count_var = gomp_spin_count_var;
>
> + /* If we fail, we currently default to 'disabled', both here and on
> + initialization of 'gomp_runtime_usm_var'. Eventually, it may bear
> + consideration if we wish to rather default to 'auto' - but until then,
> + we do not wish to surprise. */
> + if (!parse_rt_usm ("GOMP_RUNTIME_USM", &gomp_runtime_usm_var))
> + gomp_runtime_usm_var = GOMP_RUNTIME_USM_DISABLED;
Can you change the "If we fail" to something like "If unset" or
"If unset or we fail, ..." or something like that? Because the most
likely reason for failing is that the env var is unset - which is not
really what one would consider as 'fail'.
The consideration for 'auto' mostly depends on still copying
data for global-static 'declare target' variables ...
* * *
* * *
> --- a/libgomp/libgomp.texi
> +++ b/libgomp/libgomp.texi
I think the environment-variable name is fine, but I think we need to
avoid confusion about the following:
* Whether accessing host memory from the device is supported or not
depends on the hardware - and for some AMD GPUs whether xnack is
enabled by the runtime library. [Either an environment variable,
kernel flag, or when the binary is compiled with xnack=on
instead of xnack=off or the default 'any'.]
Thus: Accessing host memory is possible with 'disabled' in that
case. [Obviously, this feature is also required for the following.]
* Whether the runtime uses self mapping, which OpenMP since 6.0 defines as:
self map - A mapping operation for which the corresponding storage
is the same as its original storage.
Thus: With self mapping, explicit and implicit map clauses copy no
data between devices and are effectively ignored. (Likewise the
"data-motion clauses", i.e. 'to'/'from', of the 'target update'
directive.)
The problem I have wordwise with 'unified-shared memory' is that it
implies to me that all devices share the memory - in particular, that
all non-host devices can access the host memory and not only some.
As one typically only has one GPU (or with HPC: several identical ones),
I think USM is catchy and fine for the environment variable but I think
it would be useful talk also about 'device accessible host memory' and
'self mapping'.
Back to the patch:
> +@node GOMP_RUNTIME_USM
> +@section @env{GOMP_RUNTIME_USM} -- Control runtime Unified Shared Memory application
> +@cindex Environment Variable
> +@table @asis
> +@item @emph{Description}:
> +Control the behavior of the runtime in using Unified Shared Memory in
> +offloading.
I am not sure whether better, but to throw in an alternative:
Control whether for devices, which can access host memory, data mapping
actually copies data or uses self-mapping, such that the device directly
accesses the corresponding host memory.
> Setting the environment variable to @code{DISABLED} will
> +not make use of Unified Shared Memory by default. Setting it to
> +@code{AUTO} will conditionally enable unified shared memory on specific
> +integrated GPUs (APUs, in the verbiage of AMD).
Add something like "; for those GPUs, self mapping is expected to
reduce the runtime overhead in almost all cases." (¹)
Or at least I think we need to give some rational - especially as the
'specific ... GPUs' weakens the implied rational that is can be deduced
from "integrated" and "APU".
[¹ except for asynchronous code accessing the same variable for reading
on the host and device as copying permits moving the memory e.g. to L3
cache. - Otherwise, enabling it will avoid all the management overhead,
additional memory allocations and data-copying overhead.]
* * *
> Lastly, setting it to
> +@code{ENABLED} will unconditionally attempt to make use of Unified
> +Shared Memory if the device claims to support it. Note, some devices
> +will indicate support for Unified Shared Memory despite not having the
> +capabilitiy, such as the gfx902, or may lack XNACK support, and as a
> +result, stability cannot be guaranteed in this mode of operation. Of
> +additional note, a user specifying behavior with the @code{requires}
> +directive in-code will override this setting unconditionally.
Regarding the 'requires' directive:
How about "... a user specifying the @code{unified_shared_memory} or
@code{self_maps} clauses with the @code{requires} ..."
namely, stating the clauses explicitly - this helps IMHO with understanding
the sentence and also helps to find this item by doing a search.
(Recall also that there are other requirements like reverse_offload
or unified_shared_address.)
Regarding the note about gfx902 and XNACK [see also below]:
I think we should remove the explicit wording here - and just refer
to the USM sections in
https://gcc.gnu.org/onlinedocs/libgomp/Offload-Target-Specifics.html
because that generally applies for both this env var and with
the 'requires' directive.
And then add the following to
https://gcc.gnu.org/onlinedocs/libgomp/AMD-Radeon.html
* That there might be issues with APU systems that do no support
XNACK, especially related to device-side page-fault handling.
[It is expected that this fails - but should effectively only
occur with swapped-out memory, i.e. it is not nice but also not
really a show stopper.]
* That MI100 (gfx908) and gfx902 are not supported.
* * *
Regarding the latter:
- The support-USM handling explicitly disables gfx902 and gfx908
support in libgomp's GCN GOMP_OFFLOAD_get_dev_caps.
- gfx908 is is compiled with xnack=off, which does not permit
HSA_XNACK=1 - and 'requires self_maps' is rejected at compile
time.
- gfx902 is compilable - but no user will use this as by default
GCC only creates the multilibs for gfx9-generic - and that contains
GPUs with and without (working) XNACK support.
(Thus, no real need to handle it in GCC itself.)
* * *
Background:
For XNACK: It seems as if XNACK is ("only") required to handle page faults.
For non-APUs, this means all the time for USM access. - For APUs, the there
should be essentially only page faults when the memory is swapped to disk.
* gfx902 - this is one of the few APUs (integrated GPUs) that support
XNACK - and claims (with XNACK set) SVM_ACCESSIBLE_BY_DEFAULT. However,
it is an early device and similarly to MI100 there is some USM support
but it does not really work.
* MI300A - this one is an HPC APU and AMD states for this one:
"With the shared HBM on MI300A, all types of memory allocations
are pinned in the same memory space, allowing the data to be
seamlessly accessed by both the host and device components. In
this system, physical page migration is neither needed nor useful,
however, handling page-faults via XNACK is still necessary. Thus,
applications implementing the APU programming model and relying
on unified memory must be compiled on an XNACK-enabled system."
→ https://rocm.blogs.amd.com/software-tools-optimization/mi300a-programming/README.html
Namely: "handling page-faults via XNACK is still necessary"
* There are several newer consumer APUs, which claim
SVM_ACCESSIBLE_BY_DEFAULT.
I think the only one with XNACK (besides gfx902 and the never-released (?)
gfx909) is the gfx90c - which is expected to be new enough to work.
There are multiple APUs with gfx10-3-generic/gfx11-generic that
do not support XNACK.
It seems as if USM should work fine - but likely have the same
issue that requires XNACK on MI300A: For page-fault handling,
XNACK is required - and lacking it, it the program will likely
fail.
On APUs, it seems as if memory fault can only occur for memory
swapped out to disk and, possibly, memory-mapped files.
* * *
> +When Unified Shared Memory is enabled, global static variables that
> +appear in a @code{declare target} directive will not be updated
> +between device and host. That is, data copying under this model
> +for global static variables is ignored.
Side note: This is the reason for not enabling 'auto' by default,
until we do so. - In the future we may still honor those map clauses;
once we do, AUTO could become the default - and this caveat can be removed.
IMHO, this should be reworded such that it is clear that this applies
(a) only² to the environment variable use (and to the USM/self-maps
'omp requires') and
(b) it applies to both both 'auto' and 'enabled'.
[² Actually, it currently also applies to requires USM/self_maps, but
that's supposed to be fixed soon by v2 of the patch:
https://gcc.gnu.org/pipermail/gcc-patches/2026-July/723214.html ]
* * *
> +OpenACC is unaffected by the value of this environment variable,
> +however, hybrid usage of OpenMP and OpenACC offloading will result
> +in the first used framework taking precedent. As an example, if one
> +uses OpenMP offloading and then later OpenACC offloading, the
> +capabilities the device possesses will reflect the original OpenMP
> +model.
> +
> +@item @emph{See also}:
> +@ref{Offload-Target Specifics}, @ref{AMD Radeon}
> +@end table
* * *
> attribute_hidden void
> -gomp_init_device (struct gomp_device_descr *devicep)
> +gomp_init_device (struct gomp_device_descr *devicep, bool openmp_p)
> {
> int i;
> if (!devicep->init_device_func (devicep->target_id))
> @@ -3127,6 +3129,22 @@ gomp_init_device (struct gomp_device_descr *devicep)
> gomp_fatal ("device initialization failed");
> }
>
> + /* Evaluate device capabilities and determine if we want USM enabled
> + in accordance with the environment variable 'GOMP_RUNTIME_USM'.
> + If the user has explicitly requested USM, skip the check.
> + We also check to make sure we're in OpenMP - if we are in OpenACC,
> + we need to skip this. */
> + if (!(omp_requires_mask
> + & (GOMP_REQUIRES_UNIFIED_SHARED_MEMORY | GOMP_REQUIRES_SELF_MAPS))
> + && openmp_p)
> + {
> + if ((gomp_runtime_usm_var == GOMP_RUNTIME_USM_AUTO)
> + && !(devicep->capabilities & GOMP_OFFLOAD_CAP_APU_SHARED_MEM))
> + devicep->capabilities &= ~GOMP_OFFLOAD_CAP_SHARED_MEM;
> + if (gomp_runtime_usm_var == GOMP_RUNTIME_USM_DISABLED)
> + devicep->capabilities &= ~GOMP_OFFLOAD_CAP_SHARED_MEM;
> + }
> +
I have to admit that I do not understand this code. In gomp_target_init,
there is:
int dev_caps = current_device.capabilities;
if (current_device.get_dev_caps_func)
dev_caps |= current_device.get_dev_caps_func (i);
if (((omp_requires_mask & GOMP_REQUIRES_UNIFIED_ADDRESS)
&& !(dev_caps & GOMP_OFFLOAD_CAP_UNIFIED_ADDR))
|| ((omp_requires_mask & GOMP_REQUIRES_REVERSE_OFFLOAD)
&& !(dev_caps & GOMP_OFFLOAD_CAP_REV_OFFLOAD)))
continue;
if (omp_requires_mask & (GOMP_REQUIRES_UNIFIED_SHARED_MEMORY
| GOMP_REQUIRES_SELF_MAPS))
{
if (dev_caps & GOMP_OFFLOAD_CAP_SHARED_MEM)
devs[num_devs].capabilities
|= GOMP_OFFLOAD_CAP_SHARED_MEM;
else
continue;
}
I think the existing code (my bad) is not very elegant - as the properties are
queried unconditionally - while they are only used in the rare case that there
is 'omp_requires_mask' != 0.
But my issue is: the devs[num_devs].capabilities is only set to SHARED_MEM with
requires unified_shared_memory or self_maps. - The property APU_SHARED_MEM
and SHARED_MEM are available via dev_caps - but the current code doesn't
update devs[num_devs].capabilities to include those. (Which is fine without
your patch.)
On the other hand, the new gomp_init_device code seems to assume that both
flags are set - and unsets them depending on the env var handling.
Did you forget to modify gomp_target_init to replace
int dev_caps = current_device.capabilities;
if (current_device.get_dev_caps_func)
dev_caps |= current_device.get_dev_caps_func (i);
by
if (current_device.get_dev_caps_func)
current_device.capabilities
|= current_device.get_dev_caps_func (i);
If you intended this and do so in a new revision of the patch:
(a) Please add a comment (in gomp_target_init) that gomp_init_device
might modify the capabilities as that's not obvious.
(b) You need to handle !openmp_p in gomp_init_device otherwise you
will unconditionally enable shared-memory handling for OpenACC
(if supported by hardware).
Otherwise, the patch is fine.
Tobias