[PATCH 00/03] JIT refactoring

[PATCH 00/03] JIT refactoring

[David Malcolm]

Various cleanups of the jit code

Successfully bootstrapped & regrtested on x86_64-unknown-linux-gnu
(Fedora 20).

OK for trunk?
(am not yet officially blessed as the JIT maintainer, so I require
review for the non-obvious patches)

[PATCH 01/03] Move gcc_jit_result implementation to a new files jit-result.{h|c}

[David Malcolm]

Whilst investigating another issue I found I had trouble locating
the gcc::jit::result class.  It turned out to be declared in
jit-recording.h, but implemented in jit-playback.c.

Move it to new files: jit-result.{h|c}

gcc/jit/ChangeLog:
	* Make-lang.in (jit_OBJS): Add jit/jit-result.o.
	* jit-playback.c: Include new header jit-result.h.
	(gcc::jit::result::result): Move to new file jit-result.c.
	(gcc::jit::result::~result): Likewise.
	(gcc::jit::playback::result): Likewise.
	* jit-recording.h (class gcc::jit::result): Move to new
	header jit-result.h.
	* jit-result.c: New file, to contain...
	(gcc::jit::result::result): Move here from jit-playback.c,
	removing erroneous "playback" namespace from comment.
	(gcc::jit::result::~result): Likewise.
	(gcc::jit::playback::result): Likewise.
	* jit-result.h: New file, to contain...
	(class gcc::jit::result): Move from jit-recording.h.
	* libgccjit.c: Include jit-result.h.
	(gcc_jit_result_get_code): Update comment to reflect move
	of implementation.
	(gcc_jit_result_release): Likewise.
---
 gcc/jit/Make-lang.in    |  1 +
 gcc/jit/jit-playback.c  | 43 +----------------------------
 gcc/jit/jit-recording.h | 15 ----------
 gcc/jit/jit-result.c    | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
 gcc/jit/jit-result.h    | 47 +++++++++++++++++++++++++++++++
 gcc/jit/libgccjit.c     |  6 ++--
 6 files changed, 125 insertions(+), 60 deletions(-)
 create mode 100644 gcc/jit/jit-result.c
 create mode 100644 gcc/jit/jit-result.h

diff --git a/gcc/jit/Make-lang.in b/gcc/jit/Make-lang.in
index 167fcad..e88fd00 100644
--- a/gcc/jit/Make-lang.in
+++ b/gcc/jit/Make-lang.in
@@ -64,6 +64,7 @@ jit_OBJS = attribs.o \
 	jit/libgccjit.o \
 	jit/jit-recording.o \
 	jit/jit-playback.o \
+	jit/jit-result.o \
 	jit/jit-builtins.o
 
 # Use strict warnings for this front end.
diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index 584a8e6..cd124eb 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -49,6 +49,7 @@ along with GCC; see the file COPYING3.  If not see
 
 #include "jit-common.h"
 #include "jit-playback.h"
+#include "jit-result.h"
 
 
 /* gcc::jit::playback::context::build_cast uses the convert.h API,
@@ -1951,48 +1952,6 @@ add_error_va (location *loc, const char *fmt, va_list ap)
 				  fmt, ap);
 }
 
-/* Constructor for gcc::jit::playback::result.  */
-
-result::
-result(void *dso_handle)
-  : m_dso_handle(dso_handle)
-{
-}
-
-/* gcc::jit::playback::result's destructor.
-
-   Called implicitly by gcc_jit_result_release.  */
-
-result::~result()
-{
-  dlclose (m_dso_handle);
-}
-
-/* Attempt to locate the given function by name within the
-   playback::result, using dlsym.
-
-   Implements the post-error-checking part of
-   gcc_jit_result_get_code.  */
-
-void *
-result::
-get_code (const char *funcname)
-{
-  void *code;
-  const char *error;
-
-  /* Clear any existing error.  */
-  dlerror ();
-
-  code = dlsym (m_dso_handle, funcname);
-
-  if ((error = dlerror()) != NULL)  {
-    fprintf(stderr, "%s\n", error);
-  }
-
-  return code;
-}
-
 /* Dealing with the linemap API.  */
 
 /* Construct a playback::location for a recording::location, if it
diff --git a/gcc/jit/jit-recording.h b/gcc/jit/jit-recording.h
index 9b2cfc6..1b0ef91 100644
--- a/gcc/jit/jit-recording.h
+++ b/gcc/jit/jit-recording.h
@@ -1576,21 +1576,6 @@ private:
 
 } // namespace gcc::jit::recording
 
-/* The result of JIT-compilation.  */
-class result
-{
-public:
-  result(void *dso_handle);
-
-  virtual ~result();
-
-  void *
-  get_code (const char *funcname);
-
-private:
-  void *m_dso_handle;
-};
-
 } // namespace gcc::jit
 
 } // namespace gcc
