On 5/31/24 06:22, Tobias Burnus wrote:
I have to admit that I don't really see the use of metadirective_p as …
int
-omp_context_selector_matches (tree ctx)
+omp_context_selector_matches (tree ctx, bool metadirective_p, bool
delay_p)
...
+ if (metadirective_p && delay_p)
+ return -1;
I do see why the resolution of KIND/ARCH/ISA should be delayed – for
both variant/metadirective as long as the code is run by the host and
the device. Except that we could exclude, e.g., 'kind(FPGA)' early on as
we don't support it at all.
But once the device code is split off, I don't see why we can't expand
the DEVICE clause right away for both variant and metadirective – while
for 'target_device', we cannot do much until runtime – except of
excluding things like 'kind(fpga)' – or excluding all 'arch' known not
to be supported neither by the host nor by any enabled offload devices.
Thus, I see why there is a 'delay_p', but not why there is a
'metadirective_p'.
But I might have missed something important ...
Yeah, omp_context_selector_matches() is pretty substantially revised in
part 9 of the posted patch set -- among other things, to remove the
metadirective_p parameter. The current split between patches isn't
ideal but this is such a huge patch set already (with more pieces in the
works to support "begin declare variant") that refactoring them would be
a lot of work and probably result in something even more challenging to
review. :-S
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;
}
(BTW: I am happy to be enlightened as I likely have miss some fine print.)
Regarding the comment: True, but shouldn't this be handled before by
issuing an error when such a clause is used in 'declare variant', i.e.
only occur when metadirective_p is/can be true?
The error diagnostic is handled during parsing in the respective front
ends (parts 4, 5, and 7 of the series).
Besides, I have to admit that I do not understand the new code. The
current code has: constant zero → whole selector known to be false
("return 0"); nonzero constant → keep current state, i.e. either 'true'
(1) or don't known ('-1') and continue; otherwise (not const) → set to
"don't know" (-1) and continue with the next item.
That seems to make also sense for metadirectives. But your patch changes
this to keep current state if a variable. In that case, '1' is used if
this is the only item or the previous condition is true. Or "-1" when
the previous item is "don't know" (-1). - I think that doesn't make
sense and it should always return -1 for a run time value.
-1 doesn't really mean "don't know". It means "don't know YET". For
the purposes of omp_context_selector_matches, a dynamic selector always
matches in the sense that they all need to be included in the list of
replacement candidates.
Additionally, I wonder why you use tree_fits_shwi_p instead of a simple
'TREE_CODE (OMP_TP_VALUE (p)) != INTEGER_CST'. It does not seem to
matter here, but '(uint128_t)-1' looks like a valid condition and valid
constant, which integer_nonzerop should handled but if the hwi is 128bit
wide, it won't fit into a signed variable.
(As integer_nonzerop and the current code both do "break;" it won't
change the result of the current code.)
The existing code for parsing "declare variant" context selectors
already uses tree_fits_shwi_p. (See e.g. c_parser_omp_context_selector
in gcc/c/c-parser.cc.) If there's a better idiom for checking for a
constant I'll certainly use it, but I was trying to be consistent with
what I thought was standard practice already. :-S
* * *
+static tree
+omp_dynamic_cond (tree ctx)
+{
...
+ /* The user condition is not dynamic if it is constant. */
+ if (!tree_fits_shwi_p (TREE_VALUE (expr_list)))
Any reason for using tree_fits_shwi_p instead of INTEGER_CST? Here,
(uint128_t)-1 could make a difference …
Same here.
+ /* omp_initial_device is -1, omp_invalid_device is -4; choose
+ a value that isn't otherwise defined to indicate the default
+ device. */
+ device_num = build_int_cst (integer_type_node, -2);
Don't do this - we do it differently for 'target' and it should do the
same. Some value usage history:
Wait, in your January review comments on an earlier version of this
patch you told me I *should* do this -- use a negative value that is
neither omp_initial_device nor omp_invalid_device.
For target / target data etc, GCC historically used -1 to denote for
that no device clause was specified (→ default device) and >= 0 for a
user-specified device, i.e. "GOMP_target_ext(device_num,..." gets a "-1"
if nothing was specified (= default device).
OpenMP 5.2 then introduced omp_{initial,invalid}_device with
omp_initial_device == -1 and the other one < -1 but implementation defined.
Thus, we have 0...num_devices + -4 (omp_invalid_device) and "-1" for
both default device (GOMP_target_ext etc.) and as host/initial device
(API routines omp_...).
Solution was: For the GOMP_... functions, keep using -1 = default device
and use -2 for omp_initial_device. (And for the API routines, -1 ==
initial device).
If you look at the device number handling in libgomp, functions which
can be called in both context have a "remap" boolean to handle the two
usages for -1.
I strongly suggest to use the same semantic here to avoid confusion,
i.e. -1 = nothing specified, use default device. And map a
user-specified omp_initial_device (-1) to a -2.
And it would help to use GOMP_DEVICE_HOST_FALLBACK (-2, host/initial
device) and GOMP_DEVICE_ICV (-1, default device) where appropriate as
they are a tiny bit more readable and more greppable.
For the conversion, have a look at omp-expand.cc's expand_omp_target and
search for both OMP_CLAUSE_DEVICE (2nd hit) and need_device_adjustment;
if the latter is true: device = (cond ? device :
GOMP_DEVICE_HOST_FALLBACK) [where cond is 'd == -1' or rather
'(unsigned) d > (unsigned)-2']
It might make sense to move (part of) this code out of expand_omp_target
and share it by both; at least the gimple code is rather complex. And I
guess your code also runs in later phase of the compiler and, hence,
also cannot use simple code.
I'm totally lost here. Why is this necessary or a good idea? The
device number gets passed directly to a generated call to the runtime
function GOMP_evaluate_target_device, which specifically tests for -2 as
a magic number (part 3 of the patch series). It doesn't have anything
to do with the omp target construct expansion. If -2, specifically, was
a bad choice, I can change it to a different magic number that is used
only for this purpose and nothing else.
* * *
@@ -4019,6 +4019,40 @@ dump_generic_node (pretty_printer *pp, tree
node, int spc, dump_flags_t flags,
...
+ if (selector == NULL_TREE)
+ pp_string (pp, "default:");
I wonder whether we should be forward looking (OpenMP >= 5.2) and dump
'otherwise:'. (OpenMP <= 5.1 + [deprecated] 5.2 have 'default'.)
I can fix this one easily enough now, but I was envisioning some future
task to clean up all the deprecated syntaxes. Among other things, GCC
still implements only the OpenMP 5.0 syntax for requires selectors
(where the clauses of the requires directive are recognized as traits by
themselves in the "implementation" set) instead of the 5.1+ syntax (the
requires clauses are properties of a "requires" trait in the
"implementation" set). In addition to the backlog of
unreviewed/uncommitted patches relating to variant directives, there's
also a backlog of incomplete or broken features, and doing something
about different versions of the syntax is one of them.
-Sandra
