Support parallel testing in libgomp, part I [PR66005]

  Hi!

[Putting Bernhard, Honza, Segher in CC, as they are eager to test this,
based on recent comments on IRC.]  ;-P


First, establish the parallel testing infrastructure -- while still
hard-coding the number of parallel slots to one.

On 2015-05-08T10:40:02+0200, I wrote:
> On Thu, 7 May 2015 13:39:40 +0200, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Thu, May 07, 2015 at 01:26:57PM +0200, Rainer Orth wrote:
>> > As reported in the PR, with the addition of all those OpenACC tests,
>> > libgomp make check times have skyrocketed since the testsuite is still
>> > run sequentially.
>
> ACK.  And, thanks for looking into that!
>
>> > Fixing this proved trivial: I managed to almost literally copy the
>> > solution from libstdc++-v3/testsuite/Makefile.am

So I recently had re-created this patch independently, before remembering
that Rainer had -- just eight years ago... ;-) -- already submitted this.
Fortunately, we've done it in almost the exact same way, just that a few
libgomp testsuite infrastructure as well as libstdc++ parallel testing
infrastructure changes (which is where these changes have been copied
from) have since gone in, which I've adjusted for.  Additionally, use
'check_DEJAGNU_libgomp_targets' instead of 'check_DEJAGNU_normal_targets'
etc. (where "normal" is a libstdc++ detail), and regarding:

>> > with a minimal change
>> > to libgomp.exp so the generated libgomp-test-support.exp file is found
>> > in both the sequential and parallel cases.  This isn't an issue in
>> > libstdc++ since all necessary variables are stored in a single
>> > site.exp.

... in 'libgomp/testsuite/lib/libgomp.exp', I've changed:

    -load_file libgomp-test-support.exp
    +# Search in both .. and . to support parallel and sequential testing.
    +load_file -1 ../libgomp-test-support.exp libgomp-test-support.exp

... into the more explicit:

    -load_file libgomp-test-support.exp
    +# Search in '..' vs. '.' to support parallel vs. sequential testing.
    +if [info exists ::env(GCC_RUNTEST_PARALLELIZE_DIR)] {
    +    load_file ../libgomp-test-support.exp
    +} else {
    +    load_file libgomp-test-support.exp
    +}

And, for now, I hard-code the number of parallel slots to one.  This
means that libgomp for 'make -j' now does use the parallel testing code
paths, but is restricted to just one slot.  That is, no actual change in
behavior, other than that 'libgomp.sum' then is filtered through
'contrib/dg-extract-results.sh'.

OK to push the attached
"Support parallel testing in libgomp, part I [PR66005]"?


Grüße
 Thomas


>> It is far from trivial though.
>> The point is that most of the OpenMP tests are parallelized with the
>> default OMP_NUM_THREADS, so running the tests in parallel oversubscribes the
>> machine a lot, the higher number of hw threads the more.
>
> Do you agree that we have two classes of test cases in libgomp: 1) test
> cases that don't place a considerably higher load on the machine compared
> to "normal" (single-threaded) execution tests, because they're just
> testing some functionality that is not expected to actively depend
> on/interfere with parallelism.  If needed, and/or if not already done,
> such test cases can be parameterized (OMP_NUM_THREADS, OpenACC num_gangs,
> num_workers, vector_length clauses, and so on) for low parallelism
> levels.  And, 2) test cases that place a considerably higher load on the
> machine compared to "normal" (single-threaded) execution tests, because
> they're testing some functionality that actively depends on/interferes
> with some kind of parallelism.  What about marking such tests specially,
> such that DejaGnu will only ever schedule one of them for execution at
> the same time?  For example, a new dg-* directive to run them wrapped
> through »flock [libgomp/testsuite/serial.lock] [a.out]« or some such?
>
>> If we go forward with some parallelization of the tests, we at least should
>> try to export something like OMP_WAIT_POLICY=passive so that the
>> oversubscribed machine would at least not spend too much time in spinning.
>
> (Will again have the problem that DejaGnu doesn't provide infrastructure
> to communicate environment variables to boards in remote testing.)
>
>> And perhaps reconsider running all OpenACC threads 3 times, just allow
>> user to select which offloading target they want to test (host fallback,
>> the host nonshm hack, PTX, XeonPHI in the future?), and test just that
>> (that is pretty much how OpenMP offloading testing works).
>
> My rationale is: if you configure GCC to support a set of offloading
> devices (more than one), you'll also want to get the test coverage that
> indeed all these work as expected.  (It currently doesn't matter, but...)
> that's something I'd like to see improved in the libgomp OpenMP
> offloading testing (once it supports more than one architecture for
> offloading).
>
>> For tests that
>> always want to test host fallback, I hope OpenACC offers clauses to force
>> the host fallback.
>
> Yes.
>
>
> Grüße,
>  Thomas


-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
  

On Fri, 5 May 2023 10:55:41 +0200
Thomas Schwinge <thomas@codesourcery.com> wrote:

> So I recently had re-created this patch independently, before remembering
> that Rainer had -- just eight years ago... ;-) -- already submitted this.

