[Fortran] OpenMP – libgomp/testsuite – use 'stop' and 'dg-do run'

This patch is about: libgomp/testsuite/libgomp.fortran/, only

The two test cases I added recently use 'call abort()', which is 
nowadays frowned on as that's a ventor extension. Hence, I change it to  
'stop'.

Additionally, the 'fortran.exp' in the directory states: "For Fortran 
we're doing torture testing, as Fortran has far more tests with arrays 
etc. that testing just -O0 or -O2 is insufficient, that is typically not 
the case for C/C++."

The torture testing is only done if there is a "dg-do run"; without 
dg-do, it also does run, but only with a single compile flag setting.

I have only selectively added it; I think 'dg-do run' does not make 
sense for:
* condinc*.f – only do uses '!$ include'
* omp_cond*.f* – only tests '!$' code, including comments.
Hence, I excluded those and only changed the others. (However, one can 
still argue about the remaining ones – such as 'omp_hello.f' or tabs*.f*.)

OK for the trunk? Add/remove for 'dg-do run' from additional test cases?

Tobias

On Fri, Oct 25, 2019 at 06:17:26PM +0200, Tobias Burnus wrote:
> This patch is about: libgomp/testsuite/libgomp.fortran/, only
> 
> The two test cases I added recently use 'call abort()', which is 
> nowadays frowned on as that's a ventor extension. Hence, I change it to  
> 'stop'.
> 
> Additionally, the 'fortran.exp' in the directory states: "For Fortran 
> we're doing torture testing, as Fortran has far more tests with arrays 
> etc. that testing just -O0 or -O2 is insufficient, that is typically not 
> the case for C/C++."
> 
> The torture testing is only done if there is a "dg-do run"; without 
> dg-do, it also does run, but only with a single compile flag setting.
> 
> I have only selectively added it; I think 'dg-do run' does not make 
> sense for:
> * condinc*.f – only do uses '!$ include'
> * omp_cond*.f* – only tests '!$' code, including comments.
> Hence, I excluded those and only changed the others. (However, one can 
> still argue about the remaining ones – such as 'omp_hello.f' or tabs*.f*.)
> 
> OK for the trunk? Add/remove for 'dg-do run' from additional test cases?
> 
> Tobias
> 

I think this patch and the openacc patch are fine.

With openmp and openacc, I usually defer to Jakub, Thomas,
and Cesar for their expertise with these standards.  I do,
however, recognize that everyone has limited amounts of time.
If you have openmp/openacc patches I can cast an eye over
them for formatting issues; otherwise, I'll leave it to your
discretion on what a reasonable review timeout is prior to
committing a change.

Hi Tobias!

On 2019-10-25T18:17:26+0200, Tobias Burnus <tobias@codesourcery.com> wrote:
> This patch is about: libgomp/testsuite/libgomp.fortran/, only
>
> The two test cases I added recently use 'call abort()', which is 
> nowadays frowned on as that's a ventor extension. Hence, I change it to  
> 'stop'.
>
> Additionally, the 'fortran.exp' in the directory states: "For Fortran 
> we're doing torture testing, as Fortran has far more tests with arrays 
> etc. that testing just -O0 or -O2 is insufficient, that is typically not 
> the case for C/C++."
>
> The torture testing is only done if there is a "dg-do run"; without 
> dg-do, it also does run, but only with a single compile flag setting.

(It was me who suggested that task.)


> I have only selectively added it; I think 'dg-do run' does not make 
> sense for:
> * condinc*.f – only do uses '!$ include'
> * omp_cond*.f* – only tests '!$' code, including comments.
> Hence, I excluded those and only changed the others. (However, one can 
> still argue about the remaining ones – such as 'omp_hello.f' or tabs*.f*.)

But why shouldn't these still be torture tested?

Anyway -- as usual ;-) -- I'd prefer if any such rationale ("not doing
torture testing because [...]") was put into the respective testsuite
files, so that it's obvious to the next person reading that file.


> --- a/libgomp/testsuite/libgomp.fortran/strassen.f90
> +++ b/libgomp/testsuite/libgomp.fortran/strassen.f90
> @@ -0,0 +1 @@
> +! { dg-do run }
|  ! { dg-options "-O2" }

That's not a useful combination, is it?  (Just noticed this one here;
didn't verify all files suggested to be changed.)

