Re: why does gccgit require pthread?

2022-11-14 Thread LIU Hao via Gcc-patches 

在 2022/11/12 02:27, Jonathan Wakely 写道:


A clean build fixed that. This patch bootstraps and passes testing on
x86_64-pc-linux-gnu (CentOS 8 Stream).

OK for trunk?


What should we do if no one has been approving this patch?


--
Best regards,
LIU Hao



OpenPGP_signature
Description: OpenPGP digital signature

Re: why does gccgit require pthread?

2022-11-11 Thread Jonathan Wakely via Gcc-patches 
On Fri, 11 Nov 2022 at 17:16, Jonathan Wakely wrote:
>
> On Mon, 7 Nov 2022 at 13:51, Jonathan Wakely wrote:
> >
> > On Mon, 7 Nov 2022 at 13:33, LIU Hao wrote:
> > >
> > > 在 2022-11-07 20:57, Jonathan Wakely 写道:
> > > > It would be a lot nicer if playback::context met the C++ Lockable
> > > > requirements, and playback::context::compile () could just take a
> > > > scoped lock on *this:
> > > >
> > > >
> > >
> > > Yeah yeah that makes a lot of sense. Would you please just commit that? I 
> > > don't have write access to
> > > GCC repo, and it takes a couple of hours for me to bootstrap GCC just for 
> > > this tiny change.
> >
> > Somebody else needs to approve it first. I'll combine our patches and
> > test and submit it properly for approval.
>
> Here's a complete patch that actually builds now, although I'm seeing
> a stage 2 vs stage 3 comparison error which I don't have time to look
> into right now.

A clean build fixed that. This patch bootstraps and passes testing on
x86_64-pc-linux-gnu (CentOS 8 Stream).

OK for trunk?

Re: why does gccgit require pthread?

2022-11-11 Thread Jonathan Wakely via Gcc-patches 
On Mon, 7 Nov 2022 at 13:51, Jonathan Wakely wrote:
>
> On Mon, 7 Nov 2022 at 13:33, LIU Hao wrote:
> >
> > 在 2022-11-07 20:57, Jonathan Wakely 写道:
> > > It would be a lot nicer if playback::context met the C++ Lockable
> > > requirements, and playback::context::compile () could just take a
> > > scoped lock on *this:
> > >
> > >
> >
> > Yeah yeah that makes a lot of sense. Would you please just commit that? I 
> > don't have write access to
> > GCC repo, and it takes a couple of hours for me to bootstrap GCC just for 
> > this tiny change.
>
> Somebody else needs to approve it first. I'll combine our patches and
> test and submit it properly for approval.

Here's a complete patch that actually builds now, although I'm seeing
a stage 2 vs stage 3 comparison error which I don't have time to look
into right now.
commit 5dde4bd09c4706617120a42c5953908ae39b5751
Author: Jonathan Wakely 
Date:   Fri Nov 11 12:48:29 2022

jit: Use std::mutex instead of pthread_mutex_t

This allows JIT to be built with a different thread model from posix
where pthread isn't available

By renaming the acquire_mutex () and release_mutex () member functions
to lock() and unlock() we make the playback::context type meet the C++
Lockable requirements. This allows it to be used with a scoped lock
(i.e. RAII) type as std::lock_guard. This automatically releases the
mutex when leaving the scope.

Co-authored-by: LIU Hao 

gcc/jit/ChangeLog:

* jit-playback.cc (playback::context::scoped_lock): Define RAII
lock type.
(playback::context::compile): Use scoped_lock to acquire mutex
for the active playback context.
(jit_mutex): Change to std::mutex.
(playback::context::acquire_mutex): Rename to ...
(playback::context::lock): ... this.
(playback::context::release_mutex): Rename to ...
(playback::context::unlock): ... this.
* jit-playback.h (playback::context): Rename members and declare
scoped_lock.
* jit-recording.cc (INCLUDE_PTHREAD_H): Remove unused define.
* libgccjit.cc (version_mutex): Change to std::mutex.
(struct jit_version_info): Use std::lock_guard to acquire and
release mutex.

gcc/ChangeLog:

