Hi all,
Tobias Burnus wrote:
Tobias Burnus wrote:
Sandra Loosemore wrote:
This patch reimplements the middle-end support for "declare variant"
and extends the resolution mechanism to also handle metadirectives
(PR112779). It also adds partial support for dynamic selectors
(PR113904) and fixes a selector scoring bug reported as PR114596. I
hope
this rewrite also improves the engineering aspect of the code, e.g.
more
comments to explain what it is doing.
Do to a Clang bug and GCC issuing a warning - and me not finding
the right spot in the spec, I claimed in the C FE thread that
context={target}
is not fulfilled inside a declare-target function as it is not
inside an 'omp target' construct.
Well, I missed the following line:
OpenMP 6.0, "9.1 OpenMP Contexts" [326:28-30]:
3. For procedures that are determined to be target variants by a declare target
directive, the target trait is added to the beginning of the construct trait
set as c1 so the total size of the trait set is increased by one.
Seemingly the original patch author found that line - as GCC does the right
thing
forhttps://github.com/OpenMP/Examples/blob/main/program_control/sources/metadirective.3.c#L15
#pragma omp begin declare target
void exp_pi_diff(double *d, double my_pi){
#pragma omp metadirective \
when( construct={target}: distribute parallel for ) \
otherwise( parallel for simd )
namely, as the function is 'exp_pi_diff' the 'when' is correctly matched
and "distribute parallel for" is used.
So far so good, but when compiling it, one gets the confusing warning:
warning: direct calls to an offloadable function containing metadirectives
with a ‘construct={target}’ selector may produce unexpected results
(To the defense of the original patch author, back then the spec (5.0) talked
about "device routines" which is ambiguous whether it applies to functions
marked as declare target - or only those actually running on non-host
devices.)
* * *
As GCC does the right thing - but the warning is highly confusing in light
of the much clearer 6.0 wording, IMHO, we just can just remove the following
bits of the patch.
Namely, removing the said warning:
--- a/gcc/omp-low.cc
+++ b/gcc/omp-low.cc
@@ -14676,6 +14676,24 @@ lower_omp_1 (gimple_stmt_iterator *gsi_p, omp_context
*ctx)
tree fndecl;
call_stmt = as_a <gcall *> (stmt);
fndecl = gimple_call_fndecl (call_stmt);
+ if (fndecl
+ && lookup_attribute ("omp metadirective construct target",
+ DECL_ATTRIBUTES (fndecl)))
+ {
+ bool in_target_ctx = false;
+
+ for (omp_context *up = ctx; up; up = up->outer)
+ if (gimple_code (up->stmt) == GIMPLE_OMP_TARGET)
+ {
+ in_target_ctx = true;
+ break;
+ }
+ if (!ctx || !in_target_ctx)
+ warning_at (gimple_location (stmt), 0,
+ "direct calls to an offloadable function containing "
+ "metadirectives with a %<construct={target}%> "
+ "selector may produce unexpected results");
+ }
if (fndecl
&& fndecl_built_in_p (fndecl, BUILT_IN_NORMAL))
switch (DECL_FUNCTION_CODE (fndecl))
And as this is the only use for "omp metadirective construct target",
the following can be then removed in
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
...
+static enum gimplify_status
+gimplify_omp_metadirective (tree *expr_p, gimple_seq *pre_p, gimple_seq *,
+ bool (*) (tree), fallback_t)
+{
Namely:
+ /* Mark offloadable functions containing metadirectives that specify
+ a 'construct' selector with a 'target' constructor. */
+ if (offloading_function_p (current_function_decl))
+ {
+ for (tree variant = OMP_METADIRECTIVE_VARIANTS (*expr_p);
+ variant != NULL_TREE; variant = TREE_CHAIN (variant))
+ {
+ tree selector = OMP_METADIRECTIVE_VARIANT_SELECTOR (variant);
+
+ if (omp_get_context_selector (selector, OMP_TRAIT_SET_CONSTRUCT,
+ OMP_TRAIT_CONSTRUCT_TARGET))
+ {
+ tree id = get_identifier ("omp metadirective construct target");
+
+ DECL_ATTRIBUTES (current_function_decl)
+ = tree_cons (id, NULL_TREE,
+ DECL_ATTRIBUTES (current_function_decl));
+ break;
+ }
+ }
+ }
Just as followup to my claim that the 2/10 patch was LGTM. With those
changes, it is then LGTM v2.0. BTW: A later patch in the patch series
added a testcase similar to the OpenMP example document; it needs to be
updated for the removed warning but it will test that this works.
Tobias PS: Sorry for causing confusion, but I hope it is now sorted out
correctly.
