[Bug libstdc++/33578] New: libstdc++ parallel mode broken on mingw32

[Bug libstdc++/33578] New: libstdc++ parallel mode broken on mingw32

[dannysmith at users dot sourceforge dot net]

/develop/svn/trunk/build/mingw32/libstdc++-v3/include/parallel/compatibility.h:
In function 'void __gnu_parallel::yield()':
/develop/svn/trunk/build/mingw32/libstdc++-v3/include/parallel/compatibility.h:331:
error: 'Sleep' was not declared in this scope
make[4]: *** [parallel_list.lo] Error 1
make[3]: *** [all-recursive] Error 1
make[2]: *** [all] Error 2
make[1]: *** [all-target-libstdc++-v3] Error 2
make: *** [all] Error 2

The 'obvious' fix is to just include <windows.h> for __MINGW32__ as well
as _MSC_VER to get the prototype for Sleep. However, I expect that would cause
many complaints because it would pollute the global namespace with all
the Win32api names.  These do cause problems in practice , especially when
porting non-Win32 apps to mingw32. The _GTHREAD_HIDE_W32API business in
gthr-win32.h was implemented precisely to avoid having to expose these
names in libstdc++.

>From a mingw point of view it would be better if __gnu_parallel::yield
could be hidden away either in a gthread_sched_yield function in libgcc
or in a libstdc++ object file.


Secondly, I don't think Sleep is actually the correct function to call
on NT4 and later

The  SwitchToThread() function which

"Causes the calling thread to yield execution to another thread that is
ready to run on the current processor. The operating system selects the
next thread to be executed."

may be better.

Danny


-- 
           Summary: libstdc++ parallel mode broken on mingw32
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: dannysmith at users dot sourceforge dot net
 GCC build triplet: i686-pc-mingw32
  GCC host triplet: i686-pc-mingw32
GCC target triplet: i686-pc-mingw32


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33578

[Bug libstdc++/33578] __gnu_parallel::yield broken on mingw32

[bkoz at gcc dot gnu dot org]

------- Comment #1 from bkoz at gcc dot gnu dot org  2007-10-09 22:24 -------

Ah yes, this one.

I believe there is controversy around this implementation point on linux as
well.

Here's some info from previous discussions between myself, Ulrich, and
Johannes.

Me:
Include <sched.h> for sched_yield in <parallel/compatibility.h>. Should this be
pthread_yield or other approaches? 

Uli:
Using sched_yield/pthread_yield is almost always wrong.  I'd have to
look at the specific code.  If you can summarize it I can easily answer
questions.

Johannes:
Well, it really helps for work-stealing algorithms, in particular if
they use more threads than there are processors available at the time,
which can change even during execution. It makes some decision based on
the current load on the machine, which requires some OS interaction.
That's the easiest and most robust way to do it.

It is probably time to resume this discussion.


-- 

bkoz at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |bkoz at gcc dot gnu dot org
                   |dot org                     |
             Status|UNCONFIRMED                 |ASSIGNED
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2007-10-09 22:24:54
               date|                            |
            Summary|libstdc++ parallel mode     |__gnu_parallel::yield broken
                   |broken on mingw32           |on mingw32


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33578

[Bug libstdc++/33578] __gnu_parallel::yield broken on mingw32

[bkoz at gcc dot gnu dot org]

------- Comment #2 from bkoz at gcc dot gnu dot org  2007-10-09 22:38 -------
Created an attachment (id=14332)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14332&action=view)
Consistent use of _WIN32 define


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33578

[Bug libstdc++/33578] __gnu_parallel::yield broken on mingw32

[bkoz at gcc dot gnu dot org]

------- Comment #3 from bkoz at gcc dot gnu dot org  2007-10-09 22:41 -------

Actually, I think that when I added sched.h, I forgot to look at the windows
stuff. Something seems off to me, WRT the current code. The includes don't
match the code...

Here's a patch to make the conditionals in the includes match the conditionals
in the code. I'm not quite sure how _MSC_VER fits into this, or SwitchToThread. 


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33578

[Bug libstdc++/33578] __gnu_parallel::yield broken on mingw32

[dannysmith at users dot sourceforge dot net]

------- Comment #4 from dannysmith at users dot sourceforge dot net  2007-10-09 23:52 -------
Created an attachment (id=14334)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14334&action=view)
mingw32 compatibility patch


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33578

[Bug libstdc++/33578] __gnu_parallel::yield broken on mingw32

