Message ID | 20160301183918.GA2148@virgil.suse.cz |
---|---|
State | New |
Headers | show |
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
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.
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
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
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
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
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
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
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
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 --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.