On Thu, 2024-11-14 at 15:27 -0500, Antoni Boucher wrote: > It seems we don't need to do the cleanup in i386-builtins.cc anymore, > so > I removed it. > David: Is it possible that your recent fixes for the GC within > libgccjit > also fixed the issue here? > > Here's the updated patch and answers below. > > (GitHub link if you find it easier for review: > https://github.com/antoyo/libgccjit/pull/5) > > Thanks.
Thanks; I looked over the patch via the above link and it looks good to me for trunk. Dave > > Le 2024-06-26 à 18 h 49, David Malcolm a écrit : > > 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. > > Done > > > > > [...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? > > It is also used in jit-playback.cc: does that mean it needs a GTY > marker? > > > > > > +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. > > I added a comment. > > > > > 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... > > Indeed, this is used as a global cache, as you noticed below. > > > > > > + > > > /* 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? > > While it is a timebomb, we cannot error out here as this process all > the > builtins, so any unsupported type needs to be processed. > The current way allows to use most builtins and usage of a few > builtins > will have incorrect type checking. > > > > > [...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? > > Done. > > > > > > > > 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. > > Done as well. > > > > > Thanks again for the patch. > > Dave
