Re: [PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
On 4/21/21 10:16 PM, Jakub Jelinek wrote: > On Wed, Apr 21, 2021 at 08:53:54PM +0100, Jonathan Wakely wrote: What would be IMHO a good idea would be to use configure test for #include int t = std::thread::hardware_concurrency (); and in that case use that as a fallback to the previous implementation, that will be strictly an improvement. >>> >>> That does not make sense to me. Original motivation was removal of the >>> complicated >>> implementation with a C++ standard function. > > The reason it is larger and more complicated is that it answers a different > question than std::thread::hardware_concurrency(). > hardware_concurrency() says how many threads the hw has available, if we > wanted just that, you could just keep the sysconf call. > The current implementation answers the question how many threads can this > process and its children be scheduled on. > Consider some NUMA box with hundreds of threads, but where it might > be desirable to keep specific compilations limited to some cores and do > other compilations or whatever else on other cores. > > Sure, hardware_concurrency() answer is better than no information about the > concurrency at all. But I don't really see the costs of the current > implementation, it is < 100 lines of code that have been written once and > it is unlikely they will need changing - at this point the interfaces are > fairly stable, haven't been changed on the glibc side for years. It is true > that when it was introduced in glibc, there have been 2 big changes to it > soon. All right, I understand that hardware_concurrency doesn't match exactly our needs. Thus I'm going to install the following patch that removes the FIXME. Thanks for help with that, Martin > > Jakub > >From dbe87a4229126829e0a912e9928c82b8dde7b51c Mon Sep 17 00:00:00 2001 From: Martin Liska Date: Thu, 22 Apr 2021 09:14:28 +0200 Subject: [PATCH] Remove not feasible FIXME gcc/ChangeLog: * lto-wrapper.c: Remove FIXME about usage of hardware_concurrency. The function is not on par with what we have now. --- gcc/lto-wrapper.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c index 0b626d7c811..49894e4fc61 100644 --- a/gcc/lto-wrapper.c +++ b/gcc/lto-wrapper.c @@ -1304,8 +1304,6 @@ init_num_threads (void) #endif } -/* FIXME: once using -std=c++11, we can use std::thread::hardware_concurrency. */ - /* Test and return reason why a jobserver cannot be detected. */ static const char * -- 2.31.1
Re: [PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
On 4/21/21 9:53 PM, Jonathan Wakely wrote:
> On Wed, 21 Apr 2021 at 20:41, Martin Liška wrote:
>>
>> On 4/21/21 9:15 PM, Jakub Jelinek wrote:
>>> On Wed, Apr 21, 2021 at 08:28:55PM +0200, Jakub Jelinek via Gcc-patches
>>> wrote:
> There's a patch attempt for the problem with
> std::thread::hardware_concurrency where
> it's used only if _GLIBCXX_HAS_GTHREADS is set.
>
> Does it help?
> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> PR bootstrap/100186
> * lto-wrapper.c: Use hardware_concurrency only if
> _GLIBCXX_HAS_GTHREADS.
> ---
> gcc/lto-wrapper.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> index 6ba401007f6..8a85b3e93a8 100644
> --- a/gcc/lto-wrapper.c
> +++ b/gcc/lto-wrapper.c
> @@ -1285,7 +1285,11 @@ run_gcc (unsigned argc, char *argv[])
>static char current_dir[] = { '.', DIR_SEPARATOR, '\0' };
>
>/* Number of CPUs that can be used for parallel LTRANS phase. */
> - unsigned long nthreads_var = std::thread::hardware_concurrency ();
> + unsigned long nthreads_var = 0;
> +
> +#ifdef _GLIBCXX_HAS_GTHREADS
> + nthreads_var = std::thread::hardware_concurrency ();
> +#endif
_GLIBCXX_HAS_GTHREADS is a libstdc++ internal macro, it shouldn't be used
outside of libstdc++.
And, when using some other compiler or standard C++ library, nthreads_var
will be always 0. That isn't an improvement.
>>>
>>> What would be IMHO a good idea would be to use configure test for
>>> #include
>>> int t = std::thread::hardware_concurrency ();
>>> and in that case use that as a fallback to the previous implementation,
>>> that will be strictly an improvement.
>>
>> That does not make sense to me. Original motivation was removal of the
>> complicated
>> implementation with a C++ standard function.
>>
>> @Jonathan:
>>
>> What standard says about usage of std::thread::hardware_concurrency() [1]?
>> The function is defined in C++11, how can one detect if threading is enabled
>> in the C++ library and use it?
>
> A configure test.
>
> The standard says it's required to exist, but libstdc++ has
> traditionally not defined things that are not supportable on the
> target.
Which seems logical to me.
> So for targets with no multithreading, we didn't define
> anything in . Even though that has changed in current releases
> (so that std::thread exists even if it's useless), that won't help you
> with previous releases. You need a configure test.
All right.
>
> As I said in the PR, opinions differ on whether it's better to define
> everything (even if it's useless) or not define them. Arguably, users
> on small embedded targets do not want the library to waste space
> defining a useless std::thread class that just throws an exception if
> you ever try to create a new thread (but that's what the standard
> requires).
>
I think defining only things that work is the right way.
Martin
Re: [PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
On Wed, Apr 21, 2021 at 08:53:54PM +0100, Jonathan Wakely wrote: > > > What would be IMHO a good idea would be to use configure test for > > > #include > > > int t = std::thread::hardware_concurrency (); > > > and in that case use that as a fallback to the previous implementation, > > > that will be strictly an improvement. > > > > That does not make sense to me. Original motivation was removal of the > > complicated > > implementation with a C++ standard function. The reason it is larger and more complicated is that it answers a different question than std::thread::hardware_concurrency(). hardware_concurrency() says how many threads the hw has available, if we wanted just that, you could just keep the sysconf call. The current implementation answers the question how many threads can this process and its children be scheduled on. Consider some NUMA box with hundreds of threads, but where it might be desirable to keep specific compilations limited to some cores and do other compilations or whatever else on other cores. Sure, hardware_concurrency() answer is better than no information about the concurrency at all. But I don't really see the costs of the current implementation, it is < 100 lines of code that have been written once and it is unlikely they will need changing - at this point the interfaces are fairly stable, haven't been changed on the glibc side for years. It is true that when it was introduced in glibc, there have been 2 big changes to it soon. Jakub
Re: [PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
> "Jakub" == Jakub Jelinek via Gcc-patches writes:
Jakub> What would be IMHO a good idea would be to use configure test for
Jakub> #include
Jakub> int t = std::thread::hardware_concurrency ();
Jakub> and in that case use that as a fallback to the previous implementation,
Jakub> that will be strictly an improvement.
FWIW, gdb had to do this. The check is in gdbsupport/common.m4.
I've appended it for convenience.
Tom
# Check for std::thread. This does not work on some platforms, like
# mingw and DJGPP.
AC_LANG_PUSH([C++])
AX_PTHREAD([threads=yes], [threads=no])
if test "$threads" = "yes"; then
save_LIBS="$LIBS"
LIBS="$PTHREAD_LIBS $LIBS"
save_CXXFLAGS="$CXXFLAGS"
CXXFLAGS="$PTHREAD_CFLAGS $save_CXXFLAGS"
AC_CACHE_CHECK([for std::thread],
gdb_cv_cxx_std_thread,
[AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
[[#include
void callback() { }]],
[[std::thread t(callback);]])],
gdb_cv_cxx_std_thread=yes,
gdb_cv_cxx_std_thread=no)])
# This check must be here, while LIBS includes any necessary
# threading library.
AC_CHECK_FUNCS([pthread_sigmask pthread_setname_np])
LIBS="$save_LIBS"
CXXFLAGS="$save_CXXFLAGS"
fi
if test "$gdb_cv_cxx_std_thread" = "yes"; then
AC_DEFINE(CXX_STD_THREAD, 1,
[Define to 1 if std::thread works.])
fi
AC_LANG_POP
Re: [PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
On Wed, 21 Apr 2021 at 20:41, Martin Liška wrote:
>
> On 4/21/21 9:15 PM, Jakub Jelinek wrote:
> > On Wed, Apr 21, 2021 at 08:28:55PM +0200, Jakub Jelinek via Gcc-patches
> > wrote:
> >>> There's a patch attempt for the problem with
> >>> std::thread::hardware_concurrency where
> >>> it's used only if _GLIBCXX_HAS_GTHREADS is set.
> >>>
> >>> Does it help?
> >>> Thanks,
> >>> Martin
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> PR bootstrap/100186
> >>> * lto-wrapper.c: Use hardware_concurrency only if
> >>> _GLIBCXX_HAS_GTHREADS.
> >>> ---
> >>> gcc/lto-wrapper.c | 6 +-
> >>> 1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> >>> index 6ba401007f6..8a85b3e93a8 100644
> >>> --- a/gcc/lto-wrapper.c
> >>> +++ b/gcc/lto-wrapper.c
> >>> @@ -1285,7 +1285,11 @@ run_gcc (unsigned argc, char *argv[])
> >>>static char current_dir[] = { '.', DIR_SEPARATOR, '\0' };
> >>>
> >>>/* Number of CPUs that can be used for parallel LTRANS phase. */
> >>> - unsigned long nthreads_var = std::thread::hardware_concurrency ();
> >>> + unsigned long nthreads_var = 0;
> >>> +
> >>> +#ifdef _GLIBCXX_HAS_GTHREADS
> >>> + nthreads_var = std::thread::hardware_concurrency ();
> >>> +#endif
> >>
> >> _GLIBCXX_HAS_GTHREADS is a libstdc++ internal macro, it shouldn't be used
> >> outside of libstdc++.
> >> And, when using some other compiler or standard C++ library, nthreads_var
> >> will be always 0. That isn't an improvement.
> >
> > What would be IMHO a good idea would be to use configure test for
> > #include
> > int t = std::thread::hardware_concurrency ();
> > and in that case use that as a fallback to the previous implementation,
> > that will be strictly an improvement.
>
> That does not make sense to me. Original motivation was removal of the
> complicated
> implementation with a C++ standard function.
>
> @Jonathan:
>
> What standard says about usage of std::thread::hardware_concurrency() [1]?
> The function is defined in C++11, how can one detect if threading is enabled
> in the C++ library and use it?
A configure test.
The standard says it's required to exist, but libstdc++ has
traditionally not defined things that are not supportable on the
target. So for targets with no multithreading, we didn't define
anything in . Even though that has changed in current releases
(so that std::thread exists even if it's useless), that won't help you
with previous releases. You need a configure test.
As I said in the PR, opinions differ on whether it's better to define
everything (even if it's useless) or not define them. Arguably, users
on small embedded targets do not want the library to waste space
defining a useless std::thread class that just throws an exception if
you ever try to create a new thread (but that's what the standard
requires).
Re: [PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
On 4/21/21 9:15 PM, Jakub Jelinek wrote:
> On Wed, Apr 21, 2021 at 08:28:55PM +0200, Jakub Jelinek via Gcc-patches wrote:
>>> There's a patch attempt for the problem with
>>> std::thread::hardware_concurrency where
>>> it's used only if _GLIBCXX_HAS_GTHREADS is set.
>>>
>>> Does it help?
>>> Thanks,
>>> Martin
>>>
>>> gcc/ChangeLog:
>>>
>>> PR bootstrap/100186
>>> * lto-wrapper.c: Use hardware_concurrency only if
>>> _GLIBCXX_HAS_GTHREADS.
>>> ---
>>> gcc/lto-wrapper.c | 6 +-
>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
>>> index 6ba401007f6..8a85b3e93a8 100644
>>> --- a/gcc/lto-wrapper.c
>>> +++ b/gcc/lto-wrapper.c
>>> @@ -1285,7 +1285,11 @@ run_gcc (unsigned argc, char *argv[])
>>>static char current_dir[] = { '.', DIR_SEPARATOR, '\0' };
>>>
>>>/* Number of CPUs that can be used for parallel LTRANS phase. */
>>> - unsigned long nthreads_var = std::thread::hardware_concurrency ();
>>> + unsigned long nthreads_var = 0;
>>> +
>>> +#ifdef _GLIBCXX_HAS_GTHREADS
>>> + nthreads_var = std::thread::hardware_concurrency ();
>>> +#endif
>>
>> _GLIBCXX_HAS_GTHREADS is a libstdc++ internal macro, it shouldn't be used
>> outside of libstdc++.
>> And, when using some other compiler or standard C++ library, nthreads_var
>> will be always 0. That isn't an improvement.
>
> What would be IMHO a good idea would be to use configure test for
> #include
> int t = std::thread::hardware_concurrency ();
> and in that case use that as a fallback to the previous implementation,
> that will be strictly an improvement.
That does not make sense to me. Original motivation was removal of the
complicated
implementation with a C++ standard function.
@Jonathan:
What standard says about usage of std::thread::hardware_concurrency() [1]?
The function is defined in C++11, how can one detect if threading is enabled
in the C++ library and use it?
[1] https://en.cppreference.com/w/cpp/thread/thread/hardware_concurrency
>
> Jakub
>
Re: [PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
On Wed, Apr 21, 2021 at 03:25:26PM -0400, David Edelsohn wrote: > > What would be IMHO a good idea would be to use configure test for > > #include > > int t = std::thread::hardware_concurrency (); > > and in that case use that as a fallback to the previous implementation, > > that will be strictly an improvement. > > Would it be good enough to add a compile-time test for GCC VERSION >= > 10 and fallback to the value 1 otherwise? If one performs the No, that will make all cross-compilers LTO non-parallelized. Adding a configure check for it is very easy. Jakub
Re: [PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
On Wed, Apr 21, 2021 at 3:16 PM Jakub Jelinek wrote:
>
> On Wed, Apr 21, 2021 at 08:28:55PM +0200, Jakub Jelinek via Gcc-patches wrote:
> > > There's a patch attempt for the problem with
> > > std::thread::hardware_concurrency where
> > > it's used only if _GLIBCXX_HAS_GTHREADS is set.
> > >
> > > Does it help?
> > > Thanks,
> > > Martin
> > >
> > > gcc/ChangeLog:
> > >
> > > PR bootstrap/100186
> > > * lto-wrapper.c: Use hardware_concurrency only if
> > > _GLIBCXX_HAS_GTHREADS.
> > > ---
> > > gcc/lto-wrapper.c | 6 +-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> > > index 6ba401007f6..8a85b3e93a8 100644
> > > --- a/gcc/lto-wrapper.c
> > > +++ b/gcc/lto-wrapper.c
> > > @@ -1285,7 +1285,11 @@ run_gcc (unsigned argc, char *argv[])
> > >static char current_dir[] = { '.', DIR_SEPARATOR, '\0' };
> > >
> > >/* Number of CPUs that can be used for parallel LTRANS phase. */
> > > - unsigned long nthreads_var = std::thread::hardware_concurrency ();
> > > + unsigned long nthreads_var = 0;
> > > +
> > > +#ifdef _GLIBCXX_HAS_GTHREADS
> > > + nthreads_var = std::thread::hardware_concurrency ();
> > > +#endif
> >
> > _GLIBCXX_HAS_GTHREADS is a libstdc++ internal macro, it shouldn't be used
> > outside of libstdc++.
> > And, when using some other compiler or standard C++ library, nthreads_var
> > will be always 0. That isn't an improvement.
>
> What would be IMHO a good idea would be to use configure test for
> #include
> int t = std::thread::hardware_concurrency ();
> and in that case use that as a fallback to the previous implementation,
> that will be strictly an improvement.
Would it be good enough to add a compile-time test for GCC VERSION >=
10 and fallback to the value 1 otherwise? If one performs the
recommended bootstrap of the compiler, stage2 and stage3 will pick up
from the newly built libstdc++ that always defines
std::thread and will provide the expected feature. The problem is the
stage1 compiler that may be built with an older compiler or building
GCC without performing a three-stage bootstrap.
Thanks, David
Re: [PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
On Wed, Apr 21, 2021 at 08:28:55PM +0200, Jakub Jelinek via Gcc-patches wrote:
> > There's a patch attempt for the problem with
> > std::thread::hardware_concurrency where
> > it's used only if _GLIBCXX_HAS_GTHREADS is set.
> >
> > Does it help?
> > Thanks,
> > Martin
> >
> > gcc/ChangeLog:
> >
> > PR bootstrap/100186
> > * lto-wrapper.c: Use hardware_concurrency only if
> > _GLIBCXX_HAS_GTHREADS.
> > ---
> > gcc/lto-wrapper.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> > index 6ba401007f6..8a85b3e93a8 100644
> > --- a/gcc/lto-wrapper.c
> > +++ b/gcc/lto-wrapper.c
> > @@ -1285,7 +1285,11 @@ run_gcc (unsigned argc, char *argv[])
> >static char current_dir[] = { '.', DIR_SEPARATOR, '\0' };
> >
> >/* Number of CPUs that can be used for parallel LTRANS phase. */
> > - unsigned long nthreads_var = std::thread::hardware_concurrency ();
> > + unsigned long nthreads_var = 0;
> > +
> > +#ifdef _GLIBCXX_HAS_GTHREADS
> > + nthreads_var = std::thread::hardware_concurrency ();
> > +#endif
>
> _GLIBCXX_HAS_GTHREADS is a libstdc++ internal macro, it shouldn't be used
> outside of libstdc++.
> And, when using some other compiler or standard C++ library, nthreads_var
> will be always 0. That isn't an improvement.
What would be IMHO a good idea would be to use configure test for
#include
int t = std::thread::hardware_concurrency ();
and in that case use that as a fallback to the previous implementation,
that will be strictly an improvement.
Jakub
Re: [PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
On Wed, Apr 21, 2021 at 08:28:56PM +0200, Jakub Jelinek wrote:
> On Wed, Apr 21, 2021 at 08:04:18PM +0200, Martin Liška wrote:
> > Hello.
> >
> > There's a patch attempt for the problem with
> > std::thread::hardware_concurrency where
> > it's used only if _GLIBCXX_HAS_GTHREADS is set.
> >
> > Does it help?
> > Thanks,
> > Martin
> >
> > gcc/ChangeLog:
> >
> > PR bootstrap/100186
> > * lto-wrapper.c: Use hardware_concurrency only if
> > _GLIBCXX_HAS_GTHREADS.
> > ---
> > gcc/lto-wrapper.c | 6 +-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> > index 6ba401007f6..8a85b3e93a8 100644
> > --- a/gcc/lto-wrapper.c
> > +++ b/gcc/lto-wrapper.c
> > @@ -1285,7 +1285,11 @@ run_gcc (unsigned argc, char *argv[])
> >static char current_dir[] = { '.', DIR_SEPARATOR, '\0' };
> >
> >/* Number of CPUs that can be used for parallel LTRANS phase. */
> > - unsigned long nthreads_var = std::thread::hardware_concurrency ();
> > + unsigned long nthreads_var = 0;
> > +
> > +#ifdef _GLIBCXX_HAS_GTHREADS
> > + nthreads_var = std::thread::hardware_concurrency ();
> > +#endif
>
> _GLIBCXX_HAS_GTHREADS is a libstdc++ internal macro, it shouldn't be used
> outside of libstdc++.
> And, when using some other compiler or standard C++ library, nthreads_var
> will be always 0. That isn't an improvement.
Not to mention that it won't work properly for system GCC 4.8 - 9.x
where defined(_GLIBCXX_HAS_GTHREADS) doesn't imply std::thread is available.
Jakub
Re: [PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
On Wed, Apr 21, 2021 at 08:04:18PM +0200, Martin Liška wrote:
> Hello.
>
> There's a patch attempt for the problem with
> std::thread::hardware_concurrency where
> it's used only if _GLIBCXX_HAS_GTHREADS is set.
>
> Does it help?
> Thanks,
> Martin
>
> gcc/ChangeLog:
>
> PR bootstrap/100186
> * lto-wrapper.c: Use hardware_concurrency only if
> _GLIBCXX_HAS_GTHREADS.
> ---
> gcc/lto-wrapper.c | 6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
> index 6ba401007f6..8a85b3e93a8 100644
> --- a/gcc/lto-wrapper.c
> +++ b/gcc/lto-wrapper.c
> @@ -1285,7 +1285,11 @@ run_gcc (unsigned argc, char *argv[])
>static char current_dir[] = { '.', DIR_SEPARATOR, '\0' };
>
>/* Number of CPUs that can be used for parallel LTRANS phase. */
> - unsigned long nthreads_var = std::thread::hardware_concurrency ();
> + unsigned long nthreads_var = 0;
> +
> +#ifdef _GLIBCXX_HAS_GTHREADS
> + nthreads_var = std::thread::hardware_concurrency ();
> +#endif
_GLIBCXX_HAS_GTHREADS is a libstdc++ internal macro, it shouldn't be used
outside of libstdc++.
And, when using some other compiler or standard C++ library, nthreads_var
will be always 0. That isn't an improvement.
Jakub
[PATCH] Use hardware_concurrency only if _GLIBCXX_HAS_GTHREADS
Hello.
There's a patch attempt for the problem with std::thread::hardware_concurrency
where
it's used only if _GLIBCXX_HAS_GTHREADS is set.
Does it help?
Thanks,
Martin
gcc/ChangeLog:
PR bootstrap/100186
* lto-wrapper.c: Use hardware_concurrency only if
_GLIBCXX_HAS_GTHREADS.
---
gcc/lto-wrapper.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/gcc/lto-wrapper.c b/gcc/lto-wrapper.c
index 6ba401007f6..8a85b3e93a8 100644
--- a/gcc/lto-wrapper.c
+++ b/gcc/lto-wrapper.c
@@ -1285,7 +1285,11 @@ run_gcc (unsigned argc, char *argv[])
static char current_dir[] = { '.', DIR_SEPARATOR, '\0' };
/* Number of CPUs that can be used for parallel LTRANS phase. */
- unsigned long nthreads_var = std::thread::hardware_concurrency ();
+ unsigned long nthreads_var = 0;
+
+#ifdef _GLIBCXX_HAS_GTHREADS
+ nthreads_var = std::thread::hardware_concurrency ();
+#endif
/* Get the driver and options. */
collect_gcc = getenv ("COLLECT_GCC");
--
2.31.1