* system.h [INCLUDE_MUTEX]: Include header for std::mutex.

diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index d227d36283a..bf006903a44 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -19,7 +19,7 @@ along with GCC; see the file COPYING3.  If not see
 .  */
 
 #include "config.h"
-#define INCLUDE_PTHREAD_H
+#define INCLUDE_MUTEX
 #include "system.h"
 #include "coretypes.h"
 #include "target.h"
@@ -2302,6 +2302,20 @@ block (function *func,
   m_label_expr = NULL;
 }
 
+// This is basically std::lock_guard but it can call the private lock/unlock
+// members of playback::context.
+struct playback::context::scoped_lock
+{
+  scoped_lock (context ) : m_ctx () { m_ctx->lock (); }
+  ~scoped_lock () { m_ctx->unlock (); }
+
+  context *m_ctx;
+
+  // Not movable or copyable.
+  scoped_lock (scoped_lock &&) = delete;
+  scoped_lock = (scoped_lock &&) = delete;
+};
+
 /* Compile a playback::context:
 
- Use the context's options to cconstruct command-line options, and
@@ -2353,15 +2367,12 @@ compile ()
   m_recording_ctxt->get_all_requested_dumps (_dumps);
 
   /* Acquire the JIT mutex and set "this" as the active playback ctxt.  */
-  acquire_mutex ();
+  scoped_lock lock(*this);
 
   auto_string_vec fake_args;
   make_fake_args (_args, ctxt_progname, _dumps);
   if (errors_occurred ())
-{
-  release_mutex ();
-  return;
-}
+return;
 
   /* This runs the compiler.  */
   toplev toplev (get_timer (), /* external_timer */
@@ -2388,10 +2399,7 @@ compile ()
  followup activities use timevars, which are global state.  */
 
   if (errors_occurred ())
-{
-  release_mutex ();
-  return;
-}
+return;
 
   if (get_bool_option (GCC_JIT_BOOL_OPTION_DUMP_GENERATED_CODE))
 dump_generated_code ();
@@ -2403,8 +2411,6 @@ compile ()
  convert the .s file to the requested output format, and copy it to a
  given file (playback::compile_to_file).  */
   postprocess (ctxt_progname);
-
-  release_mutex ();
 }
 
 /* Implementation of class gcc::jit::playback::compile_to_memory,
@@ -2662,18 +2668,18 @@ playback::compile_to_file::copy_file (const char 
*src_path,
 /* This mutex guards gcc::jit::recording::context::compile, so that only
one thread can be accessing the bulk of GCC's state at once.  */
 
-static pthread_mutex_t jit_mutex = PTHREAD_MUTEX_INITIALIZER;
+static std::mutex jit_mutex;
 
 /* Acquire jit_mutex and set "this" as the active playback ctxt.  */
 
 void
