Re: [PATCH] libgccjit: Add support for machine-dependent builtins
On Thu, Jun 27, 2024 at 12:49 AM David Malcolm wrote:
>
> 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
> > 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).
Can we rather introduce TARGET_CLEANUP_BUILTINS hook and call it from
the JIT compiler at some appropriate time? IMO, this burdens
unnecessarily non-JIT compilation.
Uros.
Re: [PATCH] libgccjit: Make new_array_type take unsigned long
Thanks for the review. I'm a bit concerned about using unsigned long. Would it be OK if I change the type to uint64_t? I could rename the function to gcc_jit_context_new_array_type_u64. Regards. Le 2024-06-26 à 11 h 34, David Malcolm a écrit : On Fri, 2024-02-23 at 09:55 -0500, Antoni Boucher wrote: I had forgotten to add the doc since there is now a new API. Here it is. Sorry for the delay; the updated patch looks good to me (but may need usual ABI tag changes when pushing). Thanks Dave On Wed, 2024-02-21 at 19:45 -0500, Antoni Boucher wrote: Thanks for the review. Here's the updated patch. On Thu, 2023-12-07 at 20:04 -0500, David Malcolm wrote: On Thu, 2023-12-07 at 17:29 -0500, Antoni Boucher wrote: Hi. This patches update gcc_jit_context_new_array_type to take the size as an unsigned long instead of a int, to allow creating bigger array types. I haven't written the ChangeLog yet because I wasn't sure it's allowed to change the type of a function like that. If it isn't, what would you suggest? We've kept ABI compatibility all the way back to the version in GCC 5, so it seems a shame to break ABI. How about a new API entrypoint: gcc_jit_context_new_array_type_unsigned_long whilst keeping the old one. Then everything internally can use "unsigned long"; we just keep the old entrypoint accepting int (which internally promotes the arg to unsigned long, if positive, sharing all the implementation). Alternatively, I think there may be a way to do this with symbol versioning: https://gcc.gnu.org/wiki/SymbolVersioning see e.g. Section 3.7 of Ulrich Drepper's "How To Write Shared Libraries", but I'm a bit wary of cross-platform compatibility with that. Dave
Re: [PATCH] libgccjit: Fix get_size of size_t
Le 2024-06-26 à 18 h 01, David Malcolm a écrit : On Wed, 2024-02-21 at 14:16 -0500, Antoni Boucher wrote: On Thu, 2023-12-07 at 19:57 -0500, David Malcolm wrote: On Thu, 2023-12-07 at 17:26 -0500, Antoni Boucher wrote: Hi. This patch fixes getting the size of size_t (bug 112910). There's one issue with this patch: like every other feature that checks for target-specific stuff, it requires a compilation before actually fetching the size of the type. Which means that getting the size before a compilation might be wrong (and I actually believe is wrong on x86-64). I was wondering if we should always implicitely do the first compilation to gather the correct info: this would fix this issue and all the others that we have due to that. I'm not sure what would be the performance implication. Maybe introduce a new class target_info which contains all the information we might want to find via a compilation, and have the top- level recording::context have a pointer to it, which starts as nullptr, but can be populated on-demand the first time something needs it? That would mean that we'll need to populate it for every top-level context, right? Would the idea be that we should then use child contexts to have the proper information filled? If so, how is this different than just compiling two contexts like what I currently do? This would also mean that we'll do an implicit compilation whenever we use an API that needs this info, right? Wouldn't that be unexpected? I was thinking a compilation with an empty playback::context to lazily capture the target data. I'm still not sure I understand what you mean. Do you mean having a global context that we can compile to then fetch the size of the types? If not, could you please provide an example with some code? I'm wondering if we could have something that would also work for custom types like structs. I'm also not sure what would happen if options that change the size of types (like -m32) are provided by the user. Is the way libgccjit currently work (with 2 phases: recording and playback) this way because gcc is not thread-safe? If we could directly create GENERIC trees, we could get the size from those, but it seems like this would not be possible. My hope was that this would make things easier for users. But you're the one using this API, so if you're more comfortable with the explicit initial compilation approach, let's go with that. If so, this is OK for trunk - but we might want to add a note to the documentation about the double-compilation workaround. Dave Thanks for the idea. Another solution that I have been thinking about for a while now would be to have another frontend libgccaot (I don't like that name), which is like libgccjit but removes the JIT part so that we get access to the target stuff directly and would remove the need for having a seperation between recording and playback as far as I understand. That's a long-term solution, but I wanted to share the idea now and gather your thoughts on that. FWIW the initial version of libgccjit didn't have a split between recording and playback; instead the client code had to pass in a callback to call into the various API functions (creating tree nodes). See: https://gcc.gnu.org/legacy-ml/gcc-patches/2013-10/msg00228.html Dave
