diff mbox

[hsa,testsuite] Suppress hsa warnings in libgomp tests

Message ID 20160301183918.GA2148@virgil.suse.cz
State New
Headers show

Commit Message

Martin Jambor March 1, 2016, 6:39 p.m. UTC 
Hi,

On Fri, Feb 26, 2016 at 05:07:56PM +0100, Jakub Jelinek wrote:
> On Fri, Feb 26, 2016 at 04:59:57PM +0100, Martin Jambor wrote:
> > just like with the compiler gomp testsuite, we need to add -Wno-hsa to
> > options when compiling libgomp testcases in order not to have "excess
> > errors" failures when HSA is enabled.

...
> 
> I don't like this very much.
> Couldn't you instead add -Wno-hsa next to -fopenmp in *.exp, and just where
> you want to explicitly check the hsa warnings, enable it manually in
> dg-options or dg-additional-options (it would need to be guarded with hsa
> being enabled etc. anyway).
> 

as Jakub requested, this patch deals with HSA "excess errors" in the
libgomp library testsuite by passing -Wno-hsa to all of them.  IIUC,
that passing it in the second parameter of dg-runtest (as opposed to
the third) means that it will apply even tests that have their own
dg-options, which is presumably easier for everyone, provided that hsa
will get is own libgomp testsuite directories.

OK for trunk?

Thanks,

Martin

2016-02-29  Martin Jambor  <mjambor@suse.cz>

	* testsuite/libgomp.c/c.exp: Pass -Wno-hsa to all tests.
	* testsuite/libgomp.c++/c++.exp: Likewise.
	* testsuite/libgomp.fortran/fortran.exp: Likewise.
---
 libgomp/testsuite/libgomp.c++/c++.exp         | 2 +-
 libgomp/testsuite/libgomp.c/c.exp             | 2 +-
 libgomp/testsuite/libgomp.fortran/fortran.exp | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Comments

Jakub Jelinek March 1, 2016, 6:47 p.m. UTC | #1 
On Tue, Mar 01, 2016 at 07:39:18PM +0100, Martin Jambor wrote:
> as Jakub requested, this patch deals with HSA "excess errors" in the
> libgomp library testsuite by passing -Wno-hsa to all of them.  IIUC,
> that passing it in the second parameter of dg-runtest (as opposed to
> the third) means that it will apply even tests that have their own
> dg-options, which is presumably easier for everyone, provided that hsa
> will get is own libgomp testsuite directories.

What is the difference betwee the $flags and $default-extra-cflags
arguments to dg-runtest?  You seem to stick -Wno-hsa into the former,
which to me looks like it will be mentioned as part of the test
names (e.g. when cycling through -O* options, -Wno-hsa would be printed
along with -O2 etc.)?  Is there any reason not to stick it into the last
argument instead?

	Jakub
Mike Stump March 1, 2016, 7:07 p.m. UTC | #2 
On Mar 1, 2016, at 10:47 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> What is the difference betwee the $flags and $default-extra-cflags
> arguments to dg-runtest?  You seem to stick -Wno-hsa into the former,
> which to me looks like it will be mentioned as part of the test
> names (e.g. when cycling through -O* options, -Wno-hsa would be printed
> along with -O2 etc.)?  Is there any reason not to stick it into the last
> argument instead?

Not including this option in the test case name sounds nicer.
Martin Jambor March 1, 2016, 9:47 p.m. UTC | #3 
Hi

On Tue, Mar 01, 2016 at 07:47:49PM +0100, Jakub Jelinek wrote:
> On Tue, Mar 01, 2016 at 07:39:18PM +0100, Martin Jambor wrote:
> > as Jakub requested, this patch deals with HSA "excess errors" in the
> > libgomp library testsuite by passing -Wno-hsa to all of them.  IIUC,
> > that passing it in the second parameter of dg-runtest (as opposed to
> > the third) means that it will apply even tests that have their own
> > dg-options, which is presumably easier for everyone, provided that hsa
> > will get is own libgomp testsuite directories.
> 
> What is the difference betwee the $flags and $default-extra-cflags
> arguments to dg-runtest?