-playback::context::acquire_mutex ()
+playback::context::lock ()
 {
   auto_timevar tv (get_timer (),

Re: why does gccgit require pthread?

2022-11-07 Thread Jonathan Wakely via Gcc-patches 
On Mon, 7 Nov 2022 at 13:33, LIU Hao wrote:
>
> 在 2022-11-07 20:57, Jonathan Wakely 写道:
> > It would be a lot nicer if playback::context met the C++ Lockable
> > requirements, and playback::context::compile () could just take a
> > scoped lock on *this:
> >
> >
>
> Yeah yeah that makes a lot of sense. Would you please just commit that? I 
> don't have write access to
> GCC repo, and it takes a couple of hours for me to bootstrap GCC just for 
> this tiny change.

Somebody else needs to approve it first. I'll combine our patches and
test and submit it properly for approval.

Re: why does gccgit require pthread?

2022-11-07 Thread LIU Hao via Gcc-patches 

在 2022-11-07 20:57, Jonathan Wakely 写道:

It would be a lot nicer if playback::context met the C++ Lockable
requirements, and playback::context::compile () could just take a
scoped lock on *this:




Yeah yeah that makes a lot of sense. Would you please just commit that? I don't have write access to 
GCC repo, and it takes a couple of hours for me to bootstrap GCC just for this tiny change.



--
Best regards,
LIU Hao



OpenPGP_signature
Description: OpenPGP digital signature

Re: why does gccgit require pthread?

2022-11-07 Thread Jonathan Wakely via Gcc-patches 
On Mon, 7 Nov 2022 at 06:51, LIU Hao via Gcc  wrote:
>
> 在 2022-11-07 12:37, Andrew Pinski 写道:
> >
> > The original code which used pthread was added in GCC 5 way before GCC
> > moved to being written in C++11 which was only in the last 3 years.
> > pthread_* functions were the best choice at the time (2014) but now
> > GCC is written in C++11, I don't see any reason not to move them over
> > to using C++11 threading code.
> >
> >
>
> Attached is the proposed patch.

It would be a lot nicer if playback::context met the C++ Lockable
requirements, and playback::context::compile () could just take a
scoped lock on *this:


--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -2353,15 +2353,12 @@ compile ()
  m_recording_ctxt->get_all_requested_dumps (_dumps);

  /* Acquire the JIT mutex and set "this" as the active playback ctxt.  */
-  acquire_mutex ();
+  std::lock_guard lock(*this);

  auto_string_vec fake_args;
  make_fake_args (_args, ctxt_progname, _dumps);
  if (errors_occurred ())
-{
-  release_mutex ();
-  return;
-}
+return;

  /* This runs the compiler.  */
  toplev toplev (get_timer (), /* external_timer */
@@ -2388,10 +2385,7 @@ compile ()
 followup activities use timevars, which are global state.  */

  if (errors_occurred ())
-{
-  release_mutex ();
-  return;
-}
+return;

  if (get_bool_option (GCC_JIT_BOOL_OPTION_DUMP_GENERATED_CODE))
dump_generated_code ();
@@ -2403,8 +2397,6 @@ compile ()
 convert the .s file to the requested output format, and copy it to a
 given file (playback::compile_to_file).  */
  postprocess (ctxt_progname);
-
-  release_mutex ();
}

/* Implementation of class gcc::jit::playback::compile_to_memory,
@@ -2662,18 +2654,18 @@ playback::compile_to_file::copy_file (const
char *src_path,
/* This mutex guards gcc::jit::recording::context::compile, so that only
   one thread can be accessing the bulk of GCC's state at once.  */

-static pthread_mutex_t jit_mutex = PTHREAD_MUTEX_INITIALIZER;
+static std::mutex jit_mutex;

/* Acquire jit_mutex and set "this" as the active playback ctxt.  */

void
-playback::context::acquire_mutex ()
+playback::context::lock ()
{
  auto_timevar tv (get_timer (), TV_JIT_ACQUIRING_MUTEX);

  /* Acquire the big GCC mutex. */
  JIT_LOG_SCOPE (get_logger ());
-  pthread_mutex_lock (_mutex);
+  jit_mutex.lock();
  gcc_assert (active_playback_ctxt == NULL);
  active_playback_ctxt = this;
}
@@ -2681,13 +2673,13 @@ playback::context::acquire_mutex ()
/* Release jit_mutex and clear the active playback ctxt.  */

void
-playback::context::release_mutex ()
+playback::context::unlock ()
{
  /* Release the big GCC mutex. */
  JIT_LOG_SCOPE (get_logger ());
  gcc_assert (active_playback_ctxt == this);
  active_playback_ctxt = NULL;
-  pthread_mutex_unlock (_mutex);
+  jit_mutex.unlock();
}

/* Callback used by gcc::jit::playback::context::make_fake_args when
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index 3ba02a0451a..bd2bc389f53 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -314,8 +314,8 @@ private:

  /* Functions for implementing "compile".  */

-  void acquire_mutex ();
-  void release_mutex ();
+  void lock ();
+  void unlock ();

  void
  make_fake_args (vec  *argvec,

Re: why does gccgit require pthread?

2022-11-06 Thread LIU Hao via Gcc-patches 

在 2022/11/7 15:03, Andrew Pinski 写道:


The win32 thread model does not have `std::mutex`; but there is no 
`pthread_mutex_t` either, so it
does not build either way.

Oh, but I would assume it will later on right?



There has been effort on C++11 threading support for win32 thread model, but I have a negative 
attitude on that.


Another solution is to use `__gthread_mutex_t` instead of `pthread_mutex_t`, which is also available 
in the win32 thread model. Actually I prefer this approach as it keeps code structure like what we 
have at this moment.




Also I think you might need to change some more than you did.
That is:
-#define INCLUDE_PTHREAD_H
  #include "system.h"

You must likely have a macro, INCLUDE_MUTEX, and define that and
include mutex in system.h like it was done for pthread.h.
GCC loves to poison identifiers while lexing to make sure those
identifiers are not used inside GCC and the include of mutex should be
done early.



Well I am not familiar with such behavior. Feel free to amend the patch, until it looks good to you. 
I hope we can check this in before GCC 13 RC.




--
Best regards,
LIU Hao



OpenPGP_signature
Description: OpenPGP digital signature

Re: why does gccgit require pthread?

2022-11-06 Thread Andrew Pinski via Gcc-patches 
On Sun, Nov 6, 2022 at 10:51 PM LIU Hao  wrote:
>
> 在 2022-11-07 12:37, Andrew Pinski 写道:
> >
> > The original code which used pthread was added in GCC 5 way before GCC
> > moved to being written in C++11 which was only in the last 3 years.
> > pthread_* functions were the best choice at the time (2014) but now
> > GCC is written in C++11, I don't see any reason not to move them over
> > to using C++11 threading code.
> >
> >
>
> Attached is the proposed patch.
>
> The win32 thread model does not have `std::mutex`; but there is no 
> `pthread_mutex_t` either, so it
> does not build either way.
Oh, but I would assume it will later on right?

Also I think you might need to change some more than you did.
That is:
-#define INCLUDE_PTHREAD_H
 #include "system.h"

You must likely have a macro, INCLUDE_MUTEX, and define that and
include mutex in system.h like it was done for pthread.h.
GCC loves to poison identifiers while lexing to make sure those
identifiers are not used inside GCC and the include of mutex should be
done early.

Thanks,
Andrew

>
> Tested bootstrapping GCC on `{i686,x86_64}-w64-mingw32` with languages
> `c,lto,c++,fortran,objc,obj-c++` and with the `mcf` thread model; no errors 
> observed. The built
> `libgccjit-0.dll` does not have imports from winpthread any more.
>
> Please review.
>
>
> --
> Best regards,
> LIU Hao
>

Re: why does gccgit require pthread?

2022-11-06 Thread LIU Hao via Gcc-patches 

在 2022-11-07 12:37, Andrew Pinski 写道:


The original code which used pthread was added in GCC 5 way before GCC
moved to being written in C++11 which was only in the last 3 years.
pthread_* functions were the best choice at the time (2014) but now
GCC is written in C++11, I don't see any reason not to move them over
to using C++11 threading code.




Attached is the proposed patch.

The win32 thread model does not have `std::mutex`; but there is no `pthread_mutex_t` either, so it 
does not build either way.


Tested bootstrapping GCC on `{i686,x86_64}-w64-mingw32` with languages 
`c,lto,c++,fortran,objc,obj-c++` and with the `mcf` thread model; no errors observed. The built 
`libgccjit-0.dll` does not have imports from winpthread any more.


Please review.


--
Best regards,
LIU Hao

From ceb65f21b5ac23ce218efee82f40f641ebe44361 Mon Sep 17 00:00:00 2001
From: LIU Hao 
Date: Mon, 7 Nov 2022 13:00:12 +0800
Subject: [PATCH] gcc/jit: Use C++11 mutex instead of pthread's

This allows JIT to be built with a different thread model from `posix`
where pthread isn't available

gcc/jit/ChangeLog:

* jit-playback.cc: Use `std::mutex` instead of `pthread_mutex_t`
(playback::context::acquire_mutex): Likewise
(playback::context::release_mutex): Likewise
* jit-recording.cc: Remove the unused `INCLUDE_PTHREAD_H`
* libgccjit.cc: Use `std::mutex` instead of `pthread_mutex_t`
---
 gcc/jit/jit-playback.cc  | 9 +
 gcc/jit/jit-recording.cc | 1 -
 gcc/jit/libgccjit.cc | 8 
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
index d227d36283a..17ff98c149b 100644
--- a/gcc/jit/jit-playback.cc
+++ b/gcc/jit/jit-playback.cc
@@ -19,7 +19,6 @@ along with GCC; see the file COPYING3.  If not see
 .  */
 
 #include "config.h"
-#define INCLUDE_PTHREAD_H
 #include "system.h"
 #include "coretypes.h"
 #include "target.h"
@@ -51,6 +50,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "jit-w32.h"
 #endif
 
+#include 
+
 /* Compare with gcc/c-family/c-common.h: DECL_C_BIT_FIELD,
SET_DECL_C_BIT_FIELD.
These are redefined here to avoid depending from the C frontend.  */
@@ -2662,7 +2663,7 @@ playback::compile_to_file::copy_file (const char 
*src_path,
 /* This mutex guards gcc::jit::recording::context::compile, so that only
one thread can be accessing the bulk of GCC's state at once.  */
 
-static pthread_mutex_t jit_mutex = PTHREAD_MUTEX_INITIALIZER;
+static std::mutex jit_mutex;
 
 /* Acquire jit_mutex and set "this" as the active playback ctxt.  */
 
@@ -2673,7 +2674,7 @@ playback::context::acquire_mutex ()
 
   /* Acquire the big GCC mutex. */
   JIT_LOG_SCOPE (get_logger ());
-  pthread_mutex_lock (_mutex);
+  jit_mutex.lock ();
   gcc_assert (active_playback_ctxt == NULL);
   active_playback_ctxt = this;
 }
@@ -2687,7 +2688,7 @@ playback::context::release_mutex ()
   JIT_LOG_SCOPE (get_logger ());
   gcc_assert (active_playback_ctxt == this);
   active_playback_ctxt = NULL;
-  pthread_mutex_unlock (_mutex);
+  jit_mutex.unlock ();
 }
 
 /* Callback used by gcc::jit::playback::context::make_fake_args when
diff --git a/gcc/jit/jit-recording.cc b/gcc/jit/jit-recording.cc
index f78daed2d71..6ae5a667e90 100644
--- a/gcc/jit/jit-recording.cc
+++ b/gcc/jit/jit-recording.cc
@@ -19,7 +19,6 @@ along with GCC; see the file COPYING3.  If not see
 .  */
 
 #include "config.h"
-#define INCLUDE_PTHREAD_H
 #include "system.h"
 #include "coretypes.h"
 #include "tm.h"
diff --git a/gcc/jit/libgccjit.cc b/gcc/jit/libgccjit.cc
index ca862662777..a5105fbc1f9 100644
--- a/gcc/jit/libgccjit.cc
+++ b/gcc/jit/libgccjit.cc
@@ -19,7 +19,6 @@ along with GCC; see the file COPYING3.  If not see
 .  */
 
 #include "config.h"
-#define INCLUDE_PTHREAD_H
 #include "system.h"
 #include "coretypes.h"
 #include "timevar.h"
@@ -30,6 +29,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "jit-recording.h"
 #include "jit-result.h"
 
+#include 
+
 /* The opaque types used by the public API are actually subclasses
of the gcc::jit::recording classes.  */
 
@@ -4060,7 +4061,7 @@ gcc_jit_context_new_rvalue_from_vector (gcc_jit_context 
*ctxt,
Ideally this would be within parse_basever, but the mutex is only needed
by libgccjit.  */
 
-static pthread_mutex_t version_mutex = PTHREAD_MUTEX_INITIALIZER;
+static std::mutex version_mutex;
 
 struct jit_version_info
 {
@@ -4068,9 +4069,8 @@ struct jit_version_info
  guarded by version_mutex.  */
   jit_version_info ()
   {
-pthread_mutex_lock (_mutex);
+std::lock_guard g (version_mutex);
 parse_basever (, , );
-pthread_mutex_unlock (_mutex);
   }
 
   int major;
-- 
2.38.1



OpenPGP_signature
Description: OpenPGP digital signature