Re: Support parallel testing in libgomp, part I [PR66005]

[Thomas Schwinge <thomas@codesourcery.com>]

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