Hi Tobias! On 2019-11-25T15:02:16+0100, Tobias Burnus <tob...@codesourcery.com> wrote: > sorry for the belated reply.
Eh, no worries -- I'm way more behind on things... > On 11/11/19 10:39 AM, Thomas Schwinge wrote: >> By the way, do you know what's the status is for Fortran common blocks in >> OpenMP: supported vs. expected per the specification? > > No; however, I had a quick glance at the spec and at the test cases; > both compile-time and run-time test have some coverage, although I > didn't spot a run-time test for one 'omp target'. Thanks. > Definition (3.32.1 in F2018): "blank common" = "unnamed common block". > 'common /name/ x" (continues) define the common block named "name" by > adding 'x' to it. While "common // y" or "common y" appends 'y' to the > blank common. Thanks for the concise summary. > In OpenMP 5, common blocks appear twice – once [2.1, p.39, ll.11ff.] as > general rule in the definition of "list item" (which are inherited by > "extended list item" and "locator-list item"). [There are also some > constraints and notes regarding common blocks)]. It does not really tell > whether blank commons are permitted or not; some description is > explicitly for named-common variables, leaving blank-common ones out > (and undefined). But later sections explicitly make reference to blank > commons, hence, one can assume they are permitted unless explicitly > stated that they are not. Yes, I go by the assumption that everything contained in the base languages of OpenACC/OpenMP (so, the respective C, C++, Fortran standards), should also work in an OpenACC/OpenMP context in a sensible manner (detailed/clarified in the respective specification as necessary), and if not supported then that ought to be spelled out explicitly. (For example, see either the "catch-all" notes in OpenACC 3.0, 1.7. "References", or the more in-detail notes in specific sections.) Anything else I'd consider a bug in the respective specification, which should be reported/fixed. That said, if you think OpenMP needs to clarify whether Fortran blank common blocks are supported or not, then file an issue or directly submit a pull request against the specification on <https://github.com/OpenMP> (once we've got access). > And then very selectively for some items: > * allocate – only with default allocator. > * declare target – some restrictions and no blank commons > * depend clause – no common permitted > * threadprivate – some notes and explanation of the syntax (why?) > also only here requirement regarding common blocks with bind(c) > (why not also for declare target?) > * linear clause – no common permitted > * copyin – some notes > * copyprivate – some notes > > As target test cases were suspiciously left out, I tries '!$omp target > map(/name/)' which was rejected. I think one should add test cases for > newer features – which mostly means 'omp target' and add the missing > common-block checks. – And one has to find out why blank commons are not > permitted and for the places where they are permitted, support has to be > added. ACK. Instead of "burying" such things in long emails, I like to see GCC PRs filed, which can then be actioned on individually. > Talking about blank common blocks, the current OpenACC implementation > does not seem to like them (see below); the spec (2.7) does not mention > blank common blocks at all. – It talks about name between two slashes, > but leaves it open whether the name can also be an empty string. My assumption would thus be: yes, ought to be supported -- but I haven't thought through whether that makes sense, so... > common // x,y !blank common > !$acc parallel copyin(//) > !$acc end parallel > end > > fails with: > > 2 | !$acc parallel copyin(//) > | 1 > Error: Syntax error in OpenMP variable list at (1) ..., please test with the PGI compiler (just to get more data), and determine whether that makes sense to support in an OpenACC context (likewise for OpenMP, of course), and then (once you've got access) either file an issue, or (better) directly submit a pull request for <https://github.com/OpenACC/openacc-spec/> to clarify that. Sometimes it's as easy as replacing non-standard text ("name between two slashes") with the corresponding standard text (whatever the Fortran specification calls this). > On 2019-10-25T16:36:10+0200, Tobias Burnus<tob...@codesourcery.com> wrote: > >>> * I have now a new test case >>> libgomp/testsuite/libgomp.oacc-fortran/common-block-3.f90 which looks at >>> omplower. >> Thanks. Curious: why 'omplower' instead of 'gimple' dump? > > So far I found -fdump-tree-original, -fdump-omplower and > -fdump-optimized quite useful – I have so far not used > -fdump-tree-gimple, hence, I cannot state what's the advantage of the > latter. My rationale is that your code changes are in 'gcc/gimplify.c', so you'd test for that stuff in the 'gimple' dump (which is between 'original' and 'omplower'). > The original dump I like because it shows what the FE generates, the > omplower dump has the result after lowering including the assignments to > the omp_arr variables but it keeps a readable pragma line (avoids > guessing what the kind value was again etc.) while the optimized dump > really shows what ends up in the call (with the pro and con that it > depends on the optimization option). > > If you think it makes sense, one can switch. I think it does (but please argue if it doesn't to you), but that's not high priority, of course. >>> --- /dev/null >>> +++ b/gcc/testsuite/gfortran.dg/goacc/common-block-3.f90 >>> @@ -0,0 +1,39 @@ >>> +! { dg-options "-fopenacc -fdump-tree-omplower" } >> (For later: we usually just use 'dg-additional-options >> "-fdump-tree-omplower"'; '-fopenacc' is implied inside '*/goacc/'.) > > My impression was that it is not effective unless repeated – and I think > I even tries it. In gcc/testsuite/gfortran.dg/gomp/ all 64 test cases > with dg-options specify re-add the "-fopenmp". > > And in gcc/testsuite/gfortran.dg/goacc, 4 test cases use dg-options, 3 > specify -fopenacc and one doesn't. I wouldn't call this 'usually' Note that I said 'dg-additional-options', not 'dg-options', so please re-consider. > and I > wonder whether -fopenacc is really active for goacc/pr84963.f90. (The > file uses no directive at all!) > > Hence, I wonder whether one should add it to goacc/pr84963.f90 – see > attached patch. Good find. Please confirm that indeed this is meant to enable OpenACC processing by reading discussion in <https://gcc.gnu.org/PR84963> and <http://mid.mail-archive.com/ead44f52-ee30-6b5e-e18a-5dd49d9a2614@suse.cz>. (Likely yes, of course.) > Without the patch, I get: > > Executing on host: …/gfortran/../../gfortran -B…/gfortran/../../ > -B…/./libgfortran/ …/goacc/pr84963.f90 -fno-diagnostics-show-caret > -fno-diagnostics-show-line-numbers -fdiagnostics-color=never > -fdiagnostics-urls=never -O -O2 -S -o pr84963.s (timeout = 300) (Confirmed.) > And with the patch, I get > … -fdiagnostics-urls=never -O -fopenacc -O2 -S -o pr84963.s … > --- a/gcc/testsuite/gfortran.dg/goacc/pr84963.f90 > +++ b/gcc/testsuite/gfortran.dg/goacc/pr84963.f90 > @@ -1,5 +1,5 @@ > ! PR ipa/84963 > -! { dg-options "-O2" } > +! { dg-options "-fopenacc -O2" } I suggest to change 'dg-options "-O2"' to 'dg-additional-options "-O2"'. Please verify, and then commit that to trunk, gcc-8-branch, gcc-7-branch, referencing PR84963 in the ChangeLog update. That's a separate fix/commit from everything else discussed here. >>> + integer :: i, j >>> + real :: a(n) = 0, b(n) = 0, c, d >>> + real :: x(n) = 0, y(n), z >>> + common /BLOCK/ a, b, c, j, d >>> + common /KERNELS_BLOCK/ x, y, z >> For my understanding: the several unused variables in the common blocks >> are to make sure that they don't cause any issues, don't get mapped at >> all? > > I think that's the idea Then let's please document that in the test case sources, for that's not quite obvious. > – common-block variables which are not used > should also not get mapped (= optimization). But, obviously, they should > also not cause any issues. > > Hence, one could/should also check that they are not mapped – done in > the attached patch. Good, thanks. > --- a/gcc/testsuite/gfortran.dg/goacc/common-block-3.f90 > +++ b/gcc/testsuite/gfortran.dg/goacc/common-block-3.f90 > @@ -9,7 +9,7 @@ program main > implicit none > > integer :: i, j > - real :: a(n) = 0, b(n) = 0, c, d > + real :: a(n) = 0, b(n) = 0, c, d, e(n) > real :: x(n) = 0, y(n), z > common /BLOCK/ a, b, c, j, d > common /KERNELS_BLOCK/ x, y, z > @@ -35,5 +35,8 @@ end program main > ! { dg-final { scan-tree-dump-times "omp target oacc_kernels > .*map\\(tofrom:y \\\[len: 400\\\]\\\)" 1 "omplower" } } > ! { dg-final { scan-tree-dump-times "omp target oacc_kernels > .*map\\(force_tofrom:c \\\[len: 4\\\]\\)" 1 "omplower" } } > > -! { dg-final { scan-tree-dump-not "map\\(.*:block\\)" "omplower" } } > -! { dg-final { scan-tree-dump-not "map\\(.*:kernels_block\\)" "omplower" } } > +! { dg-final { scan-tree-dump-not "map\\(.*:block" "omplower" } } > +! { dg-final { scan-tree-dump-not "map\\(.*:kernels_block" "omplower" } } > +! { dg-final { scan-tree-dump-not "map\\(.*:d " "omplower" } } > +! { dg-final { scan-tree-dump-not "map\\(.*:e " "omplower" } } > +! { dg-final { scan-tree-dump-not "map\\(.*:z " "omplower" } } OK for trunk, with some suitable commentary added ("expecting no mapping of un-referenced blocks/variables", or something like that, before the 'scan-tree-dump-not' ones). To record the review effort, please include "Reviewed-by: Thomas Schwinge <tho...@codesourcery.com>" in the commit log, see <https://gcc.gnu.org/wiki/Reviewed-by>. >> I we were to add to 'gfortran.dg/goacc/common-block-3.f90' a test case >> for the upcoming OpenACC 'serial' construct (which basically equals the >> OpenACC 'parallel' construct), would we copy/adapt the 'parallel' 'BLOCK' >> test case, or add a new, separate common block? >> >> Or, asking the other way round: why aren't in the current test case, >> 'parallel' and 'kernels' using the same common block, and both explicitly >> 'copy' the common block vs. not do that? > > I think one could do either way – by itself, the blocks should be > independent and, hence, could re-use the same common block. Re-using the > same common block tests other things, hence, maybe you should do so – > and use different variables from both blocks. ACK, thanks. Grüße Thomas
signature.asc
Description: PGP signature