well, exactly what I wrote in the original email and what you have
quoted (and me as well) above.  But let me quote the dejagnu source
comment of dg-runtest, which is perhaps more clear:

  # FLAGS is a set of options to always pass.
  # DEFAULT_EXTRA_FLAGS is a set of options to pass if the testcase
  # doesn't
  # specify any (with dg-option).

So if I changed DEFAULT_EXTRA_FLAGS rather than FLAGS, I'd have to go
through all testcases specifying dg-options and add -Wno-hsa there
too.  Moreover, we'd have to add -Wno-hsa to all appropriate future
testcases if they specify their own dg-options.

Perhaps we should be using dg-additional-options in libgomp testsuite
wherever possible but there certainly are testcases using dg-options.

> You seem to stick -Wno-hsa into the former,
> which to me looks like it will be mentioned as part of the test
> names (e.g. when cycling through -O* options, -Wno-hsa would be printed
> along with -O2 etc.)?

Yes, that is an unfortunate side-effect. Furthermore, automated
comparison scripts might be confused by the change (mine was,
reporting all testcases as newly passed/xfailed and old as
disappeared).

But again, I do not have a strong preference, I can change the patches
to use DEFAULT_EXTRA_FLAGS and am willing to be watching for fallout
and fixing dg-options if you prefer that.  So let me know what you
consider nicer and I'll do it.

Thanks,

Martin
Jakub Jelinek March 1, 2016, 10:06 p.m. UTC | #4 
On Tue, Mar 01, 2016 at 10:47:46PM +0100, Martin Jambor wrote:
> well, exactly what I wrote in the original email and what you have
> quoted (and me as well) above.  But let me quote the dejagnu source
> comment of dg-runtest, which is perhaps more clear:
> 
>   # FLAGS is a set of options to always pass.
>   # DEFAULT_EXTRA_FLAGS is a set of options to pass if the testcase
>   # doesn't
>   # specify any (with dg-option).
> 
> So if I changed DEFAULT_EXTRA_FLAGS rather than FLAGS, I'd have to go
> through all testcases specifying dg-options and add -Wno-hsa there
> too.  Moreover, we'd have to add -Wno-hsa to all appropriate future
> testcases if they specify their own dg-options.

Ah, ok; what about adding
    # Disable HSA warnings by default.
    lappend ALWAYS_CFLAGS "additional_flags=-Wno-hsa"
in libgomp/testsuite/lib/libgomp.exp (next to e.g.
-fno-diagnostics-show-caret)?

	Jakub
Martin Jambor March 4, 2016, 3:27 p.m. UTC | #5 
Hi,

On Tue, Mar 01, 2016 at 11:06:43PM +0100, Jakub Jelinek wrote:
> On Tue, Mar 01, 2016 at 10:47:46PM +0100, Martin Jambor wrote:
> > well, exactly what I wrote in the original email and what you have
> > quoted (and me as well) above.  But let me quote the dejagnu source
> > comment of dg-runtest, which is perhaps more clear:
> > 
> >   # FLAGS is a set of options to always pass.
> >   # DEFAULT_EXTRA_FLAGS is a set of options to pass if the testcase
> >   # doesn't
> >   # specify any (with dg-option).
> > 
> > So if I changed DEFAULT_EXTRA_FLAGS rather than FLAGS, I'd have to go
> > through all testcases specifying dg-options and add -Wno-hsa there
> > too.  Moreover, we'd have to add -Wno-hsa to all appropriate future
> > testcases if they specify their own dg-options.
> 
> Ah, ok; what about adding
>     # Disable HSA warnings by default.
>     lappend ALWAYS_CFLAGS "additional_flags=-Wno-hsa"
> in libgomp/testsuite/lib/libgomp.exp (next to e.g.
> -fno-diagnostics-show-caret)?
> 

That works nicely (though I have to override it explicitely in the
libgomp.hsa.c directory with another -Whsa, but I guess we can live
with that).  So I will use the above for the libgomp case.

I have tried to come up with a similar alternative for
gcc.dg/gomp/gomp.exp, g++.dg/gomp/gomp.exp and gfortran/gomp/gomp.exp
but so far I have not achieved to make the C++ and Fortran cases work
in any other way but pass -Wno-hsa in FLAGS (and thus change the
name).  For C, adding the following before the main loop works, even
though it looks too much like a hack to me:

global TEST_ALWAYS_FLAGS
set TEST_ALWAYS_FLAGS [concat $TEST_ALWAYS_FLAGS "-Wno-hsa"]

