Hi Sandra, hi Jakub, hello world,
pre-remark: SA Sandra's patch is an updated post of her v4 version at
https://gcc.gnu.org/pipermail/gcc-patches/2024-November/669054.html
v4 (and, hence, v5) address Jakub's review comments to v3, i.e.
https://gcc.gnu.org/pipermail/gcc-patches/2024-August/660029.html
(and some other review comments).
However, the new series is different enough that reviewing just
the changes does not make sense; especially, adding a "magic cookie"
and to how target_device dynamic selector is resolved has changed.
* * *
Sandra Loosemore wrote on December 10, 2025:
This patch adds basic support for three new tree node types that will
be used in subsequent patches to support OpenMP metadirectives and
dynamic selectors.
LGTM. Thanks!
I don't know whether Jakub wants to have a look at the design of
the patch or a bit more; hence, maybe wait a day or two before
committing it.
* * *
Regarding the 02/10 patch, it looks good so far, but I want to re-read
it tomorrow - hence, no full review.
* * *
ALL OF THE FOLLOWING RELATES TO 02/10, only:
* There is one spurious ' v5' in patch (if you want to apply it,
just remove space + 'v5' first)
* The patch does not apply cleanly (sorry!) for one hunk in
in gcc/c/c-parser.cc'sc_finish_omp_declare_variant As that only modifies
omp_context_selector_matches and the newer mainline code should moved
things a bit around, it should be trivial to fix. In gimplify.cc, having two empty lines aroundfind_supercontext
does not seem to match the majority of the code there (which uses
one line), albeit it is not completely consistent.
I noticed the following (code moved around but not new):
if (targetm.omp.device_kind_arch_isa != NULL)
r = targetm.omp.device_kind_arch_isa (omp_device_kind, prop);
else
r = strcmp (prop, "cpu") == 0;
First, this assumes that the target hook is always set for offload
devices; I wonder whether we want to replace the else by an assert
#ifdef ACCEL_COMPILER.
Additionally, both this code andix86_omp_device_kind_arch_isa accept "cpu" as context selector. That's
at odds with:
https://gcc.gnu.org/onlinedocs/libgomp/OpenMP-Context-Selectors.html
which states that the host only matches kind = "host". BTW: That
documentation could also mention that the offload devices match "nohost"
and all match "any". However, the actual question is whether the host
should match "cpu" or not. In principle, I had expected does it does –
however, what I gathered at some meetings it is supposed to apply to the
offload side, which is underlined by the wording in the
additional-definitions document: cpu A parallel device optimized for
general computation which does not quite apply to the host. On the other
hand, the following program prints 1, 1, 0 with both GCC and with
Clang-19, which implies that the host is a 'cpu'. I think following both
implementations and the naive expectation, we should keep it that way!
int f1() {return 1;} int f2() {return 1;} int f3() {return 1;} #pragma
omp declare variant(f1) match(device={kind(cpu)}) int g1() {return 0;}
#pragma omp declare variant(f2) match(device={kind(host)}) int g2()
{return 0;} #pragma omp declare variant(f3) match(device={kind(nohost)})
int g3() {return 0;} int main() { __builtin_printf ("%d\n", g1());
__builtin_printf ("%d\n", g2()); __builtin_printf ("%d\n", g3()); } =>
Can you update the documention to mention "cpu", "nohost" and, possibly
"any"? Thanks! Otherwise, I still wonder about the assert for
ACCEL_COMPILER
* * * Regarding the 'target_device_ss' code: For completeness, OpenMP 6
supports here 'uid', which would be resolved by calling
omp_get_device_from_uid or omp_get_uid_from_device and comparing (either
the device number or the uid string). Thus, for 'uid' there would be no
'omp target' call but a routine call. (The routines are in GCC 15, but
we do not need to handle it now/as part of this patch, even if we
could/may.) * * *
Thanks for the patch!
Tobias