Not sure into which direction this should be fixed: continue to just test
'-O2', or remove the '-O2' override.


> --- a/libgomp/testsuite/libgomp.fortran/use_device_addr-1.f90
> +++ b/libgomp/testsuite/libgomp.fortran/use_device_addr-1.f90
> @@ -0,0 +1 @@
> +! { dg-do run }

> --- a/libgomp/testsuite/libgomp.fortran/use_device_addr-2.f90
> +++ b/libgomp/testsuite/libgomp.fortran/use_device_addr-2.f90
> @@ -0,0 +1 @@
> +! { dg-do run }

On powerpc64le-unknown-linux-gnu without offloading, I'm seeing (only)
the '-O0' execution tests FAIL for both these, with 'STOP 1' message.


Grüße
 Thomas

Hi!

Generally, we need to consider the aim to test things sufficiently vs. the
already way too long test time of libgomp.fortran testsuite which is not
parallelized itself and often takes the longest time in the testsuite.

The primary reason to do any option cycling in libgomp.fortran are tests
involving variable length types where it matters matters quite a lot if it
is -O0 or -O2 or -O3, e.g. with -O0 some temporaries need to be privatized
when with optimization they don't, etc., though perhaps we could trim the
list and just test
-O0
-O2
-O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions -g
and not the other ones.

On Fri, Oct 25, 2019 at 06:17:26PM +0200, Tobias Burnus wrote:
> diff --git a/libgomp/testsuite/libgomp.fortran/display-affinity-1.f90 b/libgomp/testsuite/libgomp.fortran/display-affinity-1.f90
> index 4811b6f801b..4e732e5427f 100644
> --- a/libgomp/testsuite/libgomp.fortran/display-affinity-1.f90
> +++ b/libgomp/testsuite/libgomp.fortran/display-affinity-1.f90
> @@ -0,0 +1 @@
> +! { dg-do run }

In this testcase I don't see the value to cycle through, it is a test for
library routines.

> +++ b/libgomp/testsuite/libgomp.fortran/lastprivate1.f90
> +++ b/libgomp/testsuite/libgomp.fortran/lastprivate2.f90
> +++ b/libgomp/testsuite/libgomp.fortran/nestedfn4.f90

These are ok.

> --- a/libgomp/testsuite/libgomp.fortran/omp_hello.f
> +++ b/libgomp/testsuite/libgomp.fortran/omp_hello.f
> @@ -0,0 +1 @@
> +C { dg-do run }

Not worth it.

> --- a/libgomp/testsuite/libgomp.fortran/omp_orphan.f
> +++ b/libgomp/testsuite/libgomp.fortran/omp_orphan.f
> @@ -0,0 +1 @@
> +C { dg-do run }
> --- a/libgomp/testsuite/libgomp.fortran/omp_reduction.f
> +++ b/libgomp/testsuite/libgomp.fortran/omp_reduction.f
> @@ -0,0 +1 @@
> +C { dg-do run }
> --- a/libgomp/testsuite/libgomp.fortran/omp_workshare1.f
> +++ b/libgomp/testsuite/libgomp.fortran/omp_workshare1.f
> @@ -0,0 +1 @@
> +C { dg-do run }
> --- a/libgomp/testsuite/libgomp.fortran/omp_workshare2.f
> +++ b/libgomp/testsuite/libgomp.fortran/omp_workshare2.f
> @@ -0,0 +1 @@
> +C { dg-do run }

Dunno, maybe, though not clear advantages of doing so.

> +++ b/libgomp/testsuite/libgomp.fortran/pr25219.f90
> +++ b/libgomp/testsuite/libgomp.fortran/pr28390.f
> +++ b/libgomp/testsuite/libgomp.fortran/pr35130.f90
> +++ b/libgomp/testsuite/libgomp.fortran/pr90779.f90

Ok.

> --- a/libgomp/testsuite/libgomp.fortran/strassen.f90
> +++ b/libgomp/testsuite/libgomp.fortran/strassen.f90
> @@ -0,0 +1 @@
> +! { dg-do run }

Not ok.  This is quite an expensive test and it is intentionally run just at
-O2.

