<div dir="ltr"><div>LGTM</div><div><br></div><div>+ void setThreadLocalMode(ThreadLocalMode Val) {</div><div>+ assert(Val == NotThreadLocal || getValueID() != Value::FunctionVal);<br></div><div><br></div><div>I guess isa<> isn't available.</div>
<div><br></div><div>+ ThreadLocal = Val;</div><div>+ }</div><div>+ ThreadLocalMode getThreadLocalMode() const {</div><div>+ assert(ThreadLocal == NotThreadLocal || getValueID() != Value::FunctionVal);</div><div><br>
</div><div>This would trigger on MyGVThatIsAFunction->isThreadLocal(). It seems a bit surprising that you can't call the getter on any GlobalValue. I'd only assert in the setter.</div><div><br></div><div>+ return static_cast<ThreadLocalMode>(ThreadLocal);</div>
<div>+ }</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, May 28, 2014 at 7:55 AM, Rafael Espíndola <span dir="ltr"><<a href="mailto:rafael.espindola@gmail.com" target="_blank">rafael.espindola@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On 27 May 2014 16:43, Reid Kleckner <<a href="mailto:rnk@google.com">rnk@google.com</a>> wrote:<br>
> This adds some oversharing that we got rid of with the GlobalObject.<br>
> Functions inherit from GlobalValue, but they don't have a thread local mode.<br>
> Is that worth worrying about?<br>
<br>
</div>Not sure. It seems a case is harder to get wrong than the section and<br>
alignment were. To fix this we would need to bring in Nicks idea of<br>
using mixins.<br>
<div class=""><br>
> This also allows us to have thread local modes on aliases to functions,<br>
> which is awkward.<br>
<br>
</div>Yes, but this one seems a fundamental issue with having a generic<br>
ConstantExpr in a GlobalAlias. At the llvm level we cannot in general<br>
know if an alias in an alias to a function.<br>
<div class=""><br>
> Do you think it's OK to add asserts to setThreadLocalMode() to make sure we<br>
> don't get into these bad states? FWIW I don't think it's worth having a<br>
> GlobalVarAlias and GlobalFuncAlias.<br>
<br>
</div>It does'n seem to add much. I would be very opposed to a<br>
GlobalVarAlias and GlobalFuncAlais. If we are going to constraint what<br>
alias point to, lets just say it is a GlobalObject and offset and then<br>
it becomes easy to check that the aliasee matches whatever constraints<br>
we have.<br>
<div class=""><br>
> I'm OK with making frontends responsible for putting the thread local mode<br>
> on any aliases produced. Aliases to thread local GVs weren't very useful<br>
> before.<br>
><br>
> +variable). Not all targets support thread-local variables. Optionally, a<br>
> +TLS model may be specified:<br>
> +<br>
> +``localdynamic``<br>
> + For variables that are only used within the current shared library.<br>
> +``initialexec``<br>
> + For variables in modules that will not be loaded dynamically.<br>
> +``localexec``<br>
> + For variables defined in the executable and only used within it.<br>
> +<br>
><br>
> I noticed this should probably describe the behavior when no model is<br>
> specified, i.e. GeneralDynamic.<br>
<br>
</div>Good point. Added.<br>
<div class=""><br>
> +The models correspond to the ELF TLS models; see `ELF Handling For<br>
><br>
> diff --git a/test/CodeGen/X86/2008-03-12-ThreadLocalAlias.ll<br>
> b/test/CodeGen/X86/2008-03-12-ThreadLocalAlias.ll<br>
> index e64375a..a0106d7 100644<br>
> --- a/test/CodeGen/X86/2008-03-12-ThreadLocalAlias.ll<br>
> +++ b/test/CodeGen/X86/2008-03-12-ThreadLocalAlias.ll<br>
> @@ -8,7 +8,7 @@ target triple = "i386-pc-linux-gnu"<br>
> @__resp = thread_local global %struct.__res_state* @_res ;<br>
> <%struct.__res_state**> [#uses=1]<br>
> @_res = global %struct.__res_state zeroinitializer, section ".bss" ;<br>
> <%struct.__res_state*> [#uses=1]<br>
><br>
> -@__libc_resp = hidden alias %struct.__res_state** @__resp ;<br>
> <%struct.__res_state**> [#uses=2]<br>
> +@__libc_resp = hidden thread_local alias %struct.__res_state** @__resp ;<br>
> <%struct.__res_state**> [#uses=2]<br>
><br>
> This test case looks like it was reduced from a real libc. Do you have a<br>
> clang-side patch ready to go for when this lands?<br>
<br>
</div>No, we now actually match gcc's behaviour of requiring the model on the alias.<br>
<br>
The full declaration in glibc is<br>
<br>
extern __thread struct __res_state *__libc_resp<br>
__attribute__ ((alias ("__resp"))) attribute_hidden;<br>
<br>
There will be some changes needed to clang to fix pr19844 and PR19843,<br>
but I haven't started coding those yet.<br>
<br>
A Patch with the documentation update is attached.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote></div><br></div>