Hi Sandra,
Sandra Loosemore wrote:
On 1/9/25 05:17, Tobias Burnus wrote:
A case where 'omp error' diagnostic should be delayed - and (here)
suppressed:
["should" in the sense it would be good not in the sense "shall".]
program_control/sources/error.1.c:15:23: error: ‘pragma omp error’
encountered: GNU compiler required.
15 | otherwise(error at(compilation) severity(fatal) \
| ^~~~~
Hmmm. Here the 6.0 spec says: […] IOW, the spec doesn't require
implementations to solve the halting problem ;-) but we could
theoretically defer the error to suppress cases where the directive
is optimized away. How about I just file a PR for this for now?
It's a problem with #pragma omp error, not metadirective, anyway, so
independent of this patch series.
Yes, putting it into an PR is good, as proposed. It is not directly
related to metadirectives, but I metadirectives are the case that makes
most sense to handle (possibly besides delimited declare variant) as it
permits special case handling.
For a block under 'if (false)' it makes more sense to have the still
diagnostic …
* * *
Here, it seems as if the 'target' + 'teams' without intervening code
check has to be updated:
program_control/sources/metadirective.1.c:16:12: error: ‘target’
construct with nested ‘teams’ construct contains directives outside
of the ‘teams’ construct […]
#pragma omp target map(to:v1,v2) map(from:v3) device(0)
#pragma omp metadirective \
when( device={arch("nvptx")}: teams loop) \
otherwise( parallel loop)
for (int i= 0; i< N; i++) v3[i] = v1[i] * v2[i];
This diagnostic is coming from omp-low.cc rather than any of the
front ends. By that time the metadirective has been gimplified into
a conditional for late resolution. I'm not sure what the right thing
to do here is; is there an actual problem with code generation if
there is some expression to test a condition between the "target" and
"teams"?
Yes. The problem is that there is no real "in between". 'target' +
'teams' works by 'target' handling all the device setup before/after –
but then it directly launches the kernel with multiple teams on the
device and no way to synchronize the teams.
Thus, it is semantically not possible to do something between starting
the target region and entering the teams. Any code that happens to be
there will either be ignored, run on the host or concurrently in
multiple teams, none of which is correct.
This case clearly shows that pushing the outer "target" into each of
the metadirective alternatives wouldn't work (because the test
expression depends on it). File another PR for now?
Yes, but that is a relatively common pattern to use 'teams' in some
cases and not in others, which is underlined by showing up both in the
example document and OpenMP_VV. Albeit using 'target teams' instead of
'teams' with an outer 'target' is an obvious alternative, reducing the
usefullness a bit. Still, I expect this showing up in real-world code.
Thus, fine for a PR but we should also try to get this fixed.
* * *
Continuing with OpenMP_VV - I think the programmer meant 'device'
and not
'target_device', but I think GCC overdoes the diagnostic, given that
there
are only two 'target_device':
tests/5.1/metadirective/test_metadirective_target_device.c:30:15:
sorry, unimplemented: ‘target_device’
tests/5.1/metadirective/test_metadirective_target_device.c:30:15:
sorry, unimplemented: ‘target_device’ selector set inside of
‘target’ directive
tests/5.1/metadirective/test_metadirective_target_device.c:30:15:
sorry, unimplemented: ‘target_device’ selector set inside of
‘target’ directive
tests/5.1/metadirective/test_metadirective_target_device.c:30:15:
sorry, unimplemented: ‘target_device’ selector set inside of
‘target’ directive
...
I wonder whether we could get rid of some of them. On the other
hand, as they
are nicely follow each other, it is not a Prio 1 issue.
This is my bad. It's coming from omp_context_selector_matches() in
omp-general.cc, where it just continues processing after the "sorry"
error instead of returning 0. Do you think it's correct for this to
be a "sorry" instead of a warning, or just silently always failing to
match? It's a trivial fix, anyway.
Good question how to handle it best. Given that, e.g.,
'omp_get_default_device()' etc. is unspecified, we might do whatever we
want …
I think having a diagnostic is sensible — but maybe we should downgrade
it to a -Wopenmp warning?
If we treat it as warning, we could consider regarding it on the device
as default_device = it's me → 'device' and on the host handle it like a
normal device. Or just say 'not matched' or …
Having multiple times this output is okayish.
Regarding the OpenMP_VV test case, I filed an issue:
https://github.com/OpenMP-Validation-and-Verification/OpenMP_VV/issues/853
as they should either use 'device' or hoist it out of the target region
and then use 'target_device' from the host side.
* * *
I'll fix this before reposting a new version of the remaining patches
in the series. The C++ code is very similar and needs the same fix.
Also, from offline discussion with Tobias:
BTW: The following seems to be valid but is rejected as the
metadirective after 'for' is not handled: "error: loop nest expected
before ‘#pragma’"
void f() {
#pragma omp metadirective when(construct={target} : parallel)
#pragma omp metadirective when(construct={target} : for)
#pragma omp metadirective when(construct={target} : simd)
for (int i =0 ; i < 1; i++)
__builtin_printf("Hello world\n");
}
Unless that's simple to fix, I suggest to just queue it for a new PR.
It's not clear to me that this is supposed to be valid; metadirective
is not included in the syntax for "canonical loop nest", nor is it a
directive with the loop-transforming property. Anyway, I think this
would require changes beyond just suppressing the error message, so
yeah, some low-priority PR would be appropriate here.
The PS solution is fine with me.
With metadirectives it is never quite clear whether they are supposed to
act like an on-the-fly text replacement that, kind of, happens before
parsing - or not. They are in a strange in-between state.
Thanks,
Tobias
