On 1/13/25 16:43, Tobias Burnus wrote:
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.
[snip]
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.
Thanks so much for your help with this patch series!
I'd also noted this change in wording and a later patch in the series
(OpenMP: Update "declare target"/OpenMP context interaction) adjusts the
behavior to account for the current 5.0-based implementation only adding
the "target" construct for "begin declare target" vs the current spec
which is clear it applies to both "begin declare target" and "declare
target". But I'd missed the connection to this bogus warning about
behavior that was unclear in the original spec but is well-specified now.
My regression-testing of the revised patches isn't going to finish while
I'm still awake tonight; I'm testing both the parts 1-3 that have been
approved to commit now, and the whole series that actually exercises the
new code. Assuming all goes well I'll push the 3 approved patches and
post the current version of the whole set in the morning. (I haven't
yet had time to address any other comments on the C front-end piece, but
I have tweaked some test cases for all 3 languages so the last posted
version is bit-rotten again.)
-Sandra