[dannysmith at users dot sourceforge dot net]

------- Comment #5 from dannysmith at users dot sourceforge dot net  2007-10-09 23:54 -------
(In reply to comment #2)
> Created an attachment (id=14332)
 --> (http://gcc.gnu.org/bugzilla/attachment.cgi?id=14332&action=view) [edit]
> Consistent use of _WIN32 define
> 
Thats not quite right.  ___MINGW32__ defines _WIN32 but doesn't have an
intrin.h.  I believe that is for MSVC compiler specific intrinsics.  _MSC_VER
is a MSVC compiler builtin define.

Also, Cygwin  __may__ define _W32 (with -mwin32 switch) but still should use
its own implementation of <sched.h> and sched_yield  since it uses pthreads 


I'll attach what I'm using currently.  Putting more #ifdef's into this file
certainly doesn't make it any prettier. If wanted, I could prepare a patch that
gets rid of *all* the non-GNU (Windows) stuff and just put a __MINGW32__
specific definition of a __gnu_parallel::sched_yield() into os_defines.h. 

FWIW,  boost uses Sleep(1) -- not Sleep(0) --  for sched_yield() on windows,
while cygwin use SwitchToThread()


Dann\y


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33578

[Bug libstdc++/33578] __gnu_parallel::yield broken on mingw32

[bkoz at gcc dot gnu dot org]

------- Comment #6 from bkoz at gcc dot gnu dot org  2007-10-10 02:00 -------

Danny, if this allows compilation of parallel_list.cc then please check your
patch in, as it doesn't change semantics.

Then we can figure out what we really want here....


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33578

[Bug libstdc++/33578] __gnu_parallel::yield broken on mingw32

[dannysmith at gcc dot gnu dot org]

------- Comment #7 from dannysmith at gcc dot gnu dot org  2007-10-10 23:39 -------
Subject: Bug 33578

Author: dannysmith
Date: Wed Oct 10 23:39:30 2007
New Revision: 129219

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=129219
Log:
        PR libstdc++/33578
        * include/parallel/compatibility.h.  Use POSIX sched_yield on
        __CYGWIN__ 
        (Sleep): Add prototype for __MINGW32__.


Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/parallel/compatibility.h


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33578

[Bug libstdc++/33578] __gnu_parallel::yield means what?

[bkoz at gcc dot gnu dot org]

------- Comment #8 from bkoz at gcc dot gnu dot org  2007-10-16 17:24 -------

Changing this summary now that the mingw32 issue is fixed. Now, opening this
out to the implementation issues... 


-- 

bkoz at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   GCC host triplet|i686-pc-mingw32             |all
 GCC target triplet|i686-pc-mingw32             |all
            Summary|__gnu_parallel::yield broken|__gnu_parallel::yield means
                   |on mingw32                  |what?


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33578

[Bug libstdc++/33578] __gnu_parallel::yield means what?

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=33578

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
For std::this_thread::yield() we do:

    inline void
    yield() noexcept
    {
#if defined _GLIBCXX_HAS_GTHREADS && defined _GLIBCXX_USE_SCHED_YIELD
      __gthread_yield();
#endif
    }

And gthr-win32.h has:

__GTHREAD_WIN32_INLINE int
__gthread_yield (void)
{
  Sleep (0);
  return 0;
}

For atomics we also support using the x86 pause instruction

    inline void
    __thread_yield() noexcept
    {
#if defined _GLIBCXX_HAS_GTHREADS && defined _GLIBCXX_USE_SCHED_YIELD
     __gthread_yield();
#endif
    }

    inline void
    __thread_relax() noexcept
    {
#if defined __i386__ || defined __x86_64__
      __builtin_ia32_pause();
#else
      __thread_yield();
#endif
    }



For Parallel Mode maybe we should just use __gthread_yield() everywhere?
gthr-win32.h already takes care of hiding the Win32 API. Since Parallel Mode is
likely to get removed at some point (now that we have the parallel STL algos
from C++17) I don't see any point in revisiting its implementation w.r.t using
of yield at all (comment 1).

tl;dr let's not reinvent the wheel when we already have portable code for
yielding, and don't want to redesign the parallel mode.

[Bug libstdc++/33578] __gnu_parallel::yield means what?

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=33578

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
           Assignee|bkoz at gcc dot gnu.org            |unassigned at gcc dot gnu.org

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Unassigning since Benjamin since not been active in GCC development for over 8
years now.