> --- a/libgomp/testsuite/libgomp.fortran/target-simd.f90
> +++ b/libgomp/testsuite/libgomp.fortran/target-simd.f90
> @@ -17 +17 @@ program test
> -  if (any (b - 5.0 *a > 10.0*epsilon(a))) call abort()
> +  if (any (b - 5.0 *a > 10.0*epsilon(a))) stop 1
> @@ -25 +25 @@ program test
> -  if (any (b - 2.0 *a > 10.0*epsilon(a))) call abort()
> +  if (any (b - 2.0 *a > 10.0*epsilon(a))) stop 2

The call abort -> stop changes are ok.  I'm not really happy
about the uppercase STOP in all the libgomp.fortran tests that are
written completely in lowercase except these stops, but didn't get
around to changing it yet.

> +++ b/libgomp/testsuite/libgomp.fortran/task2.f90
> +++ b/libgomp/testsuite/libgomp.fortran/taskgroup1.f90
> +++ b/libgomp/testsuite/libgomp.fortran/taskloop1.f90
> +++ b/libgomp/testsuite/libgomp.fortran/use_device_addr-1.f90
> +++ b/libgomp/testsuite/libgomp.fortran/use_device_addr-2.f90
> +++ b/libgomp/testsuite/libgomp.fortran/workshare1.f90
> +++ b/libgomp/testsuite/libgomp.fortran/workshare2.f90

Ok.

	Jakub

Hi,

thanks for the review.

On 10/30/19 10:31 AM, Jakub Jelinek wrote:
>> +++ b/libgomp/testsuite/libgomp.fortran/omp_orphan.f
>> +++ b/libgomp/testsuite/libgomp.fortran/omp_reduction.f
>> +++ b/libgomp/testsuite/libgomp.fortran/omp_workshare1.f
>> --- a/libgomp/testsuite/libgomp.fortran/omp_workshare2.f
>> +++ b/libgomp/testsuite/libgomp.fortran/omp_workshare2.f
> Dunno, maybe, though not clear advantages of doing so.

I didn't added 'dg-do run' for those.
Otherwise as suggested – and committed as Rev. 277606.

> I'm not really happy about the uppercase STOP in all the 
> libgomp.fortran tests that are written completely in lowercase except 
> these stops, but didn't get around to changing it yet.

How about the following patch? Created by
sed -i -e 's/STOP /stop /' in testsuite/libgomp.fortran/
and glanced over. (Seemingly, no all-capital-letters file exists.)

?

Tobias

On Wed, Oct 30, 2019 at 11:45:11AM +0100, Tobias Burnus wrote:
> On 10/30/19 10:31 AM, Jakub Jelinek wrote:
> > > +++ b/libgomp/testsuite/libgomp.fortran/omp_orphan.f
> > > +++ b/libgomp/testsuite/libgomp.fortran/omp_reduction.f
> > > +++ b/libgomp/testsuite/libgomp.fortran/omp_workshare1.f
> > > --- a/libgomp/testsuite/libgomp.fortran/omp_workshare2.f
> > > +++ b/libgomp/testsuite/libgomp.fortran/omp_workshare2.f
> > Dunno, maybe, though not clear advantages of doing so.
> 
> I didn't added 'dg-do run' for those.
> Otherwise as suggested – and committed as Rev. 277606.

Thanks.

> > I'm not really happy about the uppercase STOP in all the libgomp.fortran
> > tests that are written completely in lowercase except these stops, but
> > didn't get around to changing it yet.
> 
> How about the following patch? Created by
> sed -i -e 's/STOP /stop /' in testsuite/libgomp.fortran/
> and glanced over. (Seemingly, no all-capital-letters file exists.)

> --- a/libgomp/testsuite/libgomp.fortran/lib2.f
> +++ b/libgomp/testsuite/libgomp.fortran/lib2.f
> @@ -14 +14 @@ C { dg-do run }
> -      IF (OMP_TEST_LOCK (LCK)) STOP 1
> +      IF (OMP_TEST_LOCK (LCK)) stop 1
...
> --- a/libgomp/testsuite/libgomp.fortran/lib3.f
> +++ b/libgomp/testsuite/libgomp.fortran/lib3.f
> --- a/libgomp/testsuite/libgomp.fortran/pr25162.f
> +++ b/libgomp/testsuite/libgomp.fortran/pr25162.f

Please don't change these 3 tests, they are all-capital-letters, at least on
the corresponding lines, comments and especially dg-* directives which have
to be lowercase don't count (I've looked for IF in capital letters
in your patch ;) ).

Otherwise LGTM, thanks.

	Jakub