From: David Malcolm <dmalcolm@redhat.com>
To: Antoni Boucher <bouanto@zoho.com>,
jit@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] libgccjit: Add support for TLS variable [PR95415]
Date: Thu, 20 May 2021 16:11:03 -0400 [thread overview]
Message-ID: <bf37419ef57b6ab11eed1e791a9416e6bfe7684a.camel@redhat.com> (raw)
In-Reply-To: <750892c90ccd75ae1df099b829ce2d51fe886195.camel@zoho.com>
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?
> 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.
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?
> 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?
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?
> +}
> +
> +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 prev parent reply other threads:[~2021-05-20 20:11 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-19 0:43 Antoni Boucher
2021-05-20 20:11 ` David Malcolm [this message]
2021-11-20 22:34 ` Antoni Boucher
2021-11-21 20:28 ` David Malcolm
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bf37419ef57b6ab11eed1e791a9416e6bfe7684a.camel@redhat.com \
--to=dmalcolm@redhat.com \
--cc=bouanto@zoho.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jit@gcc.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).