However, the C++ and Fortran cases use gfortran-dg-runtest to cycle
through a set of torture options and I have not yet discovered the
right magic variable to set (for example, adding -Wno-hsa to
DG_TORTURE_OPTIONS elements does not work).

I'm afraid I have spent way too much time on this already, so unless
someone has any ideas, I'd suggest that we use the (already approved)
name-changing gomp patch as it is.  Or at least for C++ and Fortran.

Thanks,

Martin
Jakub Jelinek March 4, 2016, 3:31 p.m. UTC | #6 
On Fri, Mar 04, 2016 at 04:27:11PM +0100, Martin Jambor wrote:
> > Ah, ok; what about adding
> >     # Disable HSA warnings by default.
> >     lappend ALWAYS_CFLAGS "additional_flags=-Wno-hsa"
> > in libgomp/testsuite/lib/libgomp.exp (next to e.g.
> > -fno-diagnostics-show-caret)?
> > 
> 
> That works nicely (though I have to override it explicitely in the
> libgomp.hsa.c directory with another -Whsa, but I guess we can live
> with that).  So I will use the above for the libgomp case.

Ok.

> I have tried to come up with a similar alternative for
> gcc.dg/gomp/gomp.exp, g++.dg/gomp/gomp.exp and gfortran/gomp/gomp.exp
> but so far I have not achieved to make the C++ and Fortran cases work
> in any other way but pass -Wno-hsa in FLAGS (and thus change the
> name).  For C, adding the following before the main loop works, even
> though it looks too much like a hack to me:
> 
> global TEST_ALWAYS_FLAGS
> set TEST_ALWAYS_FLAGS [concat $TEST_ALWAYS_FLAGS "-Wno-hsa"]

Doesn't this also cause the -Wno-hsa option on all further tests executed by
other *.exp after gomp.exp by the same runtest invocation?

> However, the C++ and Fortran cases use gfortran-dg-runtest to cycle
> through a set of torture options and I have not yet discovered the
> right magic variable to set (for example, adding -Wno-hsa to
> DG_TORTURE_OPTIONS elements does not work).
> 
> I'm afraid I have spent way too much time on this already, so unless
> someone has any ideas, I'd suggest that we use the (already approved)
> name-changing gomp patch as it is.  Or at least for C++ and Fortran.

Do you have URL for what you refer to?

	Jakub
Martin Jambor March 4, 2016, 4:01 p.m. UTC | #7 
Hi,

On Fri, Mar 04, 2016 at 04:31:29PM +0100, Jakub Jelinek wrote:
> On Fri, Mar 04, 2016 at 04:27:11PM +0100, Martin Jambor wrote:
> > > Ah, ok; what about adding
> > >     # Disable HSA warnings by default.
> > >     lappend ALWAYS_CFLAGS "additional_flags=-Wno-hsa"
> > > in libgomp/testsuite/lib/libgomp.exp (next to e.g.
> > > -fno-diagnostics-show-caret)?
> > > 
> > 
> > That works nicely (though I have to override it explicitely in the
> > libgomp.hsa.c directory with another -Whsa, but I guess we can live
> > with that).  So I will use the above for the libgomp case.
> 
> Ok.
> 
> > I have tried to come up with a similar alternative for
> > gcc.dg/gomp/gomp.exp, g++.dg/gomp/gomp.exp and gfortran/gomp/gomp.exp
> > but so far I have not achieved to make the C++ and Fortran cases work
> > in any other way but pass -Wno-hsa in FLAGS (and thus change the
> > name).  For C, adding the following before the main loop works, even
> > though it looks too much like a hack to me:
> > 
> > global TEST_ALWAYS_FLAGS
> > set TEST_ALWAYS_FLAGS [concat $TEST_ALWAYS_FLAGS "-Wno-hsa"]
> 
> Doesn't this also cause the -Wno-hsa option on all further tests executed by
> other *.exp after gomp.exp by the same runtest invocation?
> 

Not in the limited runs that I experimented with so far, but I
certainly kept this possibility in mind too.  If so, I would either
set it back before invoking dg-finish or dismiss the whole idea.

