On 7/25/24 08:00, Tobias Burnus wrote:
Hi Sandra,
thanks for your patch. (Disclaimer: I have not finished reading through
your patch.)
Some upfront generic remarks:
[* When first compiling it (incremental build), I did run into the issue
that OMP_METADIRECTIVE_CHECK wasn't declared. Thus, there seems to be a
dependency issue causing that tree-check.h might generated after code
that includes tree.h is processed. (Unrelated to your patch itself, but
for completeness …)]
I've never run into this. Are you saying some .cc file is missing a
makefile dependency on tree-check.h? Which one? Or is it tree-check.h
that is missing a dependency on something else and failing to get
regenerated? Or both?
* Not required right now, but eventually we need to check whether
https://gcc.gnu.org/PR112779 is fully fixed by this patch set or whether
follow-up work is required (and if so which). There is also PR107067 for
a Fortran ICE.
* There are some not-implemented/FIXME comments in the patches for
missing features. I think we should ensure that those won't get
forgotten, e.g. by filing PRs for those. – For declare variant, some PRs
might already exist.
Can you eventually take care of the last two items?
Yes.
(For the last item: e.g. 'target_device' for declare_variant, for which
'sorry' already existed.)
* * *
I might have asked the following question before – and you might have
answered it already:
Sandra Loosemore wrote:
This patch adds the OMP_METADIRECTIVE tree node and shared tree-level
support for manipulating metadirectives. It defines/exposes
interfaces that will be used in subsequent patches that add front-end
and middle-end support, but nothing generates these nodes yet.
I have to admit that I do not understand the part:
+ else if (set == OMP_TRAIT_SET_TARGET_DEVICE)
+/* The target_device set is dynamic, so treat it as always
+ resolvable. */
+continue;
+
The current code has 3 states:
* 0 - if a trait is false; this directly returns as it cannot be fixed
later
* 1 - if the all traits are known to match (initial value)
* -1 - if one trait cannot be evaluated, either because it is too early
(e.g. during parsing) or because it is a dynamic context selector.
Your last assertion is wrong. The comments on the top of
omp_context_selector_matches explicitly *say* "Dynamic properties (which
are evaluated at run-time) should always return 1." This is because
dynamic selectors are *always* candidates, which are then scored and
sorted according to the rules in the spec for the metadirective and
declare variant constructs.
Maybe the problem is the name of the function.... Would it help if I
renamed it from "omp_context_selector_matches" to
"omp_context_selector_is_candidate"?
@@ -1804,6 +1834,12 @@ omp_context_selector_matches (tree ctx)
----<cut>----
case OMP_TRAIT_USER_CONDITION:
if (set == OMP_TRAIT_SET_USER)
for (tree p = OMP_TS_PROPERTIES (ts); p; p = TREE_CHAIN (p))
if (OMP_TP_NAME (p) == NULL_TREE)
{
+ /* OpenMP 5.1 allows non-constant conditions for
+ metadirectives. */
+ if (metadirective_p
+ && !tree_fits_shwi_p (OMP_TP_VALUE (p)))
+ break;
+
if (integer_zerop (OMP_TP_VALUE (p)))
return 0;
if (integer_nonzerop (OMP_TP_VALUE (p)))
break;
ret = -1;
}
----</cut>----
* Comment wording: Please change to imply >= 5.1 not == 5.0 * Comment: I
don't see why the non-const only applies to metadirectives; the OpenMP
>= 5.1 seems to imply that it is also valid for declare variant. Thus,
I would change the wording.
The first 7 patches in the posted set implement support for dynamic
selectors in metadirectives. Dynamic selector support for declare
variant comes in the later patches, which further modify this code.
If you'd find it easier to review, I can smash everything together into
one gigantic patch (or some smaller number of patches), but I fear it
would make everything harder to review given the amount of removed or
moved around. Alternatively, trying to split it into more but smaller
incremental patches is problematical due to inter-dependencies and,
again, multiple patches touching the same bits of code, adding and
removing temporary stubs, etc.
* The current code seems to already handle
non-const values as expected. ... except that it changes "res" to -1,
while the idea seems to be not to modify 'ret' in this case for
metadirectives. (Why? Same question as above).
This gets back to same point as earlier, dynamic selectors are always
candidates. According to the spec, the "user" selector is dynamic if
the "condition" is not a constant, so the function should return 1. The
original code, without dynamic selector support, returned -1 because it
actually required a constant expression but didn't have one yet -- e.g.
I recall an issue with the Fortran front end not substituting the value
of symbolic constants or doing any constant-folding on them. So it was
trying simply to defer a decision about that selector until later in
compilation.
-Sandra
