Re: [PATCH] libgccjit: Add support for TLS variable [PR95415]

[David Malcolm <dmalcolm@redhat.com>]

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