thanks to you both :)

> etc. (where "normal" is a libstdc++ detail), and regarding:
> 
> >> > with a minimal change
> >> > to libgomp.exp so the generated libgomp-test-support.exp file is found
> >> > in both the sequential and parallel cases.  This isn't an issue in
> >> > libstdc++ since all necessary variables are stored in a single
> >> > site.exp.  
> 
> ... in 'libgomp/testsuite/lib/libgomp.exp', I've changed:
> 
>     -load_file libgomp-test-support.exp
>     +# Search in both .. and . to support parallel and sequential testing.
>     +load_file -1 ../libgomp-test-support.exp libgomp-test-support.exp
> 
> ... into the more explicit:
> 
>     -load_file libgomp-test-support.exp
>     +# Search in '..' vs. '.' to support parallel vs. sequential testing.
>     +if [info exists ::env(GCC_RUNTEST_PARALLELIZE_DIR)] {
>     +    load_file ../libgomp-test-support.exp
>     +} else {
>     +    load_file libgomp-test-support.exp
>     +}

Do we have to re-read those? Otherwise this would be load_lib:

We have libdirs in the minimum deja we require.

Speaking of which. IIRC i additionally deleted all load_gcc_lib:
https://gcc.gnu.org/legacy-ml/fortran/2012-03/msg00094.html
in lib{atomic,ffi,go,gomp,itm,phobos,stdc++-v3,vtv}

> And, for now, I hard-code the number of parallel slots to one.  This
> means that libgomp for 'make -j' now does use the parallel testing code
> paths, but is restricted to just one slot.  That is, no actual change in
> behavior, other than that 'libgomp.sum' then is filtered through
> 'contrib/dg-extract-results.sh'.
> 
> OK to push the attached
> "Support parallel testing in libgomp, part I [PR66005]"?

Some cosmetic nits.
See Jakubs one_to_9999.

+	@test ! -f $*/site.exp || mv $*/site.exp $*/site.bak
that's twisted

+	  rm -rf libgomp-parallel || true; \

just || :; \
I count 4 times.

There seems to be a mixture of ${PWD_COMMAND} and am__cd && pwd:
+	@objdir=`${PWD_COMMAND}`/$*; \
+	srcdir=`$(am__cd) $(srcdir) && pwd`; export srcdir; \

+	runtest=$(_RUNTEST); \
+	if [ -z "$$runtest" ]; then runtest=runtest; fi; \
I think I have plain $${RUNTEST-runtest}
off the default wildcard $(top_srcdir)/../dejagnu/runtest

> >> It is far from trivial though.
> >> The point is that most of the OpenMP tests are parallelized with the
> >> default OMP_NUM_THREADS, so running the tests in parallel oversubscribes the
> >> machine a lot, the higher number of hw threads the more.  
> >
> > Do you agree that we have two classes of test cases in libgomp: 1) test
> > cases that don't place a considerably higher load on the machine compared
> > to "normal" (single-threaded) execution tests, because they're just
> > testing some functionality that is not expected to actively depend
> > on/interfere with parallelism.  If needed, and/or if not already done,
> > such test cases can be parameterized (OMP_NUM_THREADS, OpenACC num_gangs,
> > num_workers, vector_length clauses, and so on) for low parallelism
> > levels.  And, 2) test cases that place a considerably higher load on the
> > machine compared to "normal" (single-threaded) execution tests, because
> > they're testing some functionality that actively depends on/interferes
> > with some kind of parallelism.  What about marking such tests specially,
> > such that DejaGnu will only ever schedule one of them for execution at
> > the same time?  For example, a new dg-* directive to run them wrapped
> > through »flock [libgomp/testsuite/serial.lock] [a.out]« or some such?

I think we all agree one that, yes.

> >  
> >> If we go forward with some parallelization of the tests, we at least should
> >> try to export something like OMP_WAIT_POLICY=passive so that the
> >> oversubscribed machine would at least not spend too much time in spinning.  
> >
> > (Will again have the problem that DejaGnu doesn't provide infrastructure
> > to communicate environment variables to boards in remote testing.)

Are you sure? I'm pretty confident that this worked fine at least at
one point in the past for certain targets.

The rest of these 2 patches LGTM. Let's see what others have to say.
thanks,
  

Hi Bernhard!

Thanks for your comments.


