On Thu, 2023-11-23 at 17:17 -0500, Antoni Boucher wrote: > Hi. > I did split the patch and sent one for the bfloat16 support and > another > one for the vector support. > > Here's the updated patch for the machine-dependent builtins. >
Thanks for the patch; sorry about the long delay in reviewing it. CCing Jan and Uros re the i386 part of that patch; for reference the patch being discussed is here: https://gcc.gnu.org/pipermail/gcc-patches/2023-November/638027.html > From e025f95f4790ae861e709caf23cbc0723c1a3804 Mon Sep 17 00:00:00 2001 > From: Antoni Boucher <boua...@zoho.com> > Date: Mon, 23 Jan 2023 17:21:15 -0500 > Subject: [PATCH] libgccjit: Add support for machine-dependent builtins [...snip...] > diff --git a/gcc/config/i386/i386-builtins.cc > b/gcc/config/i386/i386-builtins.cc > index 42fc3751676..5cc1d6f4d2e 100644 > --- a/gcc/config/i386/i386-builtins.cc > +++ b/gcc/config/i386/i386-builtins.cc > @@ -225,6 +225,22 @@ static GTY(()) tree ix86_builtins[(int) > IX86_BUILTIN_MAX]; > > struct builtin_isa ix86_builtins_isa[(int) IX86_BUILTIN_MAX]; > > +static void > +clear_builtin_types (void) > +{ > + for (int i = 0 ; i < IX86_BT_LAST_CPTR + 1 ; i++) > + ix86_builtin_type_tab[i] = NULL; > + > + for (int i = 0 ; i < IX86_BUILTIN_MAX ; i++) > + { > + ix86_builtins[i] = NULL; > + ix86_builtins_isa[i].set_and_not_built_p = true; > + } > + > + for (int i = 0 ; i < IX86_BT_LAST_ALIAS + 1 ; i++) > + ix86_builtin_func_type_tab[i] = NULL; > +} > + > tree get_ix86_builtin (enum ix86_builtins c) > { > return ix86_builtins[c]; > @@ -1483,6 +1499,8 @@ ix86_init_builtins (void) > { > tree ftype, decl; > > + clear_builtin_types (); > + > ix86_init_builtin_types (); > > /* Builtins to get CPU type and features. */ Please can one of the i386 maintainers check this? (CCing Jan and Uros: this is for the case where the compiler code runs multiple times in-process due to being linked into libgccjit.so. We want to restore state within i386-builtins.cc to an initial state, and ensure that no GC-managed objects persist from previous in-memory compiles). > diff --git a/gcc/jit/docs/topics/compatibility.rst b/gcc/jit/docs/topics/compatibility.rst > index ebede440ee4..764de23341e 100644 > --- a/gcc/jit/docs/topics/compatibility.rst > +++ b/gcc/jit/docs/topics/compatibility.rst > @@ -378,3 +378,12 @@ alignment of a variable: > -------------------- > ``LIBGCCJIT_ABI_25`` covers the addition of > :func:`gcc_jit_type_get_restrict` > + > +.. _LIBGCCJIT_ABI_26: > + > +``LIBGCCJIT_ABI_26`` > +-------------------- > + > +``LIBGCCJIT_ABI_26`` covers the addition of a function to get target > builtins: > + > + * :func:`gcc_jit_context_get_target_builtin_function` > diff --git a/gcc/jit/docs/topics/functions.rst > b/gcc/jit/docs/topics/functions.rst > index cf5cb716daf..e9b77fdb892 100644 > --- a/gcc/jit/docs/topics/functions.rst > +++ b/gcc/jit/docs/topics/functions.rst > @@ -140,6 +140,25 @@ Functions > uses such a parameter will lead to an error being emitted within > the context. > > +.. function:: gcc_jit_function *\ > + gcc_jit_context_get_target_builtin_function (gcc_jit_context > *ctxt,\ > + const char *name) > + > + Get the :type:`gcc_jit_function` for the built-in function with the > + given name. For example: Might be nice to add the "(sometimes called intrinsic functions)" text you have in the header here. [...snip....] > diff --git a/gcc/jit/dummy-frontend.cc b/gcc/jit/dummy-frontend.cc > index a729086bafb..3ca9702d429 100644 > --- a/gcc/jit/dummy-frontend.cc > +++ b/gcc/jit/dummy-frontend.cc [...] > @@ -29,8 +30,14 @@ along with GCC; see the file COPYING3. If not see > #include "options.h" > #include "stringpool.h" > #include "attribs.h" > +#include "jit-recording.h" > +#include "print-tree.h" > > #include <mpfr.h> > +#include <unordered_map> > +#include <string> > + > +using namespace gcc::jit; > > /* Attribute handling. */ > > @@ -86,6 +93,11 @@ static const struct attribute_spec::exclusions > attr_const_pure_exclusions[] = > ATTR_EXCL (NULL, false, false, false) > }; > > +hash_map<nofree_string_hash, tree> target_builtins{}; I was wondering if this needs a GTY marker, but I don't think it does: presumably it's only used within jit_langhook_parse_file where no GC can happen - unless jit_langhook_write_globals makes use of it? > +std::unordered_map<std::string, recording::function_type*> target_function_types > +{}; > +recording::context target_builtins_ctxt{NULL}; Please add a comment to target_builtins_ctxt saying what it's for. As far as I can tell, it's for getting at recording::types from tree_type_to_jit_type; we then use a new "copy" mechanism to copy objects from target_builtins_ctxt for use with the real recording::context. This feels ugly, but maybe it's the only way to make it work. Could tree_type_to_jit_type take a recording::context as a param? The only non-recursive uses of tree_type_to_jit_type seem to be in jit_langhook_builtin_function. Could that use the recording::context of the current playback::context? You can get the current playback::context from gcc::jit::active_playback_ctxt and then access that playback::context's m_recording_ctxt. Or is there some need to have this work as a global cache? (which is what the target_builtins_ctxt effectively does). It looks like target_function_types might be a cache... > + > /* Table of machine-independent attributes supported in libgccjit. */ > const struct attribute_spec jit_attribute_table[] = > { > @@ -594,6 +606,7 @@ jit_langhook_init (void) > > build_common_tree_nodes (false); > > + target_builtins.empty (); > build_common_builtin_nodes (); > > /* The default precision for floating point numbers. This is used > @@ -601,6 +614,8 @@ jit_langhook_init (void) > eventually be controllable by a command line option. */ > mpfr_set_default_prec (256); > > + targetm.init_builtins (); > + > return true; > } > > @@ -668,11 +683,198 @@ jit_langhook_type_for_mode (machine_mode mode, int > unsignedp) > return NULL; > } > > -/* Record a builtin function. We just ignore builtin functions. */ > +recording::type* tree_type_to_jit_type (tree type) > +{ > + if (TREE_CODE (type) == VECTOR_TYPE) > + { > + tree inner_type = TREE_TYPE (type); > + recording::type* element_type = tree_type_to_jit_type (inner_type); > + poly_uint64 size = TYPE_VECTOR_SUBPARTS (type); > + long constant_size = size.to_constant (); > + if (element_type != NULL) > + return element_type->get_vector (constant_size); > + return NULL; > + } > + if (TREE_CODE (type) == REFERENCE_TYPE) > + // For __builtin_ms_va_start. > + // FIXME: wrong type. > + return new recording::memento_of_get_type (&target_builtins_ctxt, > + GCC_JIT_TYPE_VOID); The various "// FIXME: wrong type" cases in this function feel like a timebomb; could we instead fail hard on them, rather than potentially silently generate bad code? [...snip...] > +/* Record a builtin function. We save their types to be able to check types > + in recording and for reflection. */ Aha! This comment makes it clearer that the stuff above is a cache, so maybe it has to be done the way you have it above (with the recursive copying to the appropriate recording::context). > static tree > jit_langhook_builtin_function (tree decl) > { > + if (TREE_CODE (decl) == FUNCTION_DECL) > + { > + const char* name = IDENTIFIER_POINTER (DECL_NAME (decl)); > + target_builtins.put (name, decl); > + > + std::string string_name (name); > + if (target_function_types.count (string_name) == 0) > + { > + tree function_type = TREE_TYPE (decl); > + tree arg = TYPE_ARG_TYPES (function_type); > + bool is_variadic = false; > + > + auto_vec <recording::type *> param_types; > + > + while (arg != void_list_node) > + { > + if (arg == NULL) > + { > + is_variadic = true; > + break; > + } > + if (arg != void_list_node) > + { > + recording::type* arg_type = tree_type_to_jit_type (TREE_VALUE (arg)); > + if (arg_type == NULL) > + return decl; > + param_types.safe_push (arg_type); > + } > + arg = TREE_CHAIN (arg); > + } > + > + tree result_type = TREE_TYPE (function_type); > + recording::type* return_type = tree_type_to_jit_type (result_type); > + > + if (return_type == NULL) > + return decl; > + > + recording::function_type* func_type = > + new recording::function_type (&target_builtins_ctxt, return_type, > + param_types.length (), > + param_types.address (), is_variadic, > + false); > + > + target_function_types[string_name] = func_type; > + } > + } > return decl; > } > [...snip...] > diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc > index 18cc4da25b8..d71ee2b61a7 100644 > --- a/gcc/jit/jit-playback.cc > +++ b/gcc/jit/jit-playback.cc > @@ -509,7 +509,8 @@ new_function (location *loc, > const char *name, > const auto_vec<param *> *params, > int is_variadic, > - enum built_in_function builtin_id) > + enum built_in_function builtin_id, > + int is_target_builtin) > { > int i; > param *param; > @@ -543,6 +544,14 @@ new_function (location *loc, > DECL_RESULT (fndecl) = resdecl; > DECL_CONTEXT (resdecl) = fndecl; > > + if (is_target_builtin) > + { > + tree *decl = target_builtins.get (name); > + if (decl != NULL) > + fndecl = *decl; > + else > + add_error (loc, "cannot find target builtin %s", name); > + } It looks like is_target_builtin is only ever used as a flag, so for clarity's sake, can it and the various m_is_target_builtin be a bool rather than an int? > if (builtin_id) > { > gcc_assert (loc == NULL); > diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h > index f9e29d0baec..06128b4d640 100644 > --- a/gcc/jit/jit-playback.h > +++ b/gcc/jit/jit-playback.h > @@ -104,7 +104,8 @@ public: > const char *name, > const auto_vec<param *> *params, > int is_variadic, > - enum built_in_function builtin_id); > + enum built_in_function builtin_id, > + int is_target_builtin); > > lvalue * > new_global (location *loc, > @@ -818,4 +819,6 @@ extern playback::context *active_playback_ctxt; > > } // namespace gcc > > +extern hash_map<nofree_string_hash, tree> target_builtins; > + > #endif /* JIT_PLAYBACK_H */ [...snip...] Assuming that the i386 maintainers are OK with the change to i386- builtins.cc, this is OK for trunk if you fix the various nitpicks I mention above (and you'll probably need to do the usual refreshing of the ABI version) On Thu, 2023-11-23 at 17:21 -0500, Antoni Boucher wrote: > I will need to not forget to update the function > tree_type_to_jit_type > in dummy-frontend.cc to add back the support for bfloat16 when the > patch for it is merged. Adding this here as a reminder. Thanks again for the patch. Dave
