Sandra Loosemore wrote:
This patch fixes a number of problems with parser error checking of
"declare variant", especially in the C front end.
The new C testcase unprototyped-variant.c added by this patch used to
ICE when gimplifying the call site, at least in part because the
variant was being recorded even after it was diagnosed as invalid.
There was also a large block of dead code in the C front end that was
supposed to fix up an unprototyped declaration of a variant function
to match the base function declaration, that was never executed because
it was nested in a conditional that could never be true. I've fixed those
problems by rearranging the code and only recording the variant if it
passes the correctness checks. I also tried to add some comments and
re-work some particularly confusing bits of code, so that it's easier to
understand.
The OpenMP specification doesn't say what the behavior of "declare
variant" with the "append_args" clause should be when the base
function is unprototyped. The additional arguments are supposed to be
inserted between the last fixed argument of the base function and any
varargs, but without a prototype, for any given call we have no idea
which arguments are fixed and which are varargs, and therefore no idea
where to insert the additional arguments. This used to trigger some
other diagnostics (which one depending on whether the variant was also
unprototyped), but I thought it was better to just reject this with an
explicit "sorry".
Finally, I also observed that a missing "match" clause was only
rejected if "append_args" or "adjust_args" was present. Per the spec,
"match" has the "required" property, so if it's missing it should be
diagnosed unconditionally. The C++ and Fortran front ends had the same
issue so I fixed this one there too.
* * *
--- a/gcc/c/c-parser.cc
+++ b/gcc/c/c-parser.cc
...
+ if ((has_adjust_args || append_args_tree)
&& !omp_get_context_selector (ctx, OMP_TRAIT_SET_CONSTRUCT,
- OMP_TRAIT_CONSTRUCT_SIMD))
+ OMP_TRAIT_CONSTRUCT_DISPATCH))
{
...
+ if (!omp_get_context_selector (ctx, OMP_TRAIT_SET_CONSTRUCT,
+ OMP_TRAIT_CONSTRUCT_SIMD))
+ /* Check that the base and variant have compatible types. */
As you are adding comments, I think you also need to add one for
the 'not construct={simd}'.
That those are skipped is part of commit r10-3744-g94e7f906ca5c73,
i.e. pretty old.
I bet is has something to do with the modifications done via
c_omp_declare_simd_clauses_to_numbers but I actually do not really
see a strong relatation between 'construct={simd}' and
'declare simd', unless the compiler adds something automatically
and hopefully does some argument checking elsewhere ...
* * *
In any case, it seems as we should but don't handle
adjust_args/append_args with declare simd/construct={simd} functions.
Besides this code, also the following code in C++'s decl.cc implies
this:
if (simd)
{
TREE_VALUE (simd)
= c_omp_declare_simd_clauses_to_numbers (DECL_ARGUMENTS (decl),
OMP_TS_PROPERTIES (simd));
/* FIXME, adjusting simd args unimplemented. */
return true;
}
I think it would be sensible to add a 'sorry' and to create
a PR about this, what do you think?
* * *
+ sorry_at (append_args_loc,
+ "%<append_args%> with unprototyped base function "
+ "is not supported yet");
Compared to previous diagnostic, we loose information here:
-/* (B) No prototype for the variant nor for the base function. */
+/* (B) No prototype for the variant nor for the base function.
...
-void variant_fn3(); /* { dg-error "argument 1 of 'variant_fn3' must be of
'omp_interop_t'" } */
+void variant_fn3();
#pragma omp declare variant(variant_fn3) match(construct={dispatch})
append_args(interop(target)) \
adjust_args(need_device_ptr: x,y)
-/* { dg-note "'append_args' specified here" "" { target *-*-* } .-5 } */
+/* { dg-message "'append_args' with unprototyped base function" "" { target
*-*-* } .-5 } */
Can we get an 'inform' back? For instance: 'inform (loc, "declared here")'
after the sorry?
* * *
Side remark:
+ if (fail)
+ {
+ error_at (token->location,
+ "variant %qD and base %qD have incompatible types "
+ "after %<append_args%> adjustment",
+ variant, fndecl);
+ return;
and a bit later
+ error_at (token->location,
+ "variant %qD and base %qD have incompatible types",
+ variant, fndecl);
having in both caes an
inform (DECL_SOURCE_LOCATION (variant),
"variant function declared here")
might be helpful to a user.
(The base function should be near 'token->location').
I know that that's unchanged to the current code.
Likewise for C++ in omp_declare_variant_finalize_one
and Fortran's gfc_trans_omp_declare_variant.
* * *
--- a/gcc/fortran/openmp.cc
+++ b/gcc/fortran/openmp.cc
@@ -6982,13 +6982,9 @@ gfc_match_omp_declare_variant (void)
return MATCH_ERROR;
}
- if ((has_adjust_args || has_append_args) && !has_match)
+ if (!has_match)
{
- gfc_error ("the %qs clause at %L can only be specified if the "
- "%<dispatch%> selector of the construct selector set appears "
- "in the %<match%> clause",
- has_adjust_args ? "adjust_args" : "append_args",
- has_adjust_args ? &adjust_args_loc : &append_args_loc);
+ gfc_error ("expected %<match%> at %C");
I think it reads better as "(E|e)xpected %<match%> clause at %C" – but
one can argue about whether it should output the literal string that
was expected or the semantically missing object.
But I think we should do so as before + as in C/C++ and add 'clause'.
* * *
All in all: LGTM. But I think:
- For Fortran, the word "clause" should be added.
Plus:
- IMHO, we should have a PR about the SIMD case; can you open one?
IMHO:
- it would be nice to get an 'inform' back for the case that
now print a sorry but showed the inform before.
- Add a few 'inform' about the variant-function decl location.
→ This directly points to the variant decl, which might be
hidden in some header file.
And, if possible:
- Add a comment and a 'sorry' for the SIMD case, possibly linking
to the the PR.
Thanks for the patch!
Tobias