On 2023-05-06T16:15:45+0200, Bernhard Reutner-Fischer via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> On Fri, 5 May 2023 10:55:41 +0200
> Thomas Schwinge <thomas@codesourcery.com> wrote:
>
>> >> > with a minimal change
>> >> > to libgomp.exp so the generated libgomp-test-support.exp file is found
>> >> > in both the sequential and parallel cases.  This isn't an issue in
>> >> > libstdc++ since all necessary variables are stored in a single
>> >> > site.exp.
>>
>> ... in 'libgomp/testsuite/lib/libgomp.exp', I've changed:
>>
>>     -load_file libgomp-test-support.exp
>>     +# Search in both .. and . to support parallel and sequential testing.
>>     +load_file -1 ../libgomp-test-support.exp libgomp-test-support.exp
>>
>> ... into the more explicit:
>>
>>     -load_file libgomp-test-support.exp
>>     +# Search in '..' vs. '.' to support parallel vs. sequential testing.
>>     +if [info exists ::env(GCC_RUNTEST_PARALLELIZE_DIR)] {
>>     +    load_file ../libgomp-test-support.exp
>>     +} else {
>>     +    load_file libgomp-test-support.exp
>>     +}
>
> Do we have to re-read those? Otherwise this would be load_lib:

Indeed there is some confusion there; conceptually the content of that
file has to be expected to vary per libgomp (multilib) build variant.
On the other hand, given the GCC target libraries testing setup, we're
running testing always via the default variant's build, so always read
the default variant's file.  I agree that should get un-confused, however
it's a pre-existing issue that I suggest we tackle independently of this
mechanical change.

> We have libdirs in the minimum deja we require.
>
> Speaking of which. IIRC i additionally deleted all load_gcc_lib:
> https://gcc.gnu.org/legacy-ml/fortran/2012-03/msg00094.html
> in lib{atomic,ffi,go,gomp,itm,phobos,stdc++-v3,vtv}

Aha, another pre-existing/independent thing to look into.


>> And, for now, I hard-code the number of parallel slots to one.  This
>> means that libgomp for 'make -j' now does use the parallel testing code
>> paths, but is restricted to just one slot.  That is, no actual change in
>> behavior, other than that 'libgomp.sum' then is filtered through
>> 'contrib/dg-extract-results.sh'.
>>
>> OK to push the attached
>> "Support parallel testing in libgomp, part I [PR66005]"?
>
> Some cosmetic nits.
> See Jakubs one_to_9999.

Thanks.  That got installed while I'd already finished my patch.  ;-)
Note that the new 'one_to_9999' etc. is just the existing
'check_p_numbers' etc. renamed and moved, as it now has a second use.
I'm happy to incorporate that into my changes -- if we agree that it
should also go into other GCC target libraries' parallel testing code:
libphobos, libstdc++-v3.

> +     @test ! -f $*/site.exp || mv $*/site.exp $*/site.bak
> that's twisted
>
> +       rm -rf libgomp-parallel || true; \
>
> just || :; \
> I count 4 times.
>
> There seems to be a mixture of ${PWD_COMMAND} and am__cd && pwd:
> +     @objdir=`${PWD_COMMAND}`/$*; \
> +     srcdir=`$(am__cd) $(srcdir) && pwd`; export srcdir; \
>
> +     runtest=$(_RUNTEST); \
> +     if [ -z "$$runtest" ]; then runtest=runtest; fi; \
> I think I have plain $${RUNTEST-runtest}
> off the default wildcard $(top_srcdir)/../dejagnu/runtest

As I've written: "libstdc++ parallel testing infrastructure [...] is
where these changes have been copied from", and that one had it copied
from GCC compilers parallel testing infrastructure, I presume.  I do
agree that there's value in unifying and, as feasible, simplifying this
code, but again that to me sounds like a pre-existing/separate thing to
do?


>> >> It is far from trivial though.
>> >> The point is that most of the OpenMP tests are parallelized with the
>> >> default OMP_NUM_THREADS, so running the tests in parallel oversubscribes the
>> >> machine a lot, the higher number of hw threads the more.
>> >
>> > Do you agree that we have two classes of test cases in libgomp: 1) test
>> > cases that don't place a considerably higher load on the machine compared
>> > to "normal" (single-threaded) execution tests, because they're just
>> > testing some functionality that is not expected to actively depend
>> > on/interfere with parallelism.  If needed, and/or if not already done,
>> > such test cases can be parameterized (OMP_NUM_THREADS, OpenACC num_gangs,
>> > num_workers, vector_length clauses, and so on) for low parallelism
>> > levels.  And, 2) test cases that place a considerably higher load on the
>> > machine compared to "normal" (single-threaded) execution tests, because
>> > they're testing some functionality that actively depends on/interferes
>> > with some kind of parallelism.  What about marking such tests specially,
>> > such that DejaGnu will only ever schedule one of them for execution at
>> > the same time?  For example, a new dg-* directive to run them wrapped
>> > through »flock [libgomp/testsuite/serial.lock] [a.out]« or some such?
>
> I think we all agree one that, yes.

