Re: [PATCH] libgccjit: Add support for TLS variable [PR95415]
On Sat, 2021-11-20 at 17:34 -0500, Antoni Boucher wrote:
> 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.
That's not needed; I think all I need to know is what the next patch
you need me to look at is (FWIW I'm about to go on vacation for a week)
[...snip...]
>
> >
> > > +
> > > +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?
That would be ideal, but I don't think it's necessary.
> 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.
It might be that some targets only support some modes; I don't know.
[...snip...]
Thanks for the updated patch. It looks good to push to trunk once the
earlier ones are in place, though as usual please re-test it before
pushing.
Dave
Re: [PATCH] libgccjit: Add support for TLS variable [PR95415]
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-recor
Re: [PATCH] libgccjit: Add support for TLS variable [PR95415]
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, /* G
