Paul-Antoine Arras wrote:
This patch adds middle-end support for the `dispatch` construct and the
`adjust_args` clause. The heavy lifting is done in `gimplify_omp_dispatch` and
`gimplify_call_expr` respectively. For `adjust_args`, this mostly consists in
emitting a call to `gomp_get_mapped_ptr` for the adequate device.
...
* gimplify.h (omp_has_novariants): Declare.
(omp_has_nocontext): Declare.
As those two functions are only used in gimplify.cc,
please make them 'static' and remove them from gimplify.h.
* * *
I have a testcase which is rejected with the bogus:
17 | !$omp end dispatch
| 1
Error: Unclassifiable OpenMP directive at (1)
That's at least valid in OpenMP 6.0 previews as those have:
"For a dispatch directive, the paired 'end' directive is optional."
In 5.2, it is implied via "3.1 Directive Format" and that 'dispatch'
has "Association: block (function dispatch structured block)"
Note: That 'nowait' is an 'end-clause' and may also appear as
'!$omp end dispatch nowait'.
(but either at 'dispatch' or at 'end dispatch'; the current code should
be able to handle this.)
* * *
But the main reason that I created the testcase was a comment which
looked wrong in gimplify_omp_dispatch – and indeed, the attached
testcase gives an ICE:
internal compiler error: in gimplify_omp_dispatch, at gimplify.cc:18064
See attached Fortran testcase + comment below at gimplify_omp_dispatch.
* * *
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
...
@@ -4052,6 +4053,7 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p, bool
want_value)
/* Gimplify the function arguments. */
if (nargs > 0)
{
+ tree device_num = NULL_TREE;
Indentation issue: Indented by 4 instead of 6 spaces.
@@ -4062,8 +4064,111 @@ gimplify_call_expr (tree *expr_p, gimple_seq *pre_p,
bool want_value)
...
+ if (flag_openmp && EXPR_P (CALL_EXPR_FN (*expr_p))
+ && DECL_P (TREE_OPERAND (CALL_EXPR_FN (*expr_p), 0))
+ && (adjust_args_list = lookup_attribute (
+ "omp declare variant variant adjust_args",
+ DECL_ATTRIBUTES (
+ TREE_OPERAND (CALL_EXPR_FN (*expr_p), 0))))
+ != NULL_TREE)
+ {
...
+ if (gimplify_omp_ctxp != NULL
+ && gimplify_omp_ctxp->code == OMP_DISPATCH)
+ {
The OpenMP spec only supports append_args/adjust_args "when a specified
function variant is selected for replacement in the context of a
function *dispatch* structured block.
Thus, IMHO, you can merge the two if conditions.
+ for (tree c = gimplify_omp_ctxp->clauses; c;
+ c = TREE_CHAIN (c))
+ {
+ if (OMP_CLAUSE_CODE (c)
+ == OMP_CLAUSE_IS_DEVICE_PTR)
+ {
+ tree decl1 = DECL_NAME (OMP_CLAUSE_DECL (c));
+ tree decl2
+ = tree_strip_nop_conversions (*arg_p);
+ if (TREE_CODE (decl2) == ADDR_EXPR)
+ decl2 = TREE_OPERAND (decl2, 0);
+ gcc_assert (TREE_CODE (decl2) == VAR_DECL
+ || TREE_CODE (decl2)
+ == PARM_DECL);
+ decl2 = DECL_NAME (decl2);
+ if (decl1 == decl2)
+ {
+ is_device_ptr = true;
+ break;
+ }
+ }
+ else if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEVICE)
+ device_num = OMP_CLAUSE_OPERAND (c, 0);
+ }
Assume(*) you have:
#pragma omp dispatch is_device_ptr(p) device_num(6)
foo(p);
If I read the code correctly, this will use the default device as the
"break" will prevent finding the device clause.
(* Or other way round, if new clauses are internally added at the
beginning of the list.)
+ if (build_int_cst (integer_type_node, i)
+ == TREE_VALUE (arg))
I think
if (wi::eq_p (i, tree_strip_any_location_wrapper (
TREE_VALUE (arg)))
is better and avoids creating new tree values that might en up being
unused. (I am assuming that TREE_CODE(TREE_VALUE (arg)) == INTEGER_CST,
if not, some additional checks might be needed.)
(The tree_strip_any_location_wrapper call, I have taken from
integer_nonzerop (etc.) and it might not be needed.)
+ if (need_device_ptr && !is_device_ptr)
+ {
+ if (device_num == NULL_TREE)
+ {
+ // device_num = omp_get_default_device();
I'd remove the ';' to make it more looking like a comment (and not
comment-out line) and add a space before the '(' for coding style.
+ // mapped_arg = omp_get_mapped_ptr(arg, device_num);
Likewise.
* * *
+gimplify_omp_dispatch (tree *expr_p, gimple_seq *pre_p)
+{
...
+ // If the novariants and nocontext clauses are not compile-time constants,
+ // we need to generate code for all possible cases:
+ // if (novariants) // implies nocontext
+ // base()
+ // else if (nocontext)
+ // variant1()
+ // else
+ // variant2()
+ tree dispatch_body = OMP_DISPATCH_BODY (expr);
+ if (TREE_CODE (dispatch_body) == BIND_EXPR)
+ dispatch_body = BIND_EXPR_BODY (dispatch_body);
+ if (TREE_CODE (dispatch_body) == STATEMENT_LIST)
+ {
+ // Fortran FE may insert some pre-call code, for instance when an
+ // array is passed as argument. Skip to the actual call.
+ dispatch_body = expr_last (dispatch_body);
+ }
One case where Fortran adds additional code is when an array that is or
might be noncontiguous is passed as actual argument to a dummy argument
that requires a contiguous array.
If the argument is not 'intent(in)', it also needs to copy afterwards
the values back to the non-contiguous array.
And regarding pre-call code, that's not different to C for code like:
f2 (p, arr, pp()[dd()]);
Except that in Fortran also post calls can exist.
In principle, I would just search for the decl attributes, except that
'pp' and 'dd' can be - in principle - also variant procedures.
* * *
I have now created a testcase for this - see attachment.
It seems as if 'novariants(1)' applies to all variant functions but
OpenMP (quoting TR13, addmittedly, I haven't checked 5.1 nor 5.2):
"If do-not-use-variant evaluates to true, no function variant is
selected for the target-call of the dispatch region associated with the
novariants clause even if one would be selected normally."
This explicitly talks about "target-call' and that is in C/C++:
[lvalue-expression =] target-call ( [expression-list] );
and it does not talk about the expression list.
For 'nocontext', it is clear that it applies to everything as:
"If do-not-update-context evaluates to true, the construct on which the
nocontext clause appears is not added to the construct trait set of the
OpenMP context."
And just to avoid any doubt: append_args/adjust_args only apply to the
arguments of 'target-call', everything else doesn't make sense, albeit
this does not seem to be spelled out explicitly.
* * *
+
+ gimplify_adjust_omp_clauses (pre_p, bind, &OMP_DISPATCH_CLAUSES (expr),
+ OMP_DISPATCH);
My impression is that this is no longer needed since no 'task' are
generated. This code seems walk the clauses and the body and then (now)
pointless 'SHARED' clauses for all variables it finds.
Or is there is hidden use I missed? If not, I think it can be removed.
Besides, it comes a bit late otherwise as not much is done afterwards.
* * *
--- a/gcc/omp-expand.cc
+++ b/gcc/omp-expand.cc
@@ -8636,6 +8636,19 @@ expand_omp_single (struct omp_region *region)
single_succ_edge (exit_bb)->flags = EDGE_FALLTHRU;
}
+/* Expand code for an OpenMP dispatch directive... */
+
+static void
+expand_omp_dispatch (struct omp_region *region)
+{
+ basic_block entry_bb = region->entry;
+ gimple_stmt_iterator si = gsi_last_nondebug_bb (entry_bb);
+ enum gimple_code code = gimple_code (gsi_stmt (si));
+ gcc_assert (code == GIMPLE_OMP_DISPATCH);
+ gsi_remove (&si, true);
+ single_succ_edge (entry_bb)->flags = EDGE_FALLTHRU;
+}
I wonder whether this is really needed. It looks as if you could
already get rit of GIMPLE_OMP_DISPATCH inside lower_omp_1, similar
to other functions. For instance, GIMPLE_OMP_STRUCTURED_BLOCK uses:
/* We have already done error checking at this point, so these nodes
can be completely removed and replaced with their body. */
...
gsi_replace_with_seq (gsi_p, gimple_omp_body (stmt), true);
Such that by the tie of omp-expand.cc, no work is left to do.
Tobiasmodule m
contains
subroutine f1 (ar)
integer :: arr(10)
end
subroutine f0 (ar)
integer :: arr(10)
!$omp declare variant (f1) match (construct={dispatch})
end
end module
subroutine call_it(ctx, arr)
logical :: ctx
integer :: arr(:)
!$omp dispatch nocontext(ctx)
call f(arr) ! This gives an ICE in gimplify.cc
! !$omp end dispatch ! valid since 5.2
! !$omp end dispatch nowait ! likewise valid (unless there is a 'nowait' at '!$omp dispatch')
end
int g1() { return 100; }
int g2() { return 200; }
#pragma omp declare variant (g1) match (construct={dispatch},user={condition(1)})
#pragma omp declare variant (g2) match (user={condition(1)})
int g() { return 300; }
int f1(int x) { return 1 + x; }
int f2(int x) { return 2 + x; }
#pragma omp declare variant (f1) match (construct={dispatch},user={condition(1)})
#pragma omp declare variant (f2) match (user={condition(1)})
int f(int x) { return 3 + x; }
void
test ()
{
int res;
// Call f2(g2()) due to user condition(1)
res = f (g ());
__builtin_printf("%d\n", res);
if (res != 202)
__builtin_abort ();
// Call 'f1(g1())' due to construct 'dispatch' and user conditional(1)
#pragma omp dispatch
res = f (g ());
__builtin_printf("%d\n", res);
if (res != 101)
__builtin_abort ();
// Call 'f2(g2())' due to nocontext (i.e. ignore construct 'dispatch') and user conditional(1)
#pragma omp dispatch nocontext(1)
res = f (g ());
__builtin_printf("%d\n", res);
if (res != 202)
__builtin_abort ();
// Call 'f' due to 'novariants'
// Call 'g1' due to construct 'dispatch' and user conditional(1)
#pragma omp dispatch novariants(1)
res = f (g ());
__builtin_printf("%d\n", res); /* ACTUAL RESULT: 303, i.e. 'g' and not 'g1' was called */
if (res != 301)
__builtin_abort ();
// Call 'f' due to 'novariants'
// Call 'g2' due to nocontext (i.e. ignore construct 'dispatch') and user conditional(1)
#pragma omp dispatch novariants(1) nocontext(1)
res = f (g ());
__builtin_printf("%d\n", res); /* ACTUAL RESULT: 303, i.e. 'g' and not 'g2' was called */
if (res != 302)
__builtin_abort ();
}
int
main ()
{
test ();
}
