[PATCH] libgccjit: Add support for TLS variable [PR95415]
Antoni Boucher
bouanto@zoho.com
Sat Nov 20 22:34:00 GMT 2021
Hi.
Here's the updated patch.
See comments below.
Thanks for your reviews!
Le jeudi 20 mai 2021 à 16:11 -0400, David Malcolm a écrit :
> On Tue, 2021-05-18 at 20:43 -0400, Antoni Boucher via Gcc-patches
> wrote:
> > Hello.
> > This patch adds support for TLS variables.
> > One thing to fix before we merge it is the libgccjit.map file which
> > contains LIBGCCJIT_ABI_16 instead of LIBGCCJIT_ABI_17.
> > LIBGCCJIT_ABI_16 was added in one of my other patches.
> > Thanks for the review.
>
> > diff --git a/gcc/jit/docs/topics/compatibility.rst
> > b/gcc/jit/docs/topics/compatibility.rst
> > index 239b6aa1a92..d10bc1df080 100644
> > --- a/gcc/jit/docs/topics/compatibility.rst
> > +++ b/gcc/jit/docs/topics/compatibility.rst
> > @@ -243,3 +243,12 @@ embedding assembler instructions:
> > * :func:`gcc_jit_extended_asm_add_input_operand`
> > * :func:`gcc_jit_extended_asm_add_clobber`
> > * :func:`gcc_jit_context_add_top_level_asm`
> > +
> > +.. _LIBGCCJIT_ABI_17:
> > +
> > +``LIBGCCJIT_ABI_17``
> > +-----------------------
> > +``LIBGCCJIT_ABI_17`` covers the addition of an API entrypoint to set
> > the
> > +thread-local storage model of a variable:
> > +
> > + * :func:`gcc_jit_lvalue_set_tls_model`
>
> Sorry about the delay in reviewing patches.
>
> Is there a summary somewhere of the various outstanding patches and
> their associated ABI versions? Are there dependencies between the
> patches?
The list of patches is there:
https://github.com/antoyo/libgccjit-patches but I don't keep them up-
to-date.
If that would help you, I could add a README to tell what is the new
ABI version for each patch.
I believe there might be some patches that depend on a previous one.
>
> > diff --git a/gcc/jit/docs/topics/expressions.rst
> b/gcc/jit/docs/topics/expressions.rst
> > index 396259ef07e..68defd6a311 100644
> > --- a/gcc/jit/docs/topics/expressions.rst
> > +++ b/gcc/jit/docs/topics/expressions.rst
> > @@ -539,6 +539,34 @@ where the rvalue is computed by reading from
> > the storage area.
> >
> > in C.
> >
> > +.. function:: void\
> > + gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue
> > *lvalue,\
> > + enum gcc_jit_tls_model
> > model)
> > +
> > + Make a variable a thread-local variable.
> > +
> > + The "model" parameter determines the thread-local storage model
> > of the "lvalue":
> > +
> > + .. type:: enum gcc_jit_tls_model
> > +
> > + .. c:macro:: GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC
> > +
> > + .. c:macro:: GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC
> > +
> > + .. c:macro:: GCC_JIT_TLS_MODEL_INITIAL_EXEC
> > +
> > + .. c:macro:: GCC_JIT_TLS_MODEL_LOCAL_EXEC
> > +
> > + .. c:macro:: GCC_JIT_TLS_MODEL_DEFAULT
> > +
> > + This is analogous to:
> > +
> > + .. code-block:: c
> > +
> > + _Thread_local int foo;
> > +
> > + in C.
>
> This comment needs the usual "This entrypoint was added in" text to
> state which API version it was added in.
>
> I confess to being a bit hazy on the different TLS models, and it's
> unclear to me what all the different enum values do. Is this
> equivalent to the various values for
> __attribute__((tls_model(VALUE)))
> ? This attribute is documented in
> https://gcc.gnu.org/onlinedocs/gcc/Common-Variable-Attributes.html,
> though sadly that document doesn't seem to have a good anchor for
> that
> attribute.
Yes, it is the equivalent of this attribute.
>
> https://gcc.gnu.org/onlinedocs/gcc/Thread-Local.html currently links
> to
> https://www.akkadia.org/drepper/tls.pdf "for a detailed explanation
> of
> the four thread-local storage addressing models, and how the runtime
> is
> expected to function."
>
> One thing that should be clarified: does GCC_JIT_TLS_MODEL_DEFAULT
> mean
> (a) thread-local storage, using a default model, or
> (b) non-thread-local storage i.e. normal storage.
>
> ?
>
> Reading the docs I thought it meant (a), but when I looked in more
> detail at the implementation it looks like it means (b); is it meant
> to? This needs clarifying.
>
> Are you using all of these enum values in your code? Is this
> something
> you need to expose for the rustc backend?
Yes, I use all of these enum values in the rustc gcc codegen.
>
>
> > Global variables
> > ****************
> >
> > diff --git a/gcc/jit/jit-playback.h b/gcc/jit/jit-playback.h
> > index 825a3e172e9..654a9c472d4 100644
> > --- a/gcc/jit/jit-playback.h
> > +++ b/gcc/jit/jit-playback.h
> > @@ -650,6 +650,8 @@ public:
> >
> > private:
> > context *m_ctxt;
> > +
> > +protected:
> > tree m_inner;
> > };
>
> As noted in another review, I don't think you need to make this
> protected...
>
> >
> > @@ -670,6 +672,12 @@ public:
> > rvalue *
> > get_address (location *loc);
> >
> > + void
> > + set_tls_model (enum tls_model tls_model)
> > + {
> > + set_decl_tls_model (m_inner, tls_model);
> > + }
>
> ...as I think you can use "as_tree ()" to get at m_inner here.
>
>
> > diff --git a/gcc/jit/jit-recording.c b/gcc/jit/jit-recording.c
> > index 117ff70114c..64f3ae2d8f9 100644
> > --- a/gcc/jit/jit-recording.c
> > +++ b/gcc/jit/jit-recording.c
> > @@ -3713,6 +3713,12 @@ recording::lvalue::get_address
> > (recording::location *loc)
> > return result;
> > }
> >
> > +void
> > +recording::lvalue::set_tls_model (enum gcc_jit_tls_model model)
> > +{
> > + m_tls_model = model;
> > +}
> > +
> > /* The implementation of class gcc::jit::recording::param. */
> >
> > /* Implementation of pure virtual hook
> > recording::memento::replay_into
> > @@ -4539,6 +4545,15 @@ recording::block::dump_edges_to_dot
> > (pretty_printer *pp)
> > # pragma GCC diagnostic pop
> > #endif
> >
> > +namespace recording {
> > +static const enum tls_model tls_models[] = {
> > + TLS_MODEL_GLOBAL_DYNAMIC, /* GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC */
> > + TLS_MODEL_LOCAL_DYNAMIC, /* GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC */
> > + TLS_MODEL_INITIAL_EXEC, /* GCC_JIT_TLS_MODEL_INITIAL_EXEC */
> > + TLS_MODEL_LOCAL_EXEC, /* GCC_JIT_TLS_MODEL_LOCAL_EXEC */
> > +};
> > +} /* namespace recording */
> > +
> > /* The implementation of class gcc::jit::recording::global. */
> >
> > /* Implementation of pure virtual hook
> > recording::memento::replay_into
> > @@ -4547,8 +4562,7 @@ recording::block::dump_edges_to_dot
> > (pretty_printer *pp)
> > void
> > recording::global::replay_into (replayer *r)
> > {
> > - set_playback_obj (
> > - m_initializer
> > + playback::lvalue *global = m_initializer
> > ? r->new_global_initialized (playback_location (r, m_loc),
> > m_kind,
> > m_type->playback_type (),
> > @@ -4560,7 +4574,12 @@ recording::global::replay_into (replayer *r)
> > : r->new_global (playback_location (r, m_loc),
> > m_kind,
> > m_type->playback_type (),
> > - playback_string (m_name)));
> > + playback_string (m_name));
> > + if (m_tls_model != GCC_JIT_TLS_MODEL_DEFAULT)
> > + {
> > + global->set_tls_model (recording::tls_models[m_tls_model]);
> > + }
> > + set_playback_obj (global);
> > }
> >
> > /* Override the default implementation of
>
> [...snip...]
>
> > @@ -4675,6 +4702,14 @@ recording::global::write_reproducer
> > (reproducer &r)
> > r.get_identifier_as_type (get_type ()),
> > m_name->get_debug_string ());
> >
> > + if (m_tls_model)
> > + {
>
> I think this conditional should be:
>
> if (m_tls_model != GCC_JIT_TLS_MODEL_DEFAULT)
>
> since the default value isn't 0. (Maybe it should be?)
>
> > + r.write (" gcc_jit_lvalue_set_tls_model (%s, /*
> gcc_jit_lvalue *lvalue */\n"
> > + " %s); /* enum
> > gcc_jit_tls_model model */\n",
> > + id,
> > + tls_model_enum_strings[m_tls_model]);
> > + }
> > +
> > if (m_initializer)
> > switch (m_type->dereference ()->get_size ())
> > {
>
> [...snip...]
>
> > class param : public lvalue
> > diff --git a/gcc/jit/libgccjit.c b/gcc/jit/libgccjit.c
> > index 0cc650f9810..768b99499cf 100644
> > --- a/gcc/jit/libgccjit.c
> > +++ b/gcc/jit/libgccjit.c
> > @@ -1953,6 +1953,24 @@ gcc_jit_lvalue_get_address (gcc_jit_lvalue
> > *lvalue,
> > return (gcc_jit_rvalue *)lvalue->get_address (loc);
> > }
> >
> > +/* Public entrypoint. See description in libgccjit.h.
> > +
> > + After error-checking, the real work is done by the
> > + gcc::jit::recording::lvalue::set_tls_model method in jit-
> > recording.c. */
> > +
> > +void
> > +gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,
> > + enum gcc_jit_tls_model model)
> > +{
> > + RETURN_IF_FAIL (lvalue, NULL, NULL, "NULL lvalue");
> > + JIT_LOG_FUNC (lvalue->get_context ()->get_logger ());
> > + RETURN_IF_FAIL_PRINTF1 (lvalue->is_global (), NULL, NULL,
> > + "lvalue \"%s\" not a global",
> > + lvalue->get_debug_string ());
>
> This should pass lvalue's context to the RETURN_IF_FAIL_PRINTF1, so
> that if it fails, the error is associated with the context.
>
>
> > + lvalue->set_tls_model (model);
> > +}
> > +
> > /* Public entrypoint. See description in libgccjit.h.
> >
> > After error-checking, the real work is done by the
> > diff --git a/gcc/jit/libgccjit.h b/gcc/jit/libgccjit.h
> > index 5c722c2c57f..2a52b351a49 100644
> > --- a/gcc/jit/libgccjit.h
> > +++ b/gcc/jit/libgccjit.h
> > @@ -722,6 +722,16 @@ enum gcc_jit_function_kind
> > GCC_JIT_FUNCTION_ALWAYS_INLINE
> > };
> >
> > +/* Thread local storage model. */
> > +enum gcc_jit_tls_model
> > +{
> > + GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC,
> > + GCC_JIT_TLS_MODEL_LOCAL_DYNAMIC,
> > + GCC_JIT_TLS_MODEL_INITIAL_EXEC,
> > + GCC_JIT_TLS_MODEL_LOCAL_EXEC,
> > + GCC_JIT_TLS_MODEL_DEFAULT,
>
> As noted above, should the DEFAULT one be the first?
I made it the first one.
>
> If DEFAULT means "not thread-local", maybe "NONE" would be clearer
> than
> "DEFAULT"?
>
> > +};
> > +
> > /* Create a function. */
> > extern gcc_jit_function *
> > gcc_jit_context_new_function (gcc_jit_context *ctxt,
> > @@ -1072,6 +1082,17 @@ extern gcc_jit_rvalue *
> > gcc_jit_lvalue_get_address (gcc_jit_lvalue *lvalue,
> > gcc_jit_location *loc);
> >
> > +#define LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model
> > +
> > +/* Set the thread-local storage model of a global variable
> > +
> > + This API entrypoint was added in LIBGCCJIT_ABI_17; you can test
> > for its
> > + presence using
> > + #ifdef LIBGCCJIT_HAVE_gcc_jit_lvalue_set_tls_model */
> > +extern void
> > +gcc_jit_lvalue_set_tls_model (gcc_jit_lvalue *lvalue,
> > + enum gcc_jit_tls_model model);
> > +
> > extern gcc_jit_lvalue *
> > gcc_jit_function_new_local (gcc_jit_function *func,
> > gcc_jit_location *loc,
> > diff --git a/gcc/jit/libgccjit.map b/gcc/jit/libgccjit.map
> > index 337ea6c7fe4..605c624ec4a 100644
> > --- a/gcc/jit/libgccjit.map
> > +++ b/gcc/jit/libgccjit.map
> > @@ -205,3 +205,8 @@ LIBGCCJIT_ABI_15 {
> > gcc_jit_extended_asm_add_clobber;
> > gcc_jit_context_add_top_level_asm;
> > } LIBGCCJIT_ABI_14;
> > +
> > +LIBGCCJIT_ABI_16 {
> > + global:
> > + gcc_jit_lvalue_set_tls_model;
> > +} LIBGCCJIT_ABI_15;
>
> Obviously the ABI needs to be fixed up here.
>
> > diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > index 4202eb7798b..c2d87a30cca 100644
> > --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> > @@ -181,6 +181,13 @@
> > #undef create_code
> > #undef verify_code
> >
> > +/* test-tls.c */
> > +#define create_code create_code_tls
> > +#define verify_code verify_code_tls
> > +#include "test-tls.c"
> > +#undef create_code
> > +#undef verify_code
> > +
> > /* test-hello-world.c */
> > #define create_code create_code_hello_world
> > #define verify_code verify_code_hello_world
>
> This is missing an entry in the "testcases" array at the bottom of
> the
> header to make use of the new {create|verify}_code_tls functions.
>
> > diff --git a/gcc/testsuite/jit.dg/test-tls.c
> > b/gcc/testsuite/jit.dg/test-tls.c
> > new file mode 100644
> > index 00000000000..d4508b16c1e
> > --- /dev/null
> > +++ b/gcc/testsuite/jit.dg/test-tls.c
> > @@ -0,0 +1,29 @@
> > +#include <stdlib.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <time.h>
> > +
> > +#include "libgccjit.h"
> > +
> > +#include "harness.h"
> > +
> > +void
> > +create_code (gcc_jit_context *ctxt, void *user_data)
> > +{
> > + /* Let's try to inject the equivalent of:
> > +
> > + _Thread_local int foo;
> > + */
> > + gcc_jit_type *int_type =
> > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT);
> > +
> > + gcc_jit_lvalue *foo =
> > + gcc_jit_context_new_global (
> > + ctxt, NULL, GCC_JIT_GLOBAL_EXPORTED, int_type, "foo");
> > + gcc_jit_lvalue_set_tls_model (foo,
> > GCC_JIT_TLS_MODEL_GLOBAL_DYNAMIC);
>
> How many of the different enum values can be supported? How target-
> dependent is this?
I'm not sure what you mean here. Are you asking that I test all the
different enum values?
The tls_model enum is defined in gcc/coretypes.h and does not seem to
change depending on the target. Maybe there are checks elsewhere for
that, though.
>
> > +}
> > +
> > +void
> > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result)
> > +{
>
> Should probably at least try to read and write the global(s).
>
> Hope this is constructive
> Dave
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-libgccjit-Add-support-for-TLS-variable-PR95415.patch
Type: text/x-patch
Size: 13150 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/jit/attachments/20211120/f9f42bb0/attachment-0001.bin>
More information about the Jit
mailing list