diff --git a/gcc/jit/jit-result.c b/gcc/jit/jit-result.c
new file mode 100644
index 0000000..9e1e6d8
--- /dev/null
+++ b/gcc/jit/jit-result.c
@@ -0,0 +1,73 @@
+/* Internals of libgccjit: implementation of gcc_jit_result
+   Copyright (C) 2013-2014 Free Software Foundation, Inc.
+   Contributed by David Malcolm <dmalcolm@redhat.com>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "jit-result.h"
+
+namespace gcc {
+namespace jit {
+
+/* Constructor for gcc::jit::result.  */
+
+result::
+result(void *dso_handle)
+  : m_dso_handle(dso_handle)
+{
+}
+
+/* gcc::jit::result's destructor.
+
+   Called implicitly by gcc_jit_result_release.  */
+
+result::~result()
+{
+  dlclose (m_dso_handle);
+}
+
+/* Attempt to locate the given function by name within the
+   playback::result, using dlsym.
+
+   Implements the post-error-checking part of
+   gcc_jit_result_get_code.  */
+
+void *
+result::
+get_code (const char *funcname)
+{
+  void *code;
+  const char *error;
+
+  /* Clear any existing error.  */
+  dlerror ();
+
+  code = dlsym (m_dso_handle, funcname);
+
+  if ((error = dlerror()) != NULL)  {
+    fprintf(stderr, "%s\n", error);
+  }
+
+  return code;
+}
+
+} // namespace gcc::jit
+
+} // namespace gcc
diff --git a/gcc/jit/jit-result.h b/gcc/jit/jit-result.h
new file mode 100644
index 0000000..60d6930
--- /dev/null
+++ b/gcc/jit/jit-result.h
@@ -0,0 +1,47 @@
+/* Internals of libgccjit: implementation of gcc_jit_result
+   Copyright (C) 2013-2014 Free Software Foundation, Inc.
+   Contributed by David Malcolm <dmalcolm@redhat.com>.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it
+under the terms of the GNU General Public License as published by
+the Free Software Foundation; either version 3, or (at your option)
+any later version.
+
+GCC is distributed in the hope that it will be useful, but
+WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3.  If not see
+<http://www.gnu.org/licenses/>.  */
+
+#ifndef JIT_RESULT_H
+#define JIT_RESULT_H
+
+namespace gcc {
+
+namespace jit {
+
+/* The result of JIT-compilation.  */
+class result
+{
+public:
+  result(void *dso_handle);
+
+  virtual ~result();
+
+  void *
+  get_code (const char *funcname);
+
+private:
+  void *m_dso_handle;
+};
+
+} // namespace gcc::jit
+
+} // namespace gcc
+
+#endif /* JIT_RESULT_H */
diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
index 7bc9209..42769e8 100644
--- a/gcc/jit/libgccjit.c
+++ b/gcc/jit/libgccjit.c
@@ -27,6 +27,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "libgccjit.h"
 #include "jit-common.h"
 #include "jit-recording.h"
