[PATCH] libgccjit: Add support for machine-dependent builtins
Antoni Boucher
bouanto@zoho.com
Thu Nov 14 20:27:39 GMT 2024
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.
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 <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.
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libgccjit-Add-support-for-machine-dependent-builtins.patch
Type: text/x-patch
Size: 32803 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/jit/attachments/20241114/5ceb01ed/attachment-0001.bin>
More information about the Jit
mailing list