From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 89954 invoked by alias); 8 Jan 2019 17:28:26 -0000 Mailing-List: contact jit-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Subscribe: Sender: jit-owner@gcc.gnu.org Received: (qmail 89931 invoked by uid 89); 8 Jan 2019 17:28:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Checked: by ClamAV 0.100.2 on sourceware.org X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy= X-Spam-Status: No, score=-25.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,SPF_HELO_PASS autolearn=ham version=3.3.2 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on sourceware.org X-Spam-Level: X-Spam-User: qpsmtpd, 2 recipients X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 08 Jan 2019 17:28:23 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id EB4F8C0AF7BA; Tue, 8 Jan 2019 17:28:21 +0000 (UTC) Received: from ovpn-116-42.phx2.redhat.com (ovpn-116-42.phx2.redhat.com [10.3.116.42]) by smtp.corp.redhat.com (Postfix) with ESMTP id D946660123; Tue, 8 Jan 2019 17:28:20 +0000 (UTC) Message-ID: <1546968499.7788.80.camel@redhat.com> Subject: Re: [PATCH][jit] Add thread-local globals to the libgccjit frontend From: David Malcolm To: Marc =?ISO-8859-1?Q?Nieper-Wi=DFkirchen?= , Marc =?ISO-8859-1?Q?Nieper-Wi=DFkirchen?= , jit@gcc.gnu.org, gcc-patches@gcc.gnu.org Date: Tue, 01 Jan 2019 00:00:00 -0000 In-Reply-To: <24366a6a-e0e6-e0e6-360f-dc67f21a57f8@nieper-wisskirchen.de> References: <1546893285.7788.47.camel@redhat.com> <24366a6a-e0e6-e0e6-360f-dc67f21a57f8@nieper-wisskirchen.de> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Tue, 08 Jan 2019 17:28:22 +0000 (UTC) X-IsSubscribed: yes X-SW-Source: 2019-q1/txt/msg00010.txt.bz2 On Tue, 2019-01-08 at 14:31 +0100, Marc Nieper-Wißkirchen wrote: > Dear David, > > thank you very much for your timely response and for talking a > thorough > look at my proposed patch. > > Am 07.01.19 um 21:34 schrieb David Malcolm: > > > Have you done the legal paperwork with the FSF for contributing to > > GCC? > > See https://gcc.gnu.org/contribute.html#legal > > Not yet; this is my first patch I would like to contribute to GCC. > You > should have received a private email to get the legal matters done. Thanks; I've replied to that. [...] > I have applied your recent patch. With the patch, there are no more > failures. Excellent. [...] > # of expected passes 10394 [...] > 2019-01-08 Marc Nieper-Wißkirchen > > * docs/topics/compatibility.rst: Add LIBGCCJIT_ABI_11. > * docs/topics/expressions.rst (Global variables): Add > documentation of gcc_jit_lvalue_set_bool_thread_local. > * docs/_build/texinfo/libgccjit.texi: Regenerate. > * jit-playback.c: Include "varasm.h". > Within namespace gcc::jit::playback... > (context::new_global) Add "thread_local_p" param and use it > to set DECL_TLS_MODEL. > * jit-playback.h: Within namespace gcc::jit::playback... > (context::new_global): Add "thread_local_p" param. > * jit-recording.c: Within namespace gcc::jit::recording... > (global::replay_into): Provide m_thread_local to call to > new_global. > (global::write_reproducer): Call write_reproducer_thread_local. > (global::write_reproducer_thread_local): New method. > * jit-recording.h: Within namespace gcc::jit::recording... > (lvalue::dyn_cast_global): New virtual function. > (global::m_thread_local): New field. > * libgccjit.c (gcc_jit_lvalue_set_bool_thread_local): New > function. > * libgccjit.h > (LIBGCCJIT_HAVE_gcc_jit_lvalue_set_bool_thread_local): New > macro. > (gcc_jit_lvalue_set_bool_thread_local): New function. > * libgccjit.map (LIBGCCJIT_ABI_11): New. > (gcc_jit_lvalue_set_bool_thread_local): Add. > * ../testsuite/jit.dg/all-non-failing-tests.h: Include new > test. > * ../testsuite/jit.dg/jit.exp: Load pthread for tests involving > thread-local globals. > * ../testsuite/jit.dg/test-thread-local.c: New test case for > thread-local globals. BTW, the convention here is to split out the ChangeLog entries by directory based on the presence of ChangeLog files. There's a gcc/jit/ChangeLog and a gcc/testsuite/ChangeLog, so for this patch there should be two sets of ChangeLog entries, one for each of these, with the paths expressed relative to the directory holding the ChangeLog. So the testsuite entries would go into gcc/testsuite/ChangeLog, and look like: * jit.dg/all-non-failing-tests.h: Include new test. ...etc. [...] > diff --git a/gcc/testsuite/jit.dg/test-thread-local.c > b/gcc/testsuite/jit.dg/test-thread-local.c > new file mode 100644 > index 000000000..287ba85e4 > --- /dev/null > +++ b/gcc/testsuite/jit.dg/test-thread-local.c > @@ -0,0 +1,99 @@ > +#include > +#include > +#include > +#include > + > +#include "libgccjit.h" > + > +#include "harness.h" > + > +void > +create_code (gcc_jit_context *ctxt, void *user_data) > +{ > + /* Let's try to inject the equivalent of: > + > + static thread_local int tl; > + set_tl (int v) > + { > + tl = v; > + } > + > + int > + get_tl (void) > + { > + return tl; > + } Thanks for posting this. This test is OK as far as it goes, but there are some gaps in test coverage: the test verifies that jit-generated code can create a new thread-local variable, and writes to it, but it doesn't seem to test reading from it; e.g. there doesn't seem to be a CHECK_VALUE that the thread-local var is 43 after the set_tl (43); It would probably also be good to verify that jit-generated code can work with a thread-local variable declared and defined in C-generated code. Thinking aloud, how about the following changes: - split out the thread-local vars there's e.g. a extern thread_local int tl_c; in the C code and the "tl" becomes "tl_jit", and access it via a GCC_JIT_GLOBAL_EXTERNAL, or somesuch. - in the test code, run two new threads, passing them two different "int" values; verify that the set_tl(value); can be matched with a CHECK_VALUE get_tl() == the value for that thread on the two threads. ...or somesuch, though the patch is in pretty good shape already. Dave > + */ > + gcc_jit_type *the_type = > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_INT); > + gcc_jit_type *void_type = > + gcc_jit_context_get_type (ctxt, GCC_JIT_TYPE_VOID); > + > + gcc_jit_lvalue *tl = > + gcc_jit_context_new_global (ctxt, NULL, GCC_JIT_GLOBAL_INTERNAL, > + the_type, "tl"); > + gcc_jit_lvalue_set_bool_thread_local (tl, 1); > + > + gcc_jit_param *v = > + gcc_jit_context_new_param (ctxt, NULL, the_type, "v"); > + gcc_jit_param *params[1] = {v}; > + gcc_jit_function *func = > + gcc_jit_context_new_function (ctxt, NULL, > + GCC_JIT_FUNCTION_EXPORTED, > + void_type, > + "set_tl", > + 1, params, 0); > + gcc_jit_block *block = > + gcc_jit_function_new_block (func, "initial"); > + gcc_jit_block_add_assignment (block, NULL, tl, > + gcc_jit_param_as_rvalue (v)); > + gcc_jit_block_end_with_void_return (block, NULL); > + > + func = > + gcc_jit_context_new_function (ctxt, NULL, > + GCC_JIT_FUNCTION_EXPORTED, > + the_type, > + "get_tl", > + 0, NULL, 0); > + block = > + gcc_jit_function_new_block (func, "initial"); > + gcc_jit_block_end_with_return (block, NULL, > + gcc_jit_lvalue_as_rvalue (tl)); > +} > + > +static void * > +test_thread_local_run (void *arg) > +{ > + void (*set_tl) (int) = arg; > + set_tl (43); > + return NULL; > +} > + > +void > +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) > +{ > + typedef void (*set_tl_fn_type) (int); > + typedef int (*get_tl_fn_type) (void); > + > + CHECK_NON_NULL (result); > + > + set_tl_fn_type set_tl = > + (set_tl_fn_type) gcc_jit_result_get_code (result, "set_tl"); > + get_tl_fn_type get_tl = > + (get_tl_fn_type) gcc_jit_result_get_code (result, "get_tl"); > + > + CHECK_NON_NULL (set_tl); > + CHECK_NON_NULL (get_tl); > + > + set_tl (42); > + > + pthread_t thread; > + CHECK_VALUE (pthread_create(&thread, NULL, test_thread_local_run, > set_tl), 0); > + CHECK_VALUE (pthread_join(thread, NULL), 0); > + > + int val = get_tl (); > + > + note ("get_tl returned: %d", val); > + CHECK_VALUE (val, 42); > +} >