+#include "jit-result.h"
 
 /* The opaque types used by the public API are actually subclasses
    of the gcc::jit::recording classes.  */
@@ -2047,8 +2048,7 @@ gcc_jit_context_get_first_error (gcc_jit_context *ctxt)
 /* Public entrypoint.  See description in libgccjit.h.
 
    After error-checking, the real work is done by the
-   gcc::jit::playback::result::get_code method in
-   jit-playback.c.  */
+   gcc::jit::result::get_code method in jit-result.c.  */
 
 void *
 gcc_jit_result_get_code (gcc_jit_result *result,
@@ -2063,7 +2063,7 @@ gcc_jit_result_get_code (gcc_jit_result *result,
 /* Public entrypoint.  See description in libgccjit.h.
 
    After error-checking, this is essentially a wrapper around the
-   destructor for gcc::jit::playback::result in jit-playback.c.  */
+   destructor for gcc::jit::result in jit-result.c.  */
 
 void
 gcc_jit_result_release (gcc_jit_result *result)
-- 
1.8.5.3

[PATCH 02/03] jit-playback: Move argv-creation to its own function

[David Malcolm]

The body of playback::context::compile is rather long, at 200 lines.

Start splitting it out parts of it into smaller member functions.

Also, eliminate a fixed-size buffer with bounds check in favor of an
auto_vec<>.

gcc/jit/ChangeLog:
	* jit-playback.c (gcc::jit::playback::context::compile): Use an
	auto_vec<const char *> rather than a const char *[20] for the
	top-level argv, and move the logic to build it to...
	(gcc::jit::playback::context::make_fake_args): New function.
	* jit-playback.h (gcc::jit::playback::context::make_fake_args):
	New function.
---
 gcc/jit/jit-playback.c | 149 ++++++++++++++++++++++++++-----------------------
 gcc/jit/jit-playback.h |   8 +++
 2 files changed, 86 insertions(+), 71 deletions(-)

diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index cd124eb..b12584e 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -1548,8 +1548,6 @@ compile ()
   void *handle = NULL;
   const char *ctxt_progname;
   result *result_obj = NULL;
-  const char *fake_args[20];
-  unsigned int num_args;
 
   m_path_template = make_tempdir_path_template ();
   if (!m_path_template)
@@ -1576,77 +1574,14 @@ compile ()
   if (!ctxt_progname)
     ctxt_progname = "libgccjit.so";
 
-  fake_args[0] = ctxt_progname;
-  fake_args[1] = m_path_c_file;
-  num_args = 2;
-
-#define ADD_ARG(arg) \
-  do \
-    { \
-      gcc_assert(num_args < sizeof(fake_args)/sizeof(char*)); \
-      fake_args[num_args++] = arg; \
-    } \
-  while (0)
-
-  ADD_ARG ("-fPIC");
-
-  /* Handle int options: */
-  switch (get_int_option (GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL))
-    {
-    default:
-      add_error (NULL,
-		 "unrecognized optimization level: %i",
-		 get_int_option (GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL));
-      return NULL;
-
-    case 0:
-      ADD_ARG ("-O0");
-      break;
-
-    case 1:
-      ADD_ARG ("-O1");
-      break;
-
-    case 2:
-      ADD_ARG ("-O2");
-      break;
-
-    case 3:
-      ADD_ARG ("-O3");
-      break;
-    }
-  /* What about -Os? */
-
-  /* Handle bool options: */
-  if (get_bool_option (GCC_JIT_BOOL_OPTION_DEBUGINFO))
-    ADD_ARG ("-g");
-
-  /* Suppress timing (and other) info.  */
-  if (!get_bool_option (GCC_JIT_BOOL_OPTION_DUMP_SUMMARY))
-    {
-      ADD_ARG ("-quiet");
-      quiet_flag = 1;
-    }
-
-  /* Aggressively garbage-collect, to shake out bugs: */
-  if (get_bool_option (GCC_JIT_BOOL_OPTION_SELFCHECK_GC))
-    {
-      ADD_ARG ("--param");
-      ADD_ARG ("ggc-min-expand=0");
-      ADD_ARG ("--param");
-      ADD_ARG ("ggc-min-heapsize=0");
-    }
-
-  if (get_bool_option (GCC_JIT_BOOL_OPTION_DUMP_EVERYTHING))
-    {
-      ADD_ARG ("-fdump-tree-all");
-      ADD_ARG ("-fdump-rtl-all");
-      ADD_ARG ("-fdump-ipa-all");
-    }
+  auto_vec <const char *> fake_args;
+  make_fake_args (&fake_args, ctxt_progname);
+  if (errors_occurred ())
+    return NULL;
 
   toplev toplev (false);
-
-  toplev.main (num_args, const_cast <char **> (fake_args));
+  toplev.main (fake_args.length (),
+	       const_cast <char **> (fake_args.address ()));
   toplev.finalize ();
 
   active_playback_ctxt = NULL;
@@ -1750,6 +1685,78 @@ compile ()
   return result_obj;
 }
 
+/* Helper functions for gcc::jit::playback::context::compile.  */
+
+/* Build a fake argv for toplev::main from the options set
+   by the user on the context .  */
+
+void
+playback::context::
+make_fake_args (auto_vec <const char *> *argvec,
+		const char *ctxt_progname)
+{
+#define ADD_ARG(arg) argvec->safe_push (arg)
+
+  ADD_ARG (ctxt_progname);
+  ADD_ARG (m_path_c_file);
+  ADD_ARG ("-fPIC");
+
+  /* Handle int options: */
+  switch (get_int_option (GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL))
+    {
+    default:
+      add_error (NULL,
+		 "unrecognized optimization level: %i",
+		 get_int_option (GCC_JIT_INT_OPTION_OPTIMIZATION_LEVEL));
+      return;
+
+    case 0:
+      ADD_ARG ("-O0");
+      break;
+
+    case 1:
+      ADD_ARG ("-O1");
+      break;
+
+    case 2:
+      ADD_ARG ("-O2");
+      break;
+
+    case 3:
+      ADD_ARG ("-O3");
+      break;
+    }
+  /* What about -Os? */
+
+  /* Handle bool options: */
+  if (get_bool_option (GCC_JIT_BOOL_OPTION_DEBUGINFO))
+    ADD_ARG ("-g");
+
+  /* Suppress timing (and other) info.  */
+  if (!get_bool_option (GCC_JIT_BOOL_OPTION_DUMP_SUMMARY))
+    {
+      ADD_ARG ("-quiet");
+      quiet_flag = 1;
+    }
+
+  /* Aggressively garbage-collect, to shake out bugs: */
+  if (get_bool_option (GCC_JIT_BOOL_OPTION_SELFCHECK_GC))
+    {
+      ADD_ARG ("--param");
+      ADD_ARG ("ggc-min-expand=0");
+      ADD_ARG ("--param");
+      ADD_ARG ("ggc-min-heapsize=0");
+    }
+
+  if (get_bool_option (GCC_JIT_BOOL_OPTION_DUMP_EVERYTHING))
+    {
+      ADD_ARG ("-fdump-tree-all");
+      ADD_ARG ("-fdump-rtl-all");
+      ADD_ARG ("-fdump-ipa-all");
+    }
+#undef ADD_ARG
+}
+
 /* Top-level hook for playing back a recording context.
 
    This plays back m_recording_ctxt, and, if no errors
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index 30e9229..5ffc869 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -227,6 +227,14 @@ private:
   void handle_locations ();
 
 private:
+
+  /* Functions for implementing "compile".  */
+
+  void
+  make_fake_args (auto_vec <const char *> *argvec,
+		  const char *ctxt_progname);
+
+private:
   ::gcc::jit::recording::context *m_recording_ctxt;
 
   /* Allocated using xmalloc (by xstrdup).  */
-- 
1.8.5.3

[PATCH 03/03] jit-playback: Move dso-creation into its own function

[David Malcolm]

Continue to refactor playback::context::compile, moving the DSO-generation
into its own function.

gcc/jit/ChangeLog:
	* jit-playback.c (gcc::jit::playback::context::compile): Move DSO
	creation code into...
	(gcc::jit::playback::context::convert_to_dso): New function.
	* jit-playback.h (gcc::jit::playback::context::convert_to_dso):
	New function.
---
 gcc/jit/jit-playback.c | 150 ++++++++++++++++++++++++++-----------------------
 gcc/jit/jit-playback.h |   3 +
 2 files changed, 82 insertions(+), 71 deletions(-)

diff --git a/gcc/jit/jit-playback.c b/gcc/jit/jit-playback.c
index b12584e..d16b3c4 100644
--- a/gcc/jit/jit-playback.c
+++ b/gcc/jit/jit-playback.c
@@ -1590,78 +1590,11 @@ compile ()
     return NULL;
 
   if (get_bool_option (GCC_JIT_BOOL_OPTION_DUMP_GENERATED_CODE))
-   dump_generated_code ();
-
-  /* Gross hacks follow:
-     We have a .s file; we want a .so file.
-     We could reuse parts of gcc/gcc.c to do this.
-     For now, just use the driver binary from the install, as
-     named in gcc-driver-name.h
-     e.g. "x86_64-unknown-linux-gnu-gcc-5.0.0".
-   */
-  {
-    auto_timevar assemble_timevar (TV_ASSEMBLE);
-    const char *errmsg;
-    const char *argv[7];
-    int exit_status = 0;
-    int err = 0;
-    const char *gcc_driver_name = GCC_DRIVER_NAME;
-
-    argv[0] = gcc_driver_name;
-    argv[1] = "-shared";
-    /* The input: assembler.  */
-    argv[2] = m_path_s_file;
-    /* The output: shared library.  */
-    argv[3] = "-o";
-    argv[4] = m_path_so_file;
-
-    /* Don't use the linker plugin.
-       If running with just a "make" and not a "make install", then we'd
-       run into
-          "fatal error: -fuse-linker-plugin, but liblto_plugin.so not found"
-       libto_plugin is a .la at build time, with it becoming installed with
-       ".so" suffix: i.e. it doesn't exist with a .so suffix until install
-       time.  */
-    argv[5] = "-fno-use-linker-plugin";
-
-    /* pex argv arrays are NULL-terminated.  */
-    argv[6] = NULL;
-
-    /* pex_one's error-handling requires pname to be non-NULL.  */
-    gcc_assert (ctxt_progname);
-
-    errmsg = pex_one (PEX_SEARCH, /* int flags, */
-		      gcc_driver_name,
-		      const_cast<char * const *> (argv),
-		      ctxt_progname, /* const char *pname */
-		      NULL, /* const char *outname */
-		      NULL, /* const char *errname */
-		      &exit_status, /* int *status */
-		      &err); /* int *err*/
-    if (errmsg)
-      {
-	add_error (NULL, "error invoking gcc driver: %s", errmsg);
-	return NULL;
-      }
+    dump_generated_code ();
 
-    /* pex_one can return a NULL errmsg when the executable wasn't
-       found (or doesn't exist), so trap these cases also.  */
-    if (exit_status || err)
-      {
-	add_error (NULL,
-		   "error invoking gcc driver: exit_status: %i err: %i",
-		   exit_status, err);
-	add_error (NULL,
-		   "whilst attempting to run a driver named: %s",
-		   gcc_driver_name);
-	add_error (NULL,
-		   "PATH was: %s",
-		   getenv ("PATH"));
-	return NULL;
-      }
-  }
-
-  // TODO: split out assembles vs linker
+  convert_to_dso (ctxt_progname);
+  if (errors_occurred ())
+    return NULL;
 
   /* dlopen the .so file. */
   {
@@ -1757,6 +1690,81 @@ make_fake_args (auto_vec <const char *> *argvec,
 #undef ADD_ARG
 }
 
+/* Part of playback::context::compile ().
+
+   We have a .s file; we want a .so file.
+   We could reuse parts of gcc/gcc.c to do this.
+   For now, just use the driver binary from the install, as
+   named in gcc-driver-name.h
+   e.g. "x86_64-unknown-linux-gnu-gcc-5.0.0".  */
+
+void
+playback::context::
+convert_to_dso (const char *ctxt_progname)
+{
+  /* Currently this lumps together both assembling and linking into
+     TV_ASSEMBLE.  */
+  auto_timevar assemble_timevar (TV_ASSEMBLE);
+  const char *errmsg;
+  const char *argv[7];
+  int exit_status = 0;
+  int err = 0;
+  const char *gcc_driver_name = GCC_DRIVER_NAME;
+
+  argv[0] = gcc_driver_name;
+  argv[1] = "-shared";
+  /* The input: assembler.  */
+  argv[2] = m_path_s_file;
+  /* The output: shared library.  */
+  argv[3] = "-o";
+  argv[4] = m_path_so_file;
+
+  /* Don't use the linker plugin.
+     If running with just a "make" and not a "make install", then we'd
+     run into
+       "fatal error: -fuse-linker-plugin, but liblto_plugin.so not found"
+     libto_plugin is a .la at build time, with it becoming installed with
+     ".so" suffix: i.e. it doesn't exist with a .so suffix until install
+     time.  */
+  argv[5] = "-fno-use-linker-plugin";
+
+  /* pex argv arrays are NULL-terminated.  */
+  argv[6] = NULL;
+
+  /* pex_one's error-handling requires pname to be non-NULL.  */
+  gcc_assert (ctxt_progname);
+
+  errmsg = pex_one (PEX_SEARCH, /* int flags, */
+		    gcc_driver_name,
+		    const_cast<char * const *> (argv),
+		    ctxt_progname, /* const char *pname */
+		    NULL, /* const char *outname */
+		    NULL, /* const char *errname */
+		    &exit_status, /* int *status */
+		    &err); /* int *err*/
+  if (errmsg)
+    {
+      add_error (NULL, "error invoking gcc driver: %s", errmsg);
+      return;
+    }
+
+  /* pex_one can return a NULL errmsg when the executable wasn't
+     found (or doesn't exist), so trap these cases also.  */
+  if (exit_status || err)
+    {
+      add_error (NULL,
+		 "error invoking gcc driver: exit_status: %i err: %i",
+		 exit_status, err);
+      add_error (NULL,
+		 "whilst attempting to run a driver named: %s",
+		 gcc_driver_name);
+      add_error (NULL,
+		 "PATH was: %s",
+		 getenv ("PATH"));
+      return;
+    }
+}
+
 /* Top-level hook for playing back a recording context.
 
    This plays back m_recording_ctxt, and, if no errors
diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
index 5ffc869..25a4c28 100644
--- a/gcc/jit/jit-playback.h
+++ b/gcc/jit/jit-playback.h
@@ -234,6 +234,9 @@ private:
   make_fake_args (auto_vec <const char *> *argvec,
 		  const char *ctxt_progname);
 
+  void
+  convert_to_dso (const char *ctxt_progname);
+
 private:
   ::gcc::jit::recording::context *m_recording_ctxt;
 
-- 
1.8.5.3

Re: [PATCH 00/03] JIT refactoring

[Jeff Law]

On 11/25/14 18:45, David Malcolm wrote:
> Various cleanups of the jit code
>
> Successfully bootstrapped & regrtested on x86_64-unknown-linux-gnu
> (Fedora 20).
>
> OK for trunk?
> (am not yet officially blessed as the JIT maintainer, so I require
> review for the non-obvious patches)
You're now the official maintainer for the JIT bits.

jeff