Conceptually, yes.  However, given that my
"Support parallel testing in libgomp, part II [PR66005]" changes already
seem to enable a great amount of parallelism, occupying CPUs almost
fully,  I'm not actually sure now if adding more parallelism via
tedious-to-maintain new dg-* directives is actually necessary/useful.  As
long as libgomp testing now no longer is at the long tail end of overall
testing time, I'd rather keep it simple?


>> >> If we go forward with some parallelization of the tests, we at least should
>> >> try to export something like OMP_WAIT_POLICY=passive so that the
>> >> oversubscribed machine would at least not spend too much time in spinning.
>> >
>> > (Will again have the problem that DejaGnu doesn't provide infrastructure
>> > to communicate environment variables to boards in remote testing.)
>
> Are you sure? I'm pretty confident that this worked fine at least at
> one point in the past for certain targets.

For example, see the comments/references in recent
<https://inbox.sourceware.org/018bcdeb-b3bb-1859-cd0b-a8a92e26d3a0@codesourcery.com>.


> The rest of these 2 patches LGTM. Let's see what others have to say.

Thanks!


Grüße
 Thomas
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955
  

On Mon, 8 May 2023 12:42:33 +0200
Thomas Schwinge <thomas@codesourcery.com> wrote:

> >>     +if [info exists ::env(GCC_RUNTEST_PARALLELIZE_DIR)] {
> >>     +    load_file ../libgomp-test-support.exp
> >>     +} else {
> >>     +    load_file libgomp-test-support.exp
> >>     +}  
> >
> > Do we have to re-read those? Otherwise this would be load_lib:  
> 
> Indeed there is some confusion there; conceptually the content of that
> file has to be expected to vary per libgomp (multilib) build variant.
> On the other hand, given the GCC target libraries testing setup, we're
> running testing always via the default variant's build, so always read
> the default variant's file.  I agree that should get un-confused, however
> it's a pre-existing issue that I suggest we tackle independently of this
> mechanical change.

Sure. One thing at a time.

> > Some cosmetic nits.
> > See Jakubs one_to_9999.  
> 
> Thanks.  That got installed while I'd already finished my patch.  ;-)
> Note that the new 'one_to_9999' etc. is just the existing
> 'check_p_numbers' etc. renamed and moved, as it now has a second use.
> I'm happy to incorporate that into my changes -- if we agree that it
> should also go into other GCC target libraries' parallel testing code:
> libphobos, libstdc++-v3.

Yes. Streamlining these can be done in follow-ups if the respective
maintainers agree.

> >> >> It is far from trivial though.
> >> >> The point is that most of the OpenMP tests are parallelized with the
> >> >> default OMP_NUM_THREADS, so running the tests in parallel oversubscribes the
> >> >> machine a lot, the higher number of hw threads the more.  
[]
> >> > the same time?  For example, a new dg-* directive to run them wrapped
> >> > through »flock [libgomp/testsuite/serial.lock] [a.out]« or some such?  
> >
> > I think we all agree one that, yes.  
> 
> Conceptually, yes.  However, given that my
> "Support parallel testing in libgomp, part II [PR66005]" changes already
> seem to enable a great amount of parallelism, occupying CPUs almost
> fully,  I'm not actually sure now if adding more parallelism via
> tedious-to-maintain new dg-* directives is actually necessary/useful.  As
> long as libgomp testing now no longer is at the long tail end of overall
> testing time, I'd rather keep it simple?

If the testsuite runtime is fine with your part II as is then we
should keep it as simple as possible.

> >> > (Will again have the problem that DejaGnu doesn't provide infrastructure
> >> > to communicate environment variables to boards in remote testing.)  
> >
> > Are you sure? I'm pretty confident that this worked fine at least at
> > one point in the past for certain targets.  
> 
> For example, see the comments/references in recent
> <https://inbox.sourceware.org/018bcdeb-b3bb-1859-cd0b-a8a92e26d3a0@codesourcery.com>.

oh, i think i had an rsh2 remote that uploaded $program.env and the rsh
itself was something like
 $RSH $rsh_useropts $hostname sh -c 'test -r $program.env && .
 $program.env \\; $program $pargs \\; echo ...

but that was ages ago and was properly implemented in the meantime, one
would hope? Well, apparently not.

PS: Back then I usually needed very different per-program env and not a
single, static env. So for me it made no sense to have a set of default
env and have tests add their own variables on-demand on top of the
default. The situation in gcc might be the exact opposite?

thanks,