Hi Sandra, hello world,
sorry for the belated review.
Sandra Loosemore wrote:
Support for dynamic selectors in "declare variant" was developed in
parallel with support for the adjust_args/append_args clauses and the
dispatch construct; they collided in a bad way. This patch fixes the
"sorry" for calls that need both by removing the adjust_args/append_args
code from gimplify_call_expr and invoking it from the new variant
substitution code instead. It's handled as a tree -> tree transformation
rather than tree -> gimple because eventually this code may end up being
invoked from the front ends instead of the gimplifier (see PR115076).
Besides this code move with the listed changed, there is one other change:
The 'interop' clause to 'dispatch' does not affect the base function
(i.e. the case of no variant call). That's a fallout of moving the code,
but as discussed on the omp-lang mailing list, the sentiment is that
that's the correct behavior and also matches the wording of the spec.
[See also OpenMP spec issue #4495; that's about something else that came
up in the thread but it also links to the mentioned omp-lang thread.
* * *
gcc/ChangeLog
PR middle-end/118457
* gimplify.cc (modify_call_for_omp_dispatch): New, containing
code split from gimplify_call_expr and modified to emit tree
instead of gimple. Remove the error for falling through to a call
to the base function.
(expand_variant_call_expr): New, split from gimplify_variant_call_expr.
Call modify_call_for_omp_dispatch on calls to
variants in a dispatch construct context.
(gimplify_variant_call_expr): Make it call expand_variant_call_expr
to do the actual work.
(gimplify_call_expr): Remove sorry for calls involving both
dynamic/late selectors and adjust_args/append_args, and adjust
for new interface. Move adjust_args/append_args code to
modify_call_for_omp_dispatch.
(gimplify_omp_dispatch): Add some comments.
gcc/testsuite/ChangeLog
PR middle-end/118457
* c-c++-common/gomp/adjust-args-6.c: Remove xfails and adjust
expected output.
* c-c++-common/gomp/append-args-5.c: Adjust expected output.
* c-c++-common/gomp/append-args-dynamic.c: New.
* c-c++-common/gomp/dispatch-11.c: Adjust expected output.
* gfortran.dg/gomp/dispatch-11.f90: Likewise.
LGTM – but I would prefer if we could make the scan-tree-dump check of
the new test case a bit stricter, just to avoid that we accidentally
regress on this when modifying the patch.
Currently, it just checks (that it compiles plus):
/* { dg-final { scan-tree-dump "f \\(3" "gimple" } } */
/* { dg-final { scan-tree-dump "g1 \\(3" "gimple" } } */
/* { dg-final { scan-tree-dump "g2 \\(3" "gimple" } } *
I think it makes sense to check also whether the
__builtin_omp_get_mapped_ptr calls are done and the interop objects get
attached.
Or in other words: I think the dump looks fine – but we should test a
bit more. Namely, I see in the dump (with some lines removed):
D.3043 = __builtin_omp_get_default_device ();
__builtin_omp_set_default_device (5);
D.3047 = __builtin_omp_get_mapped_ptr (b, 5);
g2 (3, cp1, a, D.3047, obj1, obj2);
D.3052 = __builtin_omp_get_mapped_ptr (b, 5);
g1 (3, cp1, a, D.3052, obj1, obj2);
f (3, cp1, a, b);
__builtin_omp_set_default_device (D.3043);
Thus, I wonder whether we should at least check for 2×
'__builtin_omp_get_mapped_ptr (b, 5);' and for g1/g2 the "…, D.\[0-9\}+,
obj1, obj2\\); " and a plain 'f \\(3, cp1, a, b\\);'.
We could also check for 'if (flag.\[0-9\]+ == 2)' comparisons – to make
sure that they are in there (albeit maybe use 1, 2, 3, 4 instead of just
1, 2 to distinguish the two variants.
Plus: If we had a different set of need_device_ptr, we could check
whether those get properly handled in the different cases, i.e. g1's and
g2's call would get the D\.[....\] differently (more/less often and/or
different argument).
Thanks for your patch!
Tobias
PS: Eventually, we want to add a runtime test for this, but I guess we
should do this once GOMP_interop is available.