> > However, the C++ and Fortran cases use gfortran-dg-runtest to cycle
> > through a set of torture options and I have not yet discovered the
> > right magic variable to set (for example, adding -Wno-hsa to
> > DG_TORTURE_OPTIONS elements does not work).
> > 
> > I'm afraid I have spent way too much time on this already, so unless
> > someone has any ideas, I'd suggest that we use the (already approved)
> > name-changing gomp patch as it is.  Or at least for C++ and Fortran.
> 
> Do you have URL for what you refer to?
> 

Sure, the patch has been posted here:

https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00071.html

and approved here:

https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00074.html

Martin
Jakub Jelinek March 4, 2016, 4:04 p.m. UTC | #8 
On Fri, Mar 04, 2016 at 05:01:34PM +0100, Martin Jambor wrote:
> Not in the limited runs that I experimented with so far, but I
> certainly kept this possibility in mind too.  If so, I would either
> set it back before invoking dg-finish or dismiss the whole idea.
> 
> > > However, the C++ and Fortran cases use gfortran-dg-runtest to cycle
> > > through a set of torture options and I have not yet discovered the
> > > right magic variable to set (for example, adding -Wno-hsa to
> > > DG_TORTURE_OPTIONS elements does not work).
> > > 
> > > I'm afraid I have spent way too much time on this already, so unless
> > > someone has any ideas, I'd suggest that we use the (already approved)
> > > name-changing gomp patch as it is.  Or at least for C++ and Fortran.
> > 
> > Do you have URL for what you refer to?
> > 
> 
> Sure, the patch has been posted here:
> 
> https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00071.html

For the g*.dg/gomp/, if you'd only move -Wno-hsa into the last argument
next to -fopenmp, how many tests would be affected?  If not really many,
perhaps those could be changed to use dg-additional-options instead of
dg-options.

	Jakub
Martin Jambor March 4, 2016, 5:12 p.m. UTC | #9 
On Fri, Mar 04, 2016 at 05:04:31PM +0100, Jakub Jelinek wrote:
> On Fri, Mar 04, 2016 at 05:01:34PM +0100, Martin Jambor wrote:
> > Not in the limited runs that I experimented with so far, but I
> > certainly kept this possibility in mind too.  If so, I would either
> > set it back before invoking dg-finish or dismiss the whole idea.
> > 
> > > > However, the C++ and Fortran cases use gfortran-dg-runtest to cycle
> > > > through a set of torture options and I have not yet discovered the
> > > > right magic variable to set (for example, adding -Wno-hsa to
> > > > DG_TORTURE_OPTIONS elements does not work).
> > > > 
> > > > I'm afraid I have spent way too much time on this already, so unless
> > > > someone has any ideas, I'd suggest that we use the (already approved)
> > > > name-changing gomp patch as it is.  Or at least for C++ and Fortran.
> > > 
> > > Do you have URL for what you refer to?
> > > 
> > 
> > Sure, the patch has been posted here:
> > 
> > https://gcc.gnu.org/ml/gcc-patches/2016-03/msg00071.html
> 
> For the g*.dg/gomp/, if you'd only move -Wno-hsa into the last argument
> next to -fopenmp, how many tests would be affected?

Out of 287 files that have dg-options with them in the gomp
directories, only 9 generate hsa warnings:

  c-c++-common/gomp/clauses-1.c:/* { dg-options "-fopenmp" } */
  c-c++-common/gomp/if-1.c:/* { dg-options "-fopenmp" } */
  c-c++-common/gomp/pr61486-2.c:/* { dg-options "-fopenmp" } */
  c-c++-common/gomp/target-teams-1.c:/* { dg-options "-fopenmp -fdump-tree-gimple" } */
  g++.dg/gomp/target-teams-1.C:// { dg-options "-fopenmp -fdump-tree-gimple" }
  gcc.dg/gomp/pr68128-2.c:/* { dg-options "-O2 -fopenmp -fdump-tree-omplower" } */
  gfortran.dg/gomp/target1.f90:! { dg-options "-fopenmp" }
  gfortran.dg/gomp/target2.f90:! { dg-options "-fopenmp -ffree-line-length-160" }
  gfortran.dg/gomp/target3.f90:! { dg-options "-fopenmp" }

> If not really many,
> perhaps those could be changed to use dg-additional-options instead of
> dg-options.

