From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 102892 invoked by alias); 8 Jan 2019 21:06:11 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 102860 invoked by uid 89); 8 Jan 2019 21:06:10 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,FREEMAIL_FROM,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wr1-f53.google.com Received: from mail-wr1-f53.google.com (HELO mail-wr1-f53.google.com) (209.85.221.53) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 08 Jan 2019 21:06:06 +0000 Received: by mail-wr1-f53.google.com with SMTP id t27so5488789wra.6; Tue, 08 Jan 2019 13:06:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:subject:to:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=eioTuGOJhddJmYV70yt8BLR+meEKUXbazep9FAMLO6U=; b=reAovEDbk1TD1VUjuRQ++fAu6L0oAM0gXEI/EFai7kFugtJm3o2RF/Y6YOuGr0Z/y4 h61FbmhHiNnrZAoOrzohOzkOquG99m9o7gRzcJvJu68tStEDmq8fAYhRIlBJbdxs3syW OegAtCcpXmDRcH3gjsDPVvXrIDm4zHS8ph96XGbekrYw0cBbER4SkvqRrp0ZEHppOTfg wUUnHUDFoMEUg+IO+zHQvSpr1KOCjMRLCEHY1hot5644yTjhVQpfdkR4Bzzri9W4ntZw c1AxW3YXS58gZ0DRJ2rJxEDR8tLMOlz+3JJdN803lo71mHxab82e8m8yNiRKBZR83Frl BgEA== Return-Path: Received: from ?IPv6:2a02:810d:340:25f8:46d:f2b2:3936:4d0a? ([2a02:810d:340:25f8:46d:f2b2:3936:4d0a]) by smtp.gmail.com with ESMTPSA id 125sm16252597wmm.26.2019.01.08.13.06.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 08 Jan 2019 13:06:01 -0800 (PST) Sender: =?UTF-8?Q?Marc_Nieper=2DWi=C3=9Fkirchen?= Subject: Re: [PATCH][jit] Add thread-local globals to the libgccjit frontend To: David Malcolm , =?UTF-8?Q?Marc_Nieper-Wi=c3=9fkirchen?= , jit@gcc.gnu.org, gcc-patches@gcc.gnu.org References: <1546893285.7788.47.camel@redhat.com> <24366a6a-e0e6-e0e6-360f-dc67f21a57f8@nieper-wisskirchen.de> <1546968499.7788.80.camel@redhat.com> From: =?UTF-8?Q?Marc_Nieper-Wi=c3=9fkirchen?= Message-ID: Date: Tue, 08 Jan 2019 21:06:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <1546968499.7788.80.camel@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-SW-Source: 2019-01/txt/msg00433.txt.bz2 [...] >> 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. > Thanks for explaining the policy. Corrected ChangeLogs: gcc/jit/ChangeLog ================= 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. gcc/testsuite/ChangeLog ======================= 2019-01-08 Marc Nieper-Wißkirchen * jit.dg/all-non-failing-tests.h: Include new test. * jit.dg/jit.exp: Load pthread for tests involving thread-local globals. * jit.dg/test-thread-local.c: New test case for thread-local globals. [...] >> 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); The test does check reading the thread-local variable in the main thread. If the variable were non thread-local, the value 42 set in the main thread would have been overwritten by the subordinate thread. > > 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. Is the following extended test-case along the lines you have been thinking? 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..e21f144c4 --- /dev/null +++ b/gcc/testsuite/jit.dg/test-thread-local.c @@ -0,0 +1,161 @@ +#include +#include +#include +#include + +#include "libgccjit.h" + +#include "harness.h" + +int _Thread_local tl_c; + +void +create_code (gcc_jit_context *ctxt, void *user_data) +{ + /* Let's try to inject the equivalent of: + + extern thread_local int tl_c; + static thread_ int tl_jit; + set_tl_jit (int v) + { + tl_jit = v; + } + + int + get_tl_jit (void) + { + return tl_jit; + } + + set_tl_c (int v) + { + tl_c = v; + } + + int + get_tl_c (void) + { + return tl_c; + } + */ + 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_jit"); + 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_jit", + 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_jit", + 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)); + + tl = + gcc_jit_context_new_global (ctxt, NULL, GCC_JIT_GLOBAL_IMPORTED, + the_type, "tl_c"); + gcc_jit_lvalue_set_bool_thread_local (tl, 1); + + v = + gcc_jit_context_new_param (ctxt, NULL, the_type, "v"); + params[0] = v; + func = + gcc_jit_context_new_function (ctxt, NULL, + GCC_JIT_FUNCTION_EXPORTED, + void_type, + "set_tl_c", + 1, params, 0); + 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_c", + 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)); +} + +typedef void (*set_tl_fn_type) (int); +typedef int (*get_tl_fn_type) (void); + +static set_tl_fn_type set_tl_jit, set_tl_c; +static get_tl_fn_type get_tl_jit, get_tl_c; + +static void * +test_thread_local_run (void *arg) +{ + set_tl_jit (43); + set_tl_c (101); + int val = get_tl_jit (); + CHECK_VALUE (val, 43); + val = get_tl_c (); + CHECK_VALUE (val, 101); + return NULL; +} + +void +verify_code (gcc_jit_context *ctxt, gcc_jit_result *result) +{ + + CHECK_NON_NULL (result); + + set_tl_jit = + (set_tl_fn_type) gcc_jit_result_get_code (result, "set_tl_jit"); + get_tl_jit = + (get_tl_fn_type) gcc_jit_result_get_code (result, "get_tl_jit"); + set_tl_c = + (set_tl_fn_type) gcc_jit_result_get_code (result, "set_tl_c"); + get_tl_c = + (get_tl_fn_type) gcc_jit_result_get_code (result, "get_tl_c"); + + CHECK_NON_NULL (set_tl_jit); + CHECK_NON_NULL (get_tl_jit); + CHECK_NON_NULL (set_tl_c); + CHECK_NON_NULL (get_tl_c); + + set_tl_jit (42); + set_tl_c (100); + + pthread_t thread; + CHECK_VALUE (pthread_create(&thread, NULL, test_thread_local_run, NULL), 0); + CHECK_VALUE (pthread_join(thread, NULL), 0); + + int val = get_tl_jit (); + note ("get_tl_jit returned: %d", val); + CHECK_VALUE (val, 42); + + val = get_tl_c (); + note ("get_tl_c returned: %d", val); + CHECK_VALUE (val, 100); +} > ...or somesuch, though the patch is in pretty good shape already. Thanks! Your information at https://gcc.gnu.org/onlinedocs/jit/internals/index.html has been extremely helpful. [...]