From: David Malcolm <dmalcolm@redhat.com>
To: "Marc Nieper-Wißkirchen" <marc.nieper+gnu@gmail.com>,
jit@gcc.gnu.org, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH][jit] Add thread-local globals to the libgccjit frontend
Date: Tue, 01 Jan 2019 00:00:00 -0000 [thread overview]
Message-ID: <1546893285.7788.47.camel@redhat.com> (raw)
In-Reply-To: <CAEYrNrSZH-tR4JExg6bFiz+g-9GRdh=oWZzgikin-0MZK-mmfw@mail.gmail.com>
On Sat, 2019-01-05 at 23:10 +0100, Marc Nieper-WiÃkirchen wrote:
> This patch adds thread-local globals to the libgccjit frontend. The
> library user can mark a global as being thread-local by calling
> `gcc_jit_lvalue_set_bool_thread_local'. It is implemented by calling
> `set_decl_tls_model (inner, decl_default_tls_model (inner))', where
> `inner' is the GENERIC tree corresponding to the global.
Thanks for creating this patch.
Have you done the legal paperwork with the FSF for contributing to GCC?
See https://gcc.gnu.org/contribute.html#legal
Overall, this looks pretty good (though we'd want to get release
manager approval for late-breaking changes given where we are in the
release cycle).
Various comments inline below.
> ChangeLog
>
> 2019-01-05 Marc Nieper-WiÃkirchen <marc@nieper-wisskirchen.de>
>
> * 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.
>
> Testing
>
> The result of `make check-jit' is (the failing test in
> `test-sum-squares.c` is also failing without this patch on my
> machine):
>
> Native configuration is x86_64-pc-linux-gnu
[...]
> FAIL: test-combination.c.exe iteration 1 of 5:
> verify_code_sum_of_squares: dump_vrp1: actual: "
> FAIL: test-combination.c.exe killed: 20233 exp6 0 0 CHILDKILLED
> SIGABRT SIGABRT
> FAIL: test-sum-of-squares.c.exe iteration 1 of 5: verify_code:
> dump_vrp1: actual: "
> FAIL: test-sum-of-squares.c.exe killed: 22698 exp6 0 0 CHILDKILLED
> SIGABRT SIGABRT
> FAIL: test-threads.c.exe: verify_code_sum_of_squares: dump_vrp1:
> actual: "
> FAIL: test-threads.c.exe killed: 22840 exp6 0 0 CHILDKILLED SIGABRT
> SIGABRT
That one's failing for me as well. I'm investigating (I've filed it as
PR jit/88747).
[...]
diff --git a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> index bf02e1258..c2654ff09 100644
> --- a/gcc/testsuite/jit.dg/all-non-failing-tests.h
> +++ b/gcc/testsuite/jit.dg/all-non-failing-tests.h
> @@ -224,6 +224,13 @@
> #undef create_code
> #undef verify_code
>
> +/* test-factorial-must-tail-call.c */
Looks like a cut&paste error: presumably the above comment is meant to
refer to the new test...
> +#define create_code create_code_thread_local
> +#define verify_code verify_code_thread_local
> +#include "test-thread-local.c"
...but it looks like the new test file is missing from the patch.
> +#undef create_code
> +#undef verify_code
> +
> /* test-types.c */
> #define create_code create_code_types
> #define verify_code verify_code_types
> @@ -353,6 +360,9 @@ const struct testcase testcases[] = {
> {"switch",
> create_code_switch,
> verify_code_switch},
> + {"thread_local",
> + create_code_thread_local,
> + verify_code_thread_local},
> {"types",
> create_code_types,
> verify_code_types},
[...]
Thanks
Dave
next prev parent reply other threads:[~2019-01-07 20:34 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-01 0:00 Marc Nieper-Wißkirchen
2019-01-01 0:00 ` David Malcolm [this message]
2019-01-01 0:00 ` [committed] Fix jit test case (PR jit/88747) David Malcolm
2019-01-01 0:00 ` [PATCH][jit] Add thread-local globals to the libgccjit frontend Marc Nieper-Wißkirchen
2019-01-01 0:00 ` David Malcolm
2019-01-01 0:00 ` Marc Nieper-Wißkirchen
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=1546893285.7788.47.camel@redhat.com \
--to=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jit@gcc.gnu.org \
--cc=marc.nieper+gnu@gmail.com \
/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).