Re: [PATCH] Add rvalue::get_name method (and its C equivalent)
On Mon, 2024-04-22 at 19:56 +0200, Guillaume Gomez wrote:
> `param` is also inheriting from `lvalue`. I don't think adding this
> check is a good idea
> because it will not evolve nicely if more changes are done in
> libgccjit.
Sorry for not responding earlier.
I think I agree with Guillaume here.
Looking at the checklist at:
https://gcc.gnu.org/onlinedocs/jit/internals/index.html#submitting-patches
the patch is missing:
- a feature macro in libgccjit.h such as
#define LIBGCCJIT_HAVE_gcc_jit_lvalue_get_name
- documentation for the new C entrypoint
- documentation for the new ABI tag (see topics/compatibility.rst).
Other than that, the patch looks reasonable to me.
Dave
>
> Le lun. 22 avr. 2024 à 17:19, Antoni Boucher a
> écrit :
> >
> > For your new API endpoint, please add a check like:
> >
> > RETURN_IF_FAIL (lvalue->is_global () || lvalue->is_local (),
> > NULL,
> > NULL,
> > "lvalue should be a variable");
> >
> >
> > Le 2024-04-22 à 09 h 16, Guillaume Gomez a écrit :
> > > Good point!
> > >
> > > New patch attached.
> > >
> > > Le lun. 22 avr. 2024 à 15:13, Antoni Boucher a
> > > écrit :
> > > >
> > > > Please move the function to be on lvalue since there are no
> > > > rvalue types
> > > > that are not lvalues that have a name.
> > > >
> > > > Le 2024-04-22 à 09 h 04, Guillaume Gomez a écrit :
> > > > > Hey Arthur :)
> > > > >
> > > > > > Is there any reason for that getter to return a mutable
> > > > > > pointer to the
> > > > > > name? Would something like this work instead if you're just
> > > > > > looking at
> > > > > > getting the name?
> > > > > >
> > > > > > + virtual string * get_name () const { return NULL; }
> > > > > >
> > > > > > With of course adequate modifications to the inheriting
> > > > > > classes.
> > > > >
> > > > > Good catch, thanks!
> > > > >
> > > > > Updated the patch and attached the new version to this email.
> > > > >
> > > > > Cordially.
> > > > >
> > > > > Le lun. 22 avr. 2024 à 11:51, Arthur Cohen
> > > > > a écrit :
> > > > > >
> > > > > > Hey Guillaume :)
> > > > > >
> > > > > > On 4/20/24 01:05, Guillaume Gomez wrote:
> > > > > > > Hi,
> > > > > > >
> > > > > > > I just encountered the need to retrieve the name of an
> > > > > > > `rvalue` (if
> > > > > > > there is one) while working on the Rust GCC backend.
> > > > > > >
> > > > > > > This patch adds a getter to retrieve the information.
> > > > > > >
> > > > > > > Cordially.
> > > > > >
> > > > > > > virtual bool get_wide_int (wide_int *) const {
> > > > > > > return false; }
> > > > > > >
> > > > > > > + virtual string * get_name () { return NULL; }
> > > > > > > +
> > > > > > > private:
> > > > > > > virtual enum precedence get_precedence () const = 0;
> > > > > >
> > > > > > Is there any reason for that getter to return a mutable
> > > > > > pointer to the
> > > > > > name? Would something like this work instead if you're just
> > > > > > looking at
> > > > > > getting the name?
> > > > > >
> > > > > > + virtual string * get_name () const { return NULL; }
> > > > > >
> > > > > > With of course adequate modifications to the inheriting
> > > > > > classes.
> > > > > >
> > > > > > Best,
> > > > > >
> > > > > > Arthur
>
Re: [PATCH] libgccjit: Add ability to get the alignment of a type
On Thu, 2024-04-04 at 18:59 -0400, Antoni Boucher wrote: > Hi. > This patch adds a new API to produce an rvalue representing the > alignment of a type. > Thanks for the review. Patch looks good to me (but may need the usual ABI version updates when merging). Thanks; sorry for the delay in reviewing. Dave
Re: [PATCH] libgccjit: Make new_array_type take unsigned long
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: Allow comparing array types
On Thu, 2024-01-25 at 07:52 -0500, Antoni Boucher wrote: > Thanks. > Can we please agree on some wording to use so I know when the patch > can > be pushed. Especially since we're now in stage 4, it would help me if > you say something like "you can push to master". Sorry about the ambiguity. Yes, you can push this one. Thanks Dave > Regards. > > On Wed, 2024-01-24 at 12:14 -0500, David Malcolm wrote: > > On Fri, 2024-01-19 at 16:55 -0500, Antoni Boucher wrote: > > > Hi. > > > This patch allows comparing different instances of array types as > > > equal. > > > Thanks for the review. > > > > Thanks; the patch looks good to me. > > > > Dave > > >
Re: [PATCH] libgccjit: Add support for the type bfloat16
On Wed, 2024-02-21 at 10:56 -0500, Antoni Boucher wrote:
> Thanks for the review.
> Here's the updated patch.
Thanks for the update patch; sorry for the delay in reviewing.
The updated patch looks good for trunk.
Dave
>
> On Fri, 2023-12-01 at 12:45 -0500, David Malcolm wrote:
> > On Thu, 2023-11-16 at 17:20 -0500, Antoni Boucher wrote:
> > > I forgot to attach the patch.
> > >
> > > On Thu, 2023-11-16 at 17:19 -0500, Antoni Boucher wrote:
> > > > Hi.
> > > > This patch adds the support for the type bfloat16 (bug 112574).
> > > >
> > > > This was asked to be splitted from a another patch sent here:
> > > > https://gcc.gnu.org/pipermail/jit/2023q1/001607.html
> > > >
> > > > Thanks for the review.
> > >
> >
> > Thanks for the patch.
> >
> > > diff --git a/gcc/jit/jit-playback.cc b/gcc/jit/jit-playback.cc
> > > index 18cc4da25b8..7e1c97a4638 100644
> > > --- a/gcc/jit/jit-playback.cc
> > > +++ b/gcc/jit/jit-playback.cc
> > > @@ -280,6 +280,8 @@ get_tree_node_for_type (enum gcc_jit_types
> > > type_)
> > >
> > > case GCC_JIT_TYPE_FLOAT:
> > > return float_type_node;
> > > + case GCC_JIT_TYPE_BFLOAT16:
> > > + return bfloat16_type_node;
> >
> > The code to create bfloat16_type_node (in build_common_tree_nodes)
> > is
> > guarded by #ifdef HAVE_BFmode, so we should probably have a test
> > for
> > this in case GCC_JIT_TYPE_BFLOAT16 to at least add an error message
> > when it's NULL_TREE, rather than silently returning NULL_TREE and
> > crashing.
> >
> > [...]
> >
> > > diff --git a/gcc/testsuite/jit.dg/test-bfloat16.c
> > > b/gcc/testsuite/jit.dg/test-bfloat16.c
> > > new file mode 100644
> > > index 000..6aed3920351
> > > --- /dev/null
> > > +++ b/gcc/testsuite/jit.dg/test-bfloat16.c
> > > @@ -0,0 +1,37 @@
> > > +/* { dg-do compile { target x86_64-*-* } } */
> > > +
> > > +#include
> > > +#include
> > > +
> > > +#include "libgccjit.h"
> > > +
> > > +/* We don't want set_options() in harness.h to set -O3 so our
> > > little local
> > > + is optimized away. */
> > > +#define TEST_ESCHEWS_SET_OPTIONS
> > > +static void set_options (gcc_jit_context *ctxt, const char
> > > *argv0)
> > > +{
> > > +}
> >
> >
> > Please add a comment to all-non-failing-tests.h noting the
> > exclusion
> > of
> > this test case from the array.
> >
> > [...]
> >
> > > diff --git a/gcc/testsuite/jit.dg/test-types.c
> > > b/gcc/testsuite/jit.dg/test-types.c
> > > index a01944e35fa..9e7c4f3e046 100644
> > > --- a/gcc/testsuite/jit.dg/test-types.c
> > > +++ b/gcc/testsuite/jit.dg/test-types.c
> > > @@ -1,3 +1,4 @@
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -492,4 +493,5 @@ verify_code (gcc_jit_context *ctxt,
> > > gcc_jit_result *result)
> > >
> > > CHECK_VALUE (gcc_jit_type_get_size (gcc_jit_context_get_type
> > > (ctxt, GCC_JIT_TYPE_FLOAT)), sizeof (float));
> > > CHECK_VALUE (gcc_jit_type_get_size (gcc_jit_context_get_type
> > > (ctxt, GCC_JIT_TYPE_DOUBLE)), sizeof (double));
> > > + CHECK_VALUE (gcc_jit_type_get_size (gcc_jit_context_get_type
> > > (ctxt, GCC_JIT_TYPE_BFLOAT16)), sizeof (__bfloat16));
> >
> >
> > This is only going to work on targets which #ifdef HAVE_BFmode, so
> > this
> > CHECK_VALUE needs to be conditionalized somehow, to avoid having
> > this,
> > test-combination, and test-threads from bailing out early on
> > targets
> > without BFmode.
> >
> > Dave
> >
>
Re: Frontend access to target features (was Re: [PATCH] libgccjit: Add ability to get CPU features)
On Sun, 2024-03-10 at 12:05 +0100, Iain Buclaw wrote:
> Excerpts from David Malcolm's message of März 5, 2024 4:09 pm:
> > On Thu, 2023-11-09 at 19:33 -0500, Antoni Boucher wrote:
> > > Hi.
> > > See answers below.
> > >
> > > On Thu, 2023-11-09 at 18:04 -0500, David Malcolm wrote:
> > > > On Thu, 2023-11-09 at 17:27 -0500, Antoni Boucher wrote:
> > > > > Hi.
> > > > > This patch adds support for getting the CPU features in
> > > > > libgccjit
> > > > > (bug
> > > > > 112466)
> > > > >
> > > > > There's a TODO in the test:
> > > > > I'm not sure how to test that gcc_jit_target_info_arch
> > > > > returns
> > > > > the
> > > > > correct value since it is dependant on the CPU.
> > > > > Any idea on how to improve this?
> > > > >
> > > > > Also, I created a CStringHash to be able to have a
> > > > > std::unordered_set. Is there any built-in way
> > > > > of
> > > > > doing
> > > > > this?
> > > >
> > > > Thanks for the patch.
> > > >
> > > > Some high-level questions:
> > > >
> > > > Is this specifically about detecting capabilities of the host
> > > > that
> > > > libgccjit is currently running on? or how the target was
> > > > configured
> > > > when libgccjit was built?
> > >
> > > I'm less sure about this part. I'll need to do more tests.
> > >
> > > >
> > > > One of the benefits of libgccjit is that, in theory, we support
> > > > all
> > > > of
> > > > the targets that GCC already supports. Does this patch change
> > > > that,
> > > > or
> > > > is this more about giving client code the ability to determine
> > > > capabilities of the specific host being compiled for?
> > >
> > > This should not change that. If it does, this is a bug.
> > >
> > > >
> > > > I'm nervous about having per-target jit code. Presumably
> > > > there's a
> > > > reason that we can't reuse existing target logic here - can you
> > > > please
> > > > describe what the problem is. I see that the ChangeLog has:
> > > >
> > > > > * config/i386/i386-jit.cc: New file.
> > > >
> > > > where i386-jit.cc has almost 200 lines of nontrivial code.
> > > > Where
> > > > did
> > > > this come from? Did you base it on existing code in our source
> > > > tree,
> > > > making modifications to fit the new internal API, or did you
> > > > write
> > > > it
> > > > from scratch? In either case, how onerous would this be for
> > > > other
> > > > targets?
> > >
> > > This was mostly copied from the same code done for the Rust and D
> > > frontends.
> > > See this commit and the following:
> > > https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=b1c06fd9723453dd2b2ec306684cb806dc2b4fbb
> > > The equivalent to i386-jit.cc is there:
> > > https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=22e3557e2d52f129f2bbfdc98688b945dba28dc9
> >
> > [CCing Iain and Arthur re those patches; for reference, the patch
> > being
> > discussed is attached to :
> > https://gcc.gnu.org/pipermail/jit/2024q1/001792.html ]
> >
> > One of my concerns about this patch is that we seem to be gaining
> > code
> > that's per-(frontend x config) which seems to be copied and pasted
> > with
> > a search and replace, which could lead to an M*N explosion.
> >
>
> That's certainly the case with the configure/make rules. Itself I
> think
> is copied originally from the {cpu_type}-protos.h machinery.
>
> It might be worth pointing out that the c-family of front-ends don't
> have separate headers because their per-target macros are defined in
> {cpu_type}.h directly - for better or worse.
>
> > Is there any real difference between the per-config code for the
> > different frontends, or should there be a general "enumerate all
> > features of the target" hook that's independent of the frontend?
> > (but
> > perhaps calls into it).
> >
>
> As far as I understand, the configure parts should all be identical
> between tm_p, tm_d, tm_rust, ..., so would benefit from being
> templated
> to aid any other front-ends adding in their own per target hooks.
>
> > Am I right in thinking that (rustc with default LLVM backend) has
> > some
> > set of feature strings that both (rustc with rustc_codegen_gcc) and
> > gccrs are trying to emulate? If so, is it presumably a goal that
> > libgccjit gives identical results to gccrs? If so, would it be
> > crazy
> > for libgccjit to consume e.g. config/i386/i386-rust.cc ?
>
> I don't know whether libgccjit can just pull in directly the
> implementation of the rust target hooks here.
Sorry for the delay in responding.
I don't want to be in the business of maintaining a copy of the per-
target code for "jit", and I think it makes sense for libgccjit to
return identical information compared to gccrs.
So I think it would be ideal for jit to share code with rust for this,
rather than do a one-time copy-and-paste followed by a ongoing "keep
things updated" treadmill.
Presumably there would be Makefile.in issues given that e.g. Makefile
has i386-rust.o listed in:
# Target specific, Rust specific object file
RUST_TARGET_OBJS= i386-rust.o
Re: [PATCH] libgccjit: Fix get_size of size_t
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. 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 > > >
Re: [PATCH] libgccjit: Add support for machine-dependent builtins
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).
> 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
> +#include
> +#include
> +
> +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 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
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_funct
