public inbox for jit@gcc.gnu.org
 help / color / mirror / Atom feed
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



  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).