Re: [PATCH] Report errors on inconsistent OpenACC nested reduction clauses
Hi Thomas, On 05.11.19 15:22, Thomas Schwinge wrote: > For your convenience, I'm attaching an incremental patch, to be merged > into yours.> [...]> With that addressed, OK for trunk. Thank you. I have merged the patches and committed. > A few more comments to address separately, later on. I will look into your remaining questions. Best regards, Frederik
Re: [PATCH] Report errors on inconsistent OpenACC nested reduction clauses
Hi Frederik! On 2019-10-29T13:20:53+0100, "Harwath, Frederik" wrote: > On 24.10.19 16:31, Thomas Schwinge wrote: >> So just C/C++ testing, no Fortran at all. This is not ideal, but >> probably (hopefully) acceptable given that this is working on the middle >> end representation shared between all front ends. > > Thanks to Tobias, we now also have Fortran tests. Indeed, thanks, Tobias. I have not reviewed these in great detail, but they certainly do look plausible. >>> --- a/gcc/testsuite/c-c++-common/goacc/reduction-6.c >>> +++ b/gcc/testsuite/c-c++-common/goacc/reduction-6.c >>> @@ -16,17 +16,6 @@ int foo (int N) >>> } >>> } >>> >>> - #pragma acc parallel >>> - { >>> -#pragma acc loop reduction(+:b) >>> -for (int i = 0; i < N; i++) >>> - { >>> -#pragma acc loop >>> - for (int j = 0; j < N; j++) >>> - b += 1; >>> - } >>> - } >>> - >>> #pragma acc parallel >>> { >>> #pragma acc loop reduction(+:c) >> >> That one stays in, but gets a 'dg-warning'. > > What warning would you expect to see here? I do not get any warnings. What I meant was that you should re-instantiate the code removed here, and then add the expected 'dg-warning'. ..., but upon having a look myself, I notice that there actually is no "nested loop in reduction needs reduction clause" diagnostic printed here, huh. Should there be? (OK to address separately, later on.) Similar for the libgomp execution test cases: undo the 'reduction' clause additions, and instead add the expected 'dg-warning's (here, they're really necessary), for the reason I had given at the end of my email. Sorry if that was unclear. For the same reason, please also leave out Tobias' translated 'libgomp.oacc-fortran/par-loop-comb-reduction-1.f90' -- we shall later consider that one, separately. For your convenience, I'm attaching an incremental patch, to be merged into yours. > From 22f45d4c2c11febce171272f9289c487aed4f9d7 Mon Sep 17 00:00:00 2001 > From: Frederik Harwath > Date: Tue, 29 Oct 2019 12:39:23 +0100 > Subject: [PATCH] Warn about inconsistent OpenACC nested reduction clauses > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > OpenACC (cf. OpenACC 2.7, section 2.9.11. "reduction clause"; > this was first clarified by OpenACC 2.6) requires that, if a > variable is used in reduction clauses on two nested loops, then > there must be reduction clauses for that variable on all loops > that are nested in between the two loops and all these reduction > clauses must use the same operator. > This commit introduces a check for that property which reports > warnings if it is violated. > > In gcc/testsuite/c-c++-common/goacc/reduction-6.c, we remove the erroneous > reductions on variable b; adding a reduction clause to make it compile > cleanly > would make it a duplicate of the test for variable c. The latter paragraph then is not needed anymore. > 2019-10-29 Gergö Barany > Tobias Burnus > Frederik Harwath > Thomas Schwinge > >gcc/ >* omp-low.c (struct omp_context): New fields >local_reduction_clauses, outer_reduction_clauses. >(new_omp_context): Initialize these. >(scan_sharing_clauses): Record reduction clauses on OpenACC constructs. >(scan_omp_for): Check reduction clauses for incorrect nesting. >gcc/testsuite/ >* c-c++-common/goacc/nested-reductions-warn.c: New test. >* c-c++-common/goacc/nested-reductions.c: New test. >* c-c++-common/goacc/reduction-6.c: Adjust. >* gfortran.dg/goacc/nested-reductions-warn.f90: New test. >* gfortran.dg/goacc/nested-reductions.f90: New test. >libgomp/ >* testsuite/libgomp.oacc-c-c++-common/par-loop-comb-reduction-1.c: >Add missing reduction clauses. >* testsuite/libgomp.oacc-c-c++-common/par-loop-comb-reduction-2.c: >Likewise. >* testsuite/libgomp.oacc-c-c++-common/par-loop-comb-reduction-3.c: >Likewise. >* testsuite/libgomp.oacc-c-c++-common/par-loop-comb-reduction-4.c: >Likewise. >* testsuite/libgomp.oacc-fortran/par-loop-comb-reduction-1.f90: >New test. The ChangeLog updates still have to be adjusted per my incremental patch. With that addressed, OK for trunk. A few more comments to address separately, later on. I noticed in the 'libgomp.log' that we currently print: [...]/libgomp.oacc-c-c++-common/par-loop-comb-reduction-1.c: In function 'main': [...]/libgomp.oacc-c-c++-common/par-loop-comb-reduction-1.c:18:13: warning: nested loop in reduction needs reduction clause for 'res' [...]/libgomp.oacc-c-c++-common/par-loop-comb-reduction-1.c:18:13: warning: nested loop in reduction needs reduction clause for 'res' Duplicate diagnostic, due to the the two nested inner loops. (I'm just noting that, not
Re: [PATCH] Report errors on inconsistent OpenACC nested reduction clauses
On 24.10.19 16:31, Thomas Schwinge wrote: Hi, I have attached a revised patch. [...] I was wondering if the way in which the patch avoids issuing errors about operator switches more than once by modifying the clauses (cf. the corresponding comment in omp-low.c) could lead to problems [...] "Patching up" erroneous state or even completely removing OMP clauses is -- as far as I understand -- acceptable to avoid "issuing errors about operator switches more than once". This doesn't affect code generation, because no code will be generated at all. (Does that answer your question?) Yes, thank you. Regarding my suggestions to "demote error to warning diagnostics", I'd suggest that at this point we do *not* try to fix for the user any presumed wrong/missing 'reduction' clauses (difficult/impossible to do correctly in the general case), but really only diagnose them. Ok, I have changed the errors into warnings and I have removed the code for avoiding repeated messages. So just C/C++ testing, no Fortran at all. This is not ideal, but probably (hopefully) acceptable given that this is working on the middle end representation shared between all front ends. Thanks to Tobias, we now also have Fortran tests. To match the order in 'struct omp_context' (see above), move these new initializations before those of 'ctx->depth'. (Even if that also just achieves "some local consistency".) ;-) Done. @@ -1131,6 +1141,9 @@ scan_sharing_clauses (tree clauses, omp_context *ctx) case OMP_CLAUSE_REDUCTION: case OMP_CLAUSE_IN_REDUCTION: + if (is_oacc_parallel (ctx) || is_oacc_kernels (ctx)) + ctx->local_reduction_clauses + = tree_cons (NULL, c, ctx->local_reduction_clauses); decl = OMP_CLAUSE_DECL (c); if (TREE_CODE (decl) == MEM_REF) { I think this should really only apply to 'OMP_CLAUSE_REDUCTION' but not > 'OMP_CLAUSE_IN_REDUCTION' (please verify)? Right, I have moved the new code to the OMP_CLAUSE_REDUCTION case above. I'm usually the last one to complain about such things ;-) -- but here really the indentation of the new code seems to be off? Please verify. Maybe you had set a tab-stop to four spaces instead of eight? Oh, it should look better now. --- /dev/null +++ b/gcc/testsuite/c-c++-common/goacc/nested-reductions-fail.c Rename to '*-warn.c', and instead of 'dg-error' use 'dg-warning' (possibly more than currently). Ok. --- a/gcc/testsuite/c-c++-common/goacc/reduction-6.c +++ b/gcc/testsuite/c-c++-common/goacc/reduction-6.c @@ -16,17 +16,6 @@ int foo (int N) } } - #pragma acc parallel - { -#pragma acc loop reduction(+:b) -for (int i = 0; i < N; i++) - { -#pragma acc loop - for (int j = 0; j < N; j++) - b += 1; - } - } - #pragma acc parallel { #pragma acc loop reduction(+:c) That one stays in, but gets a 'dg-warning'. What warning would you expect to see here? I do not get any warnings. Best regards, Frederik >From 22f45d4c2c11febce171272f9289c487aed4f9d7 Mon Sep 17 00:00:00 2001 From: Frederik Harwath Date: Tue, 29 Oct 2019 12:39:23 +0100 Subject: [PATCH] Warn about inconsistent OpenACC nested reduction clauses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenACC (cf. OpenACC 2.7, section 2.9.11. "reduction clause"; this was first clarified by OpenACC 2.6) requires that, if a variable is used in reduction clauses on two nested loops, then there must be reduction clauses for that variable on all loops that are nested in between the two loops and all these reduction clauses must use the same operator. This commit introduces a check for that property which reports warnings if it is violated. In gcc/testsuite/c-c++-common/goacc/reduction-6.c, we remove the erroneous reductions on variable b; adding a reduction clause to make it compile cleanly would make it a duplicate of the test for variable c. 2019-10-29 Gergö Barany Tobias Burnus Frederik Harwath Thomas Schwinge gcc/ * omp-low.c (struct omp_context): New fields local_reduction_clauses, outer_reduction_clauses. (new_omp_context): Initialize these. (scan_sharing_clauses): Record reduction clauses on OpenACC constructs. (scan_omp_for): Check reduction clauses for incorrect nesting. gcc/testsuite/ * c-c++-common/goacc/nested-reductions-warn.c: New test. * c-c++-common/goacc/nested-reductions.c: New test. * c-c++-common/goacc/reduction-6.c: Adjust. * gfortran.dg/goacc/nested-reductions-warn.f90: New test. * gfortran.dg/goacc/nested-reductions.f90: New test. libgomp/ * testsuite/libgomp.oacc-c-c++-common/par-loop-comb-reduction-1.c: Add missing reduction clauses. * testsuite/libgomp.oacc-c-c++-common/par-loop-comb-reduction-2.c: Likewise. * testsuite/libgomp.oacc-c-c++-common/par-loop-comb-reduction-3.c:
Re: [PATCH] Report errors on inconsistent OpenACC nested reduction clauses
Hi Frederik and Jakub! On 2019-10-21T09:08:28+0200, "Harwath, Frederik" wrote: > OpenACC requires that, if a variable is used in reduction clauses on two > nested loops, then there > must be reduction clauses for that variable on all loops that are nested in > between the two loops > and all these reduction clauses must use the same operator; this has been > first clarified by > OpenACC 2.6. This commit introduces a check for that property which reports > errors if the property > is violated. So I previously (internally, 2018-11-29) noted: | I wonder if these should really be diagnosed as hard errors, or rather as | warnings? | | The specification describes what the user is expected to do, and the | compiler should assist to achieve that goal, but I wonder if there might | be any reasonable cases where a compiler error diagnostic might be | considered "too strong" here? (Just a quick thought. On the other hand, | of course, "fail loudly for stupid things" is desiable. Will have to | think about that further.) In line with the discussion in <http://mid.mail-archive.com/20190529145245.GU19695@tucnak>, I would now suggest that indeed we here demote error to warning diagnostics: there isn't a problem for the compiler to generate code in presence of non-sensical/missing 'reduction' clauses, so no reason for a hard error. Does that make sense to you, too? Obviously, then also adjust all mentions in the commit log etc. from "error" to "warning". > I have tested the patch by comparing "make check" results and I am not aware > of any regressions. (For avoidance of doubt, I have not yet tested the patch.) > Gergö has implemented the check and it works, but I was wondering if the way > in which the patch > avoids issuing errors about operator switches more than once by modifying the > clauses (cf. the > corresponding comment in omp-low.c) could lead to problems - the processing > might still continue > after the error on the modified tree, right? Yes, processing continues (in order to report more than just the first error), but per my understanding a single 'error_at' call makes sure that compilation will termiate at some later point, with an error exit code. "Patching up" erroneous state or even completely removing OMP clauses is -- as far as I understand -- acceptable to avoid "issuing errors about operator switches more than once". This doesn't affect code generation, because no code will be generated at all. (Does that answer your question?) Regarding my suggestions to "demote error to warning diagnostics", I'd suggest that at this point we do *not* try to fix for the user any presumed wrong/missing 'reduction' clauses (difficult/impossible to do correctly in the general case), but really only diagnose them. Thus, no more "modifying the clauses"; that code should disappear. This may result in more warning diagnostics being emitted, but that seems reasonable, given that the user code is presumed buggy. (So, unless it's straight-forward, please don't spend much time on trying to minimize the number of warning diagnostics emitted.) > I was also wondering about the best place for such > checks. Should this be a part of "pass_lower_omp" (as in the patch) (..., and which, for example, is also where 'check_omp_nesting_restrictions' is called from 'scan_omp', running as part of 'pass_lower_omp'...) > or should it run earlier > like, for instance, "pass_diagnose_omp_blocks". (..., running as one of the first middle end passes, before 'pass_lower_omp'.) Jakub, do you have an opinion on that? (Full-quote of the patch is below, for your easy review.) I think the issue is balancing whether to have it in its own pass (for clean separation, which generally certainly is preferable) vs. embedded into existing code paths that already walk over all the GIMPLE statements (to avoid introducing more compile-time processing overhead). > Can the patch be included in trunk? Normally I might say "OK to commit with the following requests addressed", but as you're still new, it's maybe a good idea that you post another revision (as a reply to this email, simply). A few additional comments/requests: > From 99796969c1bf91048c6383dfb1b8576bdd9efd7d Mon Sep 17 00:00:00 2001 > From: Frederik Harwath > Date: Mon, 21 Oct 2019 08:27:58 +0200 > Subject: [PATCH] Report errors on inconsistent OpenACC nested reduction > clauses > MIME-Version: 1.0 > Content-Type: text/plain; charset=UTF-8 > Content-Transfer-Encoding: 8bit > > OpenACC (cf. OpenACC 2.7, section 2.9.11. "reduction clause"; > this was first clarified by OpenACC 2.6) requires that, if a > variable is used in reduction clauses on two nested loops, then > there must be reducti
[PATCH] Report errors on inconsistent OpenACC nested reduction, clauses
Hi, OpenACC requires that, if a variable is used in reduction clauses on two nested loops, then there must be reduction clauses for that variable on all loops that are nested in between the two loops and all these reduction clauses must use the same operator; this has been first clarified by OpenACC 2.6. This commit introduces a check for that property which reports errors if the property is violated. I have tested the patch by comparing "make check" results and I am not aware of any regressions. Gergö has implemented the check and it works, but I was wondering if the way in which the patch avoids issuing errors about operator switches more than once by modifying the clauses (cf. the corresponding comment in omp-low.c) could lead to problems - the processing might still continue after the error on the modified tree, right? I was also wondering about the best place for such checks. Should this be a part of "pass_lower_omp" (as in the patch) or should it run earlier like, for instance, "pass_diagnose_omp_blocks". Can the patch be included in trunk? Frederik >From 99796969c1bf91048c6383dfb1b8576bdd9efd7d Mon Sep 17 00:00:00 2001 From: Frederik Harwath Date: Mon, 21 Oct 2019 08:27:58 +0200 Subject: [PATCH] Report errors on inconsistent OpenACC nested reduction clauses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OpenACC (cf. OpenACC 2.7, section 2.9.11. "reduction clause"; this was first clarified by OpenACC 2.6) requires that, if a variable is used in reduction clauses on two nested loops, then there must be reduction clauses for that variable on all loops that are nested in between the two loops and all these reduction clauses must use the same operator. This commit introduces a check for that property which reports errors if it is violated. In gcc/testsuite/c-c++-common/goacc/reduction-6.c, we remove the erroneous reductions on variable b; adding a reduction clause to make it compile cleanly would make it a duplicate of the test for variable c. 2010-10-21 Gergö Barany Frederik Harwath gcc/ * omp-low.c (struct omp_context): New fields local_reduction_clauses, outer_reduction_clauses. (new_omp_context): Initialize these. (scan_sharing_clauses): Record reduction clauses on OpenACC constructs. (scan_omp_for): Check reduction clauses for incorrect nesting. gcc/testsuite/ * c-c++-common/goacc/nested-reductions-fail.c: New test. * c-c++-common/goacc/nested-reductions.c: New test. * c-c++-common/goacc/reduction-6.c: Adjust. libgomp/ * testsuite/libgomp.oacc-c-c++-common/par-loop-comb-reduction-1.c: Add missing reduction clauses. * testsuite/libgomp.oacc-c-c++-common/par-loop-comb-reduction-2.c: Likewise. * testsuite/libgomp.oacc-c-c++-common/par-loop-comb-reduction-3.c: Likewise. * testsuite/libgomp.oacc-c-c++-common/par-loop-comb-reduction-4.c: Likewise. --- gcc/omp-low.c | 107 +++- .../goacc/nested-reductions-fail.c| 492 ++ .../c-c++-common/goacc/nested-reductions.c| 420 +++ .../c-c++-common/goacc/reduction-6.c | 11 - .../par-loop-comb-reduction-1.c | 2 +- .../par-loop-comb-reduction-2.c | 2 +- .../par-loop-comb-reduction-3.c | 2 +- .../par-loop-comb-reduction-4.c | 2 +- 8 files changed, 1022 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/c-c++-common/goacc/nested-reductions-fail.c create mode 100644 gcc/testsuite/c-c++-common/goacc/nested-reductions.c diff --git a/gcc/omp-low.c b/gcc/omp-low.c index 279b6ef893a..a2212274685 100644 --- a/gcc/omp-low.c +++ b/gcc/omp-low.c @@ -127,6 +127,12 @@ struct omp_context corresponding tracking loop iteration variables. */ hash_map *lastprivate_conditional_map; + /* A tree_list of the reduction clauses in this context. */ + tree local_reduction_clauses; + + /* A tree_list of the reduction clauses in outer contexts. */ + tree outer_reduction_clauses; + /* Nesting depth of this context. Used to beautify error messages re invalid gotos. The outermost ctx is depth 1, with depth 0 being reserved for the main body of the function. */ @@ -902,6 +908,8 @@ new_omp_context (gimple *stmt, omp_context *outer_ctx) ctx->cb = outer_ctx->cb; ctx->cb.block = NULL; ctx->depth = outer_ctx->depth + 1; + ctx->local_reduction_clauses = NULL; + ctx->outer_reduction_clauses = ctx->outer_reduction_clauses; } else { @@ -917,6 +925,8 @@ new_omp_context (gimple *stmt, omp_context *outer_ctx) ctx->cb.adjust_array_error_bounds = true; ctx->cb.dont_remap_vla_if_no_change = true; ctx->depth = 1; + ctx->local_reduction_clauses = NULL; + ctx->outer_reduction_clauses = NULL;