This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [Patch, updated] Make emulated TLS lto-friendly.
- From: Richard Henderson <rth at redhat dot com>
- To: IainS <developer at sandoe-acoustics dot co dot uk>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Diego Novillo <dnovillo at google dot com>, Jan Hubicka <hubicka at ucw dot cz>, Jakub Jelinek <jakub at redhat dot com>
- Date: Wed, 07 Jul 2010 16:22:13 -0700
- Subject: Re: [Patch, updated] Make emulated TLS lto-friendly.
- References: <ED394B6D-943B-4EC5-8ABF-B540166AF6D8@sandoe-acoustics.co.uk>
On 07/03/2010 07:10 AM, IainS wrote:
> + return GS_OK ;
s/ ;/;/g
There are at least 2 occurrences.
> + if ((DECL_TLS_MODEL (decl) >= TLS_MODEL_EMULATED))
Extra ().
Possibly better written as != TLS_MODEL_NONE as well.
> + if (value && DECL_P (value) &&
> + (DECL_TLS_MODEL (value) >= TLS_MODEL_EMULATED))
> + if (!DECL_INITIAL (to) &&
> + DECL_INITIAL (decl) && (DECL_INITIAL (decl) != error_mark_node))
> + if (! DECL_EXTERNAL (to) && ! DECL_COMMON (to))
Coding style -- && should be on the new line.
Extra ().
Merge the unnecessarily nested ifs.
> + /* ??? It is not enough to check DECL_COMMON because variables might be
> + allocated in other uninitialized sections. However, this might still
> + not be an adequate test. */
> + if (DECL_INITIAL (h->to))
Huh? Why is DECL_COMMON not sufficient?
> + /* Refuse to build a zero-sized item, first make sure there's
> + a valid layout. */
> + if (DECL_SIZE (h->base.from) == 0)
> + layout_decl (h->base.from, 0);
Really? What is a testcase for which the decl is not layed out?
That really sounds like a bug elsewhere.
> + gcc_assert (DECL_SIZE (h->base.from) != 0);
I'm pretty sure you can declare zero sized objects as a gcc extension.
$ cat z.c
__thread int x[0] __attribute__((common));
$ cat z.s
.file "z.c"
.tls_common x,0,4
.ident "GCC: (GNU) 4.4.4 20100630 (Red Hat 4.4.4-10)"
.section .note.GNU-stack,"",@progbits
So here's a test case that will trigger that abort.
> + /* We don't emit the userland vars for emulated TLS - they should never
> + get to here, only the control vars should be emitted. */
> + gcc_assert (targetm.have_tls
> + || (TREE_CODE (decl) != VAR_DECL)
> + || !DECL_THREAD_LOCAL_P (decl));
Extra (). I'm sure there are more, but I won't comment again.
This pattern occurs many places. Pull this out to a function.
> + /* If we need the var, and it's an emulated TLS entity, that
> + means we need the control var. */
> + if (!targetm.have_tls && DECL_THREAD_LOCAL_P (node->decl))
> + {
> + struct varpool_node *cv_node;
> + cv_node = varpool_node (emutls_decl (node->decl)) ;
> + varpool_mark_needed_node (cv_node);
> + }
Do you really need this kind of thing anymore, since you're exposing
the use of the control variable so early? I would have thought that
varpool.c would no longer need any special-casing for !have_tls.
Finally, as a follow-up, I think the alias oracle needs to be taught what
__builtin_emutls_get_address does. Namely, return the address of the
associated TLS variable. This is the only way I can think of to avoid
the optimization penalty that you're introducing because of exposing the
control variable and the function call at this point.
r~