<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Nov 25, 2020 at 10:13 AM Nicolai Hähnle via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Nov 25, 2020 at 12:06 AM Joerg Sonnenberger via llvm-dev<br>
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
> On Tue, Nov 24, 2020 at 04:52:32PM -0500, James Y Knight wrote:<br>
> > In any case, what it lowers to in Clang is the LLVM IR function-attribute<br>
> > readnone, which I'd argue is even more clearly correct to use on an LLVM<br>
> > function returning the address of a TLS global variable. Note that llvm<br>
> > infers that property via the FunctionAttrs pass, e.g. for this function:<br>
> > @t = thread_local global i32 0, align 4<br>
> > define i32* @foo() {<br>
> > ret i32* @t<br>
> > }<br>
><br>
> I don't see how that is a valid transformation under the definition in<br>
> the language reference either. I also don't believe that this is the<br>
> only situation it can happen, I would expect OpenMP to expose similar<br>
> issues.</blockquote><div><br></div><div>Clang/LLVM OpenMP support <i>doesn't</i> expose similar issues, because during the design work in 2012, the explicit choice was made to outline the parallel regions to a separate function up-front. It does <i>not</i> expose an llvm IR intrinsic which switches to another (or multiple!) thread of execution. The ultimately-unsuccessful competing proposals did propose that, and were rejected because of it.</div><div><br></div><div>That choice greatly reduced the ability to optimize OpenMP parallel loops (e.g., no LICM, even where it'd be correct!). But, this was done precisely in order to avoid having to teach all the LLVM optimization passes about the rules for optimizing a function body running on multiple threads simultaneously.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The tricky part is that the "thread self identity" is a piece of state<br>
that used to be immutable, but with the introduction of coroutines, it<br>
has become mutable. The example @foo reads the "thread self identity"<br>
piece of state. As long as that state was immutable, it was correct to<br>
mark the function as readnone. Now that the state has become mutable,<br>
this is no longer correct.<br>
<br>
That may seem annoying, but I suspect it's best to just swallow that<br>
pill and take it to its logical conclusion.<br>
<br>
Doing so ends up treating pthread_self(), @foo, etc. more<br>
conservatively, but correctly. Treating them correctly and less<br>
conservatively is really a kind of alias analysis problem. I would<br>
hesitate to just add a new ad-hoc attribute for it, we already have so<br>
many of those, and would prefer adding a mechanism for this kind of<br>
thing generically.<br></blockquote><div><br></div><div><div>While it is certainly important to correctly model the fiction that the coroutine intrinsics present, I do think it's important to remember that they <i>are</i> a fiction within the compiler. Yes, in the high-level coroutine IR, before CoroSplit runs, thread identity acts as if it were mutable across the "llvm.coro.suspend" intrinsic. However, the suspend intrinsic is simply a placeholder for the transform which will ultimately occur, to turn the entire function body inside-out. This is an important distinction, because there remains no way to "secretly" mutate the thread identity. E.g. calling a function cannot result in the thread identity of the caller being mutated.</div></div><div><div></div></div><div><br></div><div>That we've introduced this set of coroutine intrinsics (and the CoroSplit pass to flip a function inside out) does <i>not</i> imply that we ought to change the meaning of 'readnone' retroactively to mean that it needs to be invariant on thread-identity. We should continue to model it as meaning "doesn't read mutable memory or mutable state, but may depend on the thread identity", which is what the meaning has been up until now, even if not explicitly stated. Given the existence of an intrinsic which "changes" the thread identity, we should, now, clarify the documentation. </div><div><div><br></div><div>We also do need to fix any optimization passes that run prior to the CoroSplit pass understand the effect that a coroutine suspend-point has on thread-identity. Note that we need to do much of the same work <i>anyways</i>, to fix access to TLS variables within a coroutine. (Or, as an alternative, we could move the CoroSplit pass first in the optimization pipeline, so that there are no such optimization passes which run before it. This would fix the correctness issues, but greatly reduce the potential for optimization -- similar to OpenMP)</div></div><div><br></div><div>Finally, it turns out to be important, it may be reasonable to add a <i>new</i> function attribute to indicate "doesn't depend on current thread identity", which could unlock additional optimization potential even across a suspend-point. But, I would not suggest actually adding it, unless a real need has been identified and justified.</div><div><div></div><div></div></div></div></div>