[PATCH] libgccjit: Add support for machine-dependent builtins
David Malcolm
dmalcolm@redhat.com
Wed Jun 26 22:49:55 GMT 2024
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 <bouanto@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
More information about the Jit
mailing list