I do not know what -ffree-line-length-160 is, but probably all of
them, even though putting -O2 in gcc.dg/gomp/pr68128-2.c to
"additional" flags feels just wrong.

However, the real question is: Would such a solution really be much
better than the first version of the patch
(https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01813.html)?  After
all, in comparison it would only avoid touching two tests and it will
not avoid issues with tests added in future if they use dg-options.

Martin
Jakub Jelinek March 4, 2016, 5:39 p.m. UTC | #10 
On Fri, Mar 04, 2016 at 06:12:09PM +0100, Martin Jambor wrote:
> > For the g*.dg/gomp/, if you'd only move -Wno-hsa into the last argument
> > next to -fopenmp, how many tests would be affected?
> 
> Out of 287 files that have dg-options with them in the gomp
> directories, only 9 generate hsa warnings:
> 
>   c-c++-common/gomp/clauses-1.c:/* { dg-options "-fopenmp" } */
>   c-c++-common/gomp/if-1.c:/* { dg-options "-fopenmp" } */
>   c-c++-common/gomp/pr61486-2.c:/* { dg-options "-fopenmp" } */
>   c-c++-common/gomp/target-teams-1.c:/* { dg-options "-fopenmp -fdump-tree-gimple" } */
>   g++.dg/gomp/target-teams-1.C:// { dg-options "-fopenmp -fdump-tree-gimple" }
>   gcc.dg/gomp/pr68128-2.c:/* { dg-options "-O2 -fopenmp -fdump-tree-omplower" } */
>   gfortran.dg/gomp/target1.f90:! { dg-options "-fopenmp" }
>   gfortran.dg/gomp/target2.f90:! { dg-options "-fopenmp -ffree-line-length-160" }
>   gfortran.dg/gomp/target3.f90:! { dg-options "-fopenmp" }
> 
> > If not really many,
> > perhaps those could be changed to use dg-additional-options instead of
> > dg-options.
> 
> I do not know what -ffree-line-length-160 is, but probably all of
> them, even though putting -O2 in gcc.dg/gomp/pr68128-2.c to
> "additional" flags feels just wrong.

{ dg-options "-fopenmp" }
can go, that should be the default.
And, I'm fine with moving the -fdump-tree-*, -O2 and -ffree-line-length-160
to dg-additional-options.

I think I prefer it that way.

	Jakub
diff mbox

Patch

diff --git a/libgomp/testsuite/libgomp.c++/c++.exp b/libgomp/testsuite/libgomp.c++/c++.exp
index 0454f95..120e573 100644
--- a/libgomp/testsuite/libgomp.c++/c++.exp
+++ b/libgomp/testsuite/libgomp.c++/c++.exp
@@ -65,7 +65,7 @@  if { $lang_test_file_found } {
     }
 
     # Main loop.
-    dg-runtest $tests "" "$libstdcxx_includes $DEFAULT_CFLAGS"
+    dg-runtest $tests "-Wno-hsa" "$libstdcxx_includes $DEFAULT_CFLAGS"
 }
 
 # All done.
diff --git a/libgomp/testsuite/libgomp.c/c.exp b/libgomp/testsuite/libgomp.c/c.exp
index 300b921..d3cd144 100644
--- a/libgomp/testsuite/libgomp.c/c.exp
+++ b/libgomp/testsuite/libgomp.c/c.exp
@@ -31,7 +31,7 @@  append ld_library_path [gcc-set-multilib-library-path $GCC_UNDER_TEST]
 set_ld_library_path_env_vars
 
 # Main loop.
-dg-runtest $tests "" $DEFAULT_CFLAGS
+dg-runtest $tests "-Wno-hsa" $DEFAULT_CFLAGS
 
 # All done.
 dg-finish
diff --git a/libgomp/testsuite/libgomp.fortran/fortran.exp b/libgomp/testsuite/libgomp.fortran/fortran.exp
index 9e6b643..ea84d5c 100644
--- a/libgomp/testsuite/libgomp.fortran/fortran.exp
+++ b/libgomp/testsuite/libgomp.fortran/fortran.exp
@@ -66,7 +66,7 @@  if { $lang_test_file_found } {
     # 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++.
-    gfortran-dg-runtest $tests "" ""
+    gfortran-dg-runtest $tests "-Wno-hsa" ""
 }
